| Message ID | 20230323024412.324085-2-fei2.wu@intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | target/riscv: reduce MSTATUS_SUM overhead | expand |
On 2023/3/23 10:44, Fei Wu wrote: > Currently it's assumed the 2 low bits of mmu_idx map to privilege mode, > this assumption won't last as we are about to add more mmu_idx. For patch set has more than 1 patch, usually add a cover letter. > > Signed-off-by: Fei Wu <fei2.wu@intel.com> > --- > target/riscv/cpu.h | 1 - > target/riscv/cpu_helper.c | 2 +- > target/riscv/insn_trans/trans_privileged.c.inc | 2 +- > target/riscv/insn_trans/trans_xthead.c.inc | 7 +------ > target/riscv/translate.c | 3 +++ > 5 files changed, 6 insertions(+), 9 deletions(-) > > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h > index 638e47c75a..66f7e3d1ba 100644 > --- a/target/riscv/cpu.h > +++ b/target/riscv/cpu.h > @@ -623,7 +623,6 @@ G_NORETURN void riscv_raise_exception(CPURISCVState *env, > target_ulong riscv_cpu_get_fflags(CPURISCVState *env); > void riscv_cpu_set_fflags(CPURISCVState *env, target_ulong); > > -#define TB_FLAGS_PRIV_MMU_MASK 3 > #define TB_FLAGS_PRIV_HYP_ACCESS_MASK (1 << 2) > #define TB_FLAGS_MSTATUS_FS MSTATUS_FS > #define TB_FLAGS_MSTATUS_VS MSTATUS_VS > diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c > index f88c503cf4..76e1b0100e 100644 > --- a/target/riscv/cpu_helper.c > +++ b/target/riscv/cpu_helper.c > @@ -762,7 +762,7 @@ static int get_physical_address(CPURISCVState *env, hwaddr *physical, > * (riscv_cpu_do_interrupt) is correct */ > MemTxResult res; > MemTxAttrs attrs = MEMTXATTRS_UNSPECIFIED; > - int mode = mmu_idx & TB_FLAGS_PRIV_MMU_MASK; > + int mode = env->priv; > bool use_background = false; > hwaddr ppn; > RISCVCPU *cpu = env_archcpu(env); > diff --git a/target/riscv/insn_trans/trans_privileged.c.inc b/target/riscv/insn_trans/trans_privileged.c.inc > index 59501b2780..9305b18299 100644 > --- a/target/riscv/insn_trans/trans_privileged.c.inc > +++ b/target/riscv/insn_trans/trans_privileged.c.inc > @@ -52,7 +52,7 @@ static bool trans_ebreak(DisasContext *ctx, arg_ebreak *a) > * that no exception will be raised when fetching them. > */ > > - if (semihosting_enabled(ctx->mem_idx < PRV_S) && > + if (semihosting_enabled(ctx->priv < PRV_S) && > (pre_addr & TARGET_PAGE_MASK) == (post_addr & TARGET_PAGE_MASK)) { > pre = opcode_at(&ctx->base, pre_addr); > ebreak = opcode_at(&ctx->base, ebreak_addr); > diff --git a/target/riscv/insn_trans/trans_xthead.c.inc b/target/riscv/insn_trans/trans_xthead.c.inc > index df504c3f2c..adfb53cb4c 100644 > --- a/target/riscv/insn_trans/trans_xthead.c.inc > +++ b/target/riscv/insn_trans/trans_xthead.c.inc > @@ -265,12 +265,7 @@ static bool trans_th_tst(DisasContext *ctx, arg_th_tst *a) > > static inline int priv_level(DisasContext *ctx) > { > -#ifdef CONFIG_USER_ONLY > - return PRV_U; > -#else > - /* Priv level is part of mem_idx. */ > - return ctx->mem_idx & TB_FLAGS_PRIV_MMU_MASK; > -#endif > + return ctx->priv; > } > > /* Test if priv level is M, S, or U (cannot fail). */ > diff --git a/target/riscv/translate.c b/target/riscv/translate.c > index 0ee8ee147d..e8880f9423 100644 > --- a/target/riscv/translate.c > +++ b/target/riscv/translate.c > @@ -69,6 +69,7 @@ typedef struct DisasContext { > uint32_t mstatus_hs_fs; > uint32_t mstatus_hs_vs; > uint32_t mem_idx; > + uint32_t priv; > /* Remember the rounding mode encoded in the previous fp instruction, > which we have already installed into env->fp_status. Or -1 for > no previous fp instruction. Note that we exit the TB when writing > @@ -1162,8 +1163,10 @@ static void riscv_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs) > } else { > ctx->virt_enabled = false; > } > + ctx->priv = env->priv; This is not right. You should put env->priv into tb flags before you use it in translation. Zhiwei > #else > ctx->virt_enabled = false; > + ctx->priv = PRV_U; > #endif > ctx->misa_ext = env->misa_ext; > ctx->frm = -1; /* unknown rounding mode */
On 3/23/2023 1:37 PM, LIU Zhiwei wrote: > > On 2023/3/23 10:44, Fei Wu wrote: >> Currently it's assumed the 2 low bits of mmu_idx map to privilege mode, >> this assumption won't last as we are about to add more mmu_idx. > For patch set has more than 1 patch, usually add a cover letter. This is cover letter: https://www.mail-archive.com/qemu-devel@nongnu.org/msg950849.html I added scripts/get_maintainer.pl to .git/config, it couldn't find out the maintainers for the cover letter, so I added the mail lists to "To" manually. >> >> Signed-off-by: Fei Wu <fei2.wu@intel.com> >> --- >> target/riscv/cpu.h | 1 - >> target/riscv/cpu_helper.c | 2 +- >> target/riscv/insn_trans/trans_privileged.c.inc | 2 +- >> target/riscv/insn_trans/trans_xthead.c.inc | 7 +------ >> target/riscv/translate.c | 3 +++ >> 5 files changed, 6 insertions(+), 9 deletions(-) >> >> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h >> index 638e47c75a..66f7e3d1ba 100644 >> --- a/target/riscv/cpu.h >> +++ b/target/riscv/cpu.h >> @@ -623,7 +623,6 @@ G_NORETURN void >> riscv_raise_exception(CPURISCVState *env, >> target_ulong riscv_cpu_get_fflags(CPURISCVState *env); >> void riscv_cpu_set_fflags(CPURISCVState *env, target_ulong); >> -#define TB_FLAGS_PRIV_MMU_MASK 3 >> #define TB_FLAGS_PRIV_HYP_ACCESS_MASK (1 << 2) >> #define TB_FLAGS_MSTATUS_FS MSTATUS_FS >> #define TB_FLAGS_MSTATUS_VS MSTATUS_VS >> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c >> index f88c503cf4..76e1b0100e 100644 >> --- a/target/riscv/cpu_helper.c >> +++ b/target/riscv/cpu_helper.c >> @@ -762,7 +762,7 @@ static int get_physical_address(CPURISCVState >> *env, hwaddr *physical, >> * (riscv_cpu_do_interrupt) is correct */ >> MemTxResult res; >> MemTxAttrs attrs = MEMTXATTRS_UNSPECIFIED; >> - int mode = mmu_idx & TB_FLAGS_PRIV_MMU_MASK; >> + int mode = env->priv; >> bool use_background = false; >> hwaddr ppn; >> RISCVCPU *cpu = env_archcpu(env); >> diff --git a/target/riscv/insn_trans/trans_privileged.c.inc >> b/target/riscv/insn_trans/trans_privileged.c.inc >> index 59501b2780..9305b18299 100644 >> --- a/target/riscv/insn_trans/trans_privileged.c.inc >> +++ b/target/riscv/insn_trans/trans_privileged.c.inc >> @@ -52,7 +52,7 @@ static bool trans_ebreak(DisasContext *ctx, >> arg_ebreak *a) >> * that no exception will be raised when fetching them. >> */ >> - if (semihosting_enabled(ctx->mem_idx < PRV_S) && >> + if (semihosting_enabled(ctx->priv < PRV_S) && >> (pre_addr & TARGET_PAGE_MASK) == (post_addr & >> TARGET_PAGE_MASK)) { >> pre = opcode_at(&ctx->base, pre_addr); >> ebreak = opcode_at(&ctx->base, ebreak_addr); >> diff --git a/target/riscv/insn_trans/trans_xthead.c.inc >> b/target/riscv/insn_trans/trans_xthead.c.inc >> index df504c3f2c..adfb53cb4c 100644 >> --- a/target/riscv/insn_trans/trans_xthead.c.inc >> +++ b/target/riscv/insn_trans/trans_xthead.c.inc >> @@ -265,12 +265,7 @@ static bool trans_th_tst(DisasContext *ctx, >> arg_th_tst *a) >> static inline int priv_level(DisasContext *ctx) >> { >> -#ifdef CONFIG_USER_ONLY >> - return PRV_U; >> -#else >> - /* Priv level is part of mem_idx. */ >> - return ctx->mem_idx & TB_FLAGS_PRIV_MMU_MASK; >> -#endif >> + return ctx->priv; >> } >> /* Test if priv level is M, S, or U (cannot fail). */ >> diff --git a/target/riscv/translate.c b/target/riscv/translate.c >> index 0ee8ee147d..e8880f9423 100644 >> --- a/target/riscv/translate.c >> +++ b/target/riscv/translate.c >> @@ -69,6 +69,7 @@ typedef struct DisasContext { >> uint32_t mstatus_hs_fs; >> uint32_t mstatus_hs_vs; >> uint32_t mem_idx; >> + uint32_t priv; >> /* Remember the rounding mode encoded in the previous fp >> instruction, >> which we have already installed into env->fp_status. Or -1 for >> no previous fp instruction. Note that we exit the TB when >> writing >> @@ -1162,8 +1163,10 @@ static void >> riscv_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs) >> } else { >> ctx->virt_enabled = false; >> } >> + ctx->priv = env->priv; > > This is not right. You should put env->priv into tb flags before you use > it in translation. > I see some other env usages in this function, when will env->priv and tb_flags.priv mismatch (assume we have recorded priv in tb_flags)? Thanks, Fei. > Zhiwei > >> #else >> ctx->virt_enabled = false; >> + ctx->priv = PRV_U; >> #endif >> ctx->misa_ext = env->misa_ext; >> ctx->frm = -1; /* unknown rounding mode */
On 3/23/2023 2:00 PM, Wu, Fei wrote: > On 3/23/2023 1:37 PM, LIU Zhiwei wrote: >> >> On 2023/3/23 10:44, Fei Wu wrote: >>> Currently it's assumed the 2 low bits of mmu_idx map to privilege mode, >>> this assumption won't last as we are about to add more mmu_idx. >> For patch set has more than 1 patch, usually add a cover letter. > > This is cover letter: > https://www.mail-archive.com/qemu-devel@nongnu.org/msg950849.html > > I added scripts/get_maintainer.pl to .git/config, it couldn't find out > the maintainers for the cover letter, so I added the mail lists to "To" > manually. > >>> >>> Signed-off-by: Fei Wu <fei2.wu@intel.com> >>> --- >>> target/riscv/cpu.h | 1 - >>> target/riscv/cpu_helper.c | 2 +- >>> target/riscv/insn_trans/trans_privileged.c.inc | 2 +- >>> target/riscv/insn_trans/trans_xthead.c.inc | 7 +------ >>> target/riscv/translate.c | 3 +++ >>> 5 files changed, 6 insertions(+), 9 deletions(-) >>> >>> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h >>> index 638e47c75a..66f7e3d1ba 100644 >>> --- a/target/riscv/cpu.h >>> +++ b/target/riscv/cpu.h >>> @@ -623,7 +623,6 @@ G_NORETURN void >>> riscv_raise_exception(CPURISCVState *env, >>> target_ulong riscv_cpu_get_fflags(CPURISCVState *env); >>> void riscv_cpu_set_fflags(CPURISCVState *env, target_ulong); >>> -#define TB_FLAGS_PRIV_MMU_MASK 3 >>> #define TB_FLAGS_PRIV_HYP_ACCESS_MASK (1 << 2) >>> #define TB_FLAGS_MSTATUS_FS MSTATUS_FS >>> #define TB_FLAGS_MSTATUS_VS MSTATUS_VS >>> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c >>> index f88c503cf4..76e1b0100e 100644 >>> --- a/target/riscv/cpu_helper.c >>> +++ b/target/riscv/cpu_helper.c >>> @@ -762,7 +762,7 @@ static int get_physical_address(CPURISCVState >>> *env, hwaddr *physical, >>> * (riscv_cpu_do_interrupt) is correct */ >>> MemTxResult res; >>> MemTxAttrs attrs = MEMTXATTRS_UNSPECIFIED; >>> - int mode = mmu_idx & TB_FLAGS_PRIV_MMU_MASK; >>> + int mode = env->priv; >>> bool use_background = false; >>> hwaddr ppn; >>> RISCVCPU *cpu = env_archcpu(env); >>> diff --git a/target/riscv/insn_trans/trans_privileged.c.inc >>> b/target/riscv/insn_trans/trans_privileged.c.inc >>> index 59501b2780..9305b18299 100644 >>> --- a/target/riscv/insn_trans/trans_privileged.c.inc >>> +++ b/target/riscv/insn_trans/trans_privileged.c.inc >>> @@ -52,7 +52,7 @@ static bool trans_ebreak(DisasContext *ctx, >>> arg_ebreak *a) >>> * that no exception will be raised when fetching them. >>> */ >>> - if (semihosting_enabled(ctx->mem_idx < PRV_S) && >>> + if (semihosting_enabled(ctx->priv < PRV_S) && >>> (pre_addr & TARGET_PAGE_MASK) == (post_addr & >>> TARGET_PAGE_MASK)) { >>> pre = opcode_at(&ctx->base, pre_addr); >>> ebreak = opcode_at(&ctx->base, ebreak_addr); >>> diff --git a/target/riscv/insn_trans/trans_xthead.c.inc >>> b/target/riscv/insn_trans/trans_xthead.c.inc >>> index df504c3f2c..adfb53cb4c 100644 >>> --- a/target/riscv/insn_trans/trans_xthead.c.inc >>> +++ b/target/riscv/insn_trans/trans_xthead.c.inc >>> @@ -265,12 +265,7 @@ static bool trans_th_tst(DisasContext *ctx, >>> arg_th_tst *a) >>> static inline int priv_level(DisasContext *ctx) >>> { >>> -#ifdef CONFIG_USER_ONLY >>> - return PRV_U; >>> -#else >>> - /* Priv level is part of mem_idx. */ >>> - return ctx->mem_idx & TB_FLAGS_PRIV_MMU_MASK; >>> -#endif >>> + return ctx->priv; >>> } >>> /* Test if priv level is M, S, or U (cannot fail). */ >>> diff --git a/target/riscv/translate.c b/target/riscv/translate.c >>> index 0ee8ee147d..e8880f9423 100644 >>> --- a/target/riscv/translate.c >>> +++ b/target/riscv/translate.c >>> @@ -69,6 +69,7 @@ typedef struct DisasContext { >>> uint32_t mstatus_hs_fs; >>> uint32_t mstatus_hs_vs; >>> uint32_t mem_idx; >>> + uint32_t priv; >>> /* Remember the rounding mode encoded in the previous fp >>> instruction, >>> which we have already installed into env->fp_status. Or -1 for >>> no previous fp instruction. Note that we exit the TB when >>> writing >>> @@ -1162,8 +1163,10 @@ static void >>> riscv_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs) >>> } else { >>> ctx->virt_enabled = false; >>> } >>> + ctx->priv = env->priv; >> >> This is not right. You should put env->priv into tb flags before you use >> it in translation. >> > I see some other env usages in this function, when will env->priv and > tb_flags.priv mismatch (assume we have recorded priv in tb_flags)? > I looks that they are from the same source. cpu_exec_loop cpu_get_tb_cpu_state(cpu->env_ptr, &pc, &cs_base, &flags); flags |= cpu_mmu_index(env, 0); // <-- generate flags from env tb = tb_gen_code(cpu, pc, cs_base, flags, cflags); tb->flags = flags; setjmp_gen_code(env, tb, pc, host_pc, &max_insns, &ti); gen_intermediate_code(env_cpu(env), tb, max_insns, pc, host_pc); DisasContext dc; translator_loop(cpu, tb, ..., &dc.base); ops->init_disas_context(db, cpu); // riscv_tr_ ctx->mem_idx = FIELD_EX32(tb_flags, TB_FLAGS, MEM_IDX); tcg_gen_code(tcg_ctx, tb, pc); Thanks, Fei. > Thanks, > Fei. > >> Zhiwei >> >>> #else >>> ctx->virt_enabled = false; >>> + ctx->priv = PRV_U; >>> #endif >>> ctx->misa_ext = env->misa_ext; >>> ctx->frm = -1; /* unknown rounding mode */ >
On 2023/3/23 14:00, Wu, Fei wrote: > On 3/23/2023 1:37 PM, LIU Zhiwei wrote: >> On 2023/3/23 10:44, Fei Wu wrote: >>> Currently it's assumed the 2 low bits of mmu_idx map to privilege mode, >>> this assumption won't last as we are about to add more mmu_idx. >> For patch set has more than 1 patch, usually add a cover letter. > This is cover letter: > https://www.mail-archive.com/qemu-devel@nongnu.org/msg950849.html > > I added scripts/get_maintainer.pl to .git/config, Interesting. > it couldn't find out > the maintainers for the cover letter, so I added the mail lists to "To" > manually. Maybe you should also cc to maintainers manually. I don't know the automatically way. > >>> Signed-off-by: Fei Wu <fei2.wu@intel.com> >>> --- >>> target/riscv/cpu.h | 1 - >>> target/riscv/cpu_helper.c | 2 +- >>> target/riscv/insn_trans/trans_privileged.c.inc | 2 +- >>> target/riscv/insn_trans/trans_xthead.c.inc | 7 +------ >>> target/riscv/translate.c | 3 +++ >>> 5 files changed, 6 insertions(+), 9 deletions(-) >>> >>> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h >>> index 638e47c75a..66f7e3d1ba 100644 >>> --- a/target/riscv/cpu.h >>> +++ b/target/riscv/cpu.h >>> @@ -623,7 +623,6 @@ G_NORETURN void >>> riscv_raise_exception(CPURISCVState *env, >>> target_ulong riscv_cpu_get_fflags(CPURISCVState *env); >>> void riscv_cpu_set_fflags(CPURISCVState *env, target_ulong); >>> -#define TB_FLAGS_PRIV_MMU_MASK 3 >>> #define TB_FLAGS_PRIV_HYP_ACCESS_MASK (1 << 2) >>> #define TB_FLAGS_MSTATUS_FS MSTATUS_FS >>> #define TB_FLAGS_MSTATUS_VS MSTATUS_VS >>> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c >>> index f88c503cf4..76e1b0100e 100644 >>> --- a/target/riscv/cpu_helper.c >>> +++ b/target/riscv/cpu_helper.c >>> @@ -762,7 +762,7 @@ static int get_physical_address(CPURISCVState >>> *env, hwaddr *physical, >>> * (riscv_cpu_do_interrupt) is correct */ >>> MemTxResult res; >>> MemTxAttrs attrs = MEMTXATTRS_UNSPECIFIED; >>> - int mode = mmu_idx & TB_FLAGS_PRIV_MMU_MASK; >>> + int mode = env->priv; >>> bool use_background = false; >>> hwaddr ppn; >>> RISCVCPU *cpu = env_archcpu(env); >>> diff --git a/target/riscv/insn_trans/trans_privileged.c.inc >>> b/target/riscv/insn_trans/trans_privileged.c.inc >>> index 59501b2780..9305b18299 100644 >>> --- a/target/riscv/insn_trans/trans_privileged.c.inc >>> +++ b/target/riscv/insn_trans/trans_privileged.c.inc >>> @@ -52,7 +52,7 @@ static bool trans_ebreak(DisasContext *ctx, >>> arg_ebreak *a) >>> * that no exception will be raised when fetching them. >>> */ >>> - if (semihosting_enabled(ctx->mem_idx < PRV_S) && >>> + if (semihosting_enabled(ctx->priv < PRV_S) && >>> (pre_addr & TARGET_PAGE_MASK) == (post_addr & >>> TARGET_PAGE_MASK)) { >>> pre = opcode_at(&ctx->base, pre_addr); >>> ebreak = opcode_at(&ctx->base, ebreak_addr); >>> diff --git a/target/riscv/insn_trans/trans_xthead.c.inc >>> b/target/riscv/insn_trans/trans_xthead.c.inc >>> index df504c3f2c..adfb53cb4c 100644 >>> --- a/target/riscv/insn_trans/trans_xthead.c.inc >>> +++ b/target/riscv/insn_trans/trans_xthead.c.inc >>> @@ -265,12 +265,7 @@ static bool trans_th_tst(DisasContext *ctx, >>> arg_th_tst *a) >>> static inline int priv_level(DisasContext *ctx) >>> { >>> -#ifdef CONFIG_USER_ONLY >>> - return PRV_U; >>> -#else >>> - /* Priv level is part of mem_idx. */ >>> - return ctx->mem_idx & TB_FLAGS_PRIV_MMU_MASK; >>> -#endif >>> + return ctx->priv; >>> } >>> /* Test if priv level is M, S, or U (cannot fail). */ >>> diff --git a/target/riscv/translate.c b/target/riscv/translate.c >>> index 0ee8ee147d..e8880f9423 100644 >>> --- a/target/riscv/translate.c >>> +++ b/target/riscv/translate.c >>> @@ -69,6 +69,7 @@ typedef struct DisasContext { >>> uint32_t mstatus_hs_fs; >>> uint32_t mstatus_hs_vs; >>> uint32_t mem_idx; >>> + uint32_t priv; >>> /* Remember the rounding mode encoded in the previous fp >>> instruction, >>> which we have already installed into env->fp_status. Or -1 for >>> no previous fp instruction. Note that we exit the TB when >>> writing >>> @@ -1162,8 +1163,10 @@ static void >>> riscv_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs) >>> } else { >>> ctx->virt_enabled = false; >>> } >>> + ctx->priv = env->priv; >> This is not right. You should put env->priv into tb flags before you use >> it in translation. >> > I see some other env usages in this function, Don't do it that way. It just be merged by accident. It will make review harder and probably be wrong. > when will env->priv and > tb_flags.priv mismatch (assume we have recorded priv in tb_flags)? We always record the env->priv in tb flags if we don't merge your second patch in this patch set. After your second patch, we will not record the env->priv into tb flags when SUM is 1. Thus we may execute a S-mode code when we actually in M-mode, which is forbidden by RISC-V. Zhiwei > > Thanks, > Fei. > >> Zhiwei >> >>> #else >>> ctx->virt_enabled = false; >>> + ctx->priv = PRV_U; >>> #endif >>> ctx->misa_ext = env->misa_ext; >>> ctx->frm = -1; /* unknown rounding mode */
On 3/23/2023 2:59 PM, LIU Zhiwei wrote: > > On 2023/3/23 14:00, Wu, Fei wrote: >> On 3/23/2023 1:37 PM, LIU Zhiwei wrote: >>> On 2023/3/23 10:44, Fei Wu wrote: >>>> Currently it's assumed the 2 low bits of mmu_idx map to privilege mode, >>>> this assumption won't last as we are about to add more mmu_idx. >>> For patch set has more than 1 patch, usually add a cover letter. >> This is cover letter: >> https://www.mail-archive.com/qemu-devel@nongnu.org/msg950849.html >> >> I added scripts/get_maintainer.pl to .git/config, > Interesting. >> it couldn't find out >> the maintainers for the cover letter, so I added the mail lists to "To" >> manually. > Maybe you should also cc to maintainers manually. I don't know the > automatically way. >> >>>> Signed-off-by: Fei Wu <fei2.wu@intel.com> >>>> --- >>>> target/riscv/cpu.h | 1 - >>>> target/riscv/cpu_helper.c | 2 +- >>>> target/riscv/insn_trans/trans_privileged.c.inc | 2 +- >>>> target/riscv/insn_trans/trans_xthead.c.inc | 7 +------ >>>> target/riscv/translate.c | 3 +++ >>>> 5 files changed, 6 insertions(+), 9 deletions(-) >>>> >>>> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h >>>> index 638e47c75a..66f7e3d1ba 100644 >>>> --- a/target/riscv/cpu.h >>>> +++ b/target/riscv/cpu.h >>>> @@ -623,7 +623,6 @@ G_NORETURN void >>>> riscv_raise_exception(CPURISCVState *env, >>>> target_ulong riscv_cpu_get_fflags(CPURISCVState *env); >>>> void riscv_cpu_set_fflags(CPURISCVState *env, target_ulong); >>>> -#define TB_FLAGS_PRIV_MMU_MASK 3 >>>> #define TB_FLAGS_PRIV_HYP_ACCESS_MASK (1 << 2) >>>> #define TB_FLAGS_MSTATUS_FS MSTATUS_FS >>>> #define TB_FLAGS_MSTATUS_VS MSTATUS_VS >>>> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c >>>> index f88c503cf4..76e1b0100e 100644 >>>> --- a/target/riscv/cpu_helper.c >>>> +++ b/target/riscv/cpu_helper.c >>>> @@ -762,7 +762,7 @@ static int get_physical_address(CPURISCVState >>>> *env, hwaddr *physical, >>>> * (riscv_cpu_do_interrupt) is correct */ >>>> MemTxResult res; >>>> MemTxAttrs attrs = MEMTXATTRS_UNSPECIFIED; >>>> - int mode = mmu_idx & TB_FLAGS_PRIV_MMU_MASK; >>>> + int mode = env->priv; >>>> bool use_background = false; >>>> hwaddr ppn; >>>> RISCVCPU *cpu = env_archcpu(env); >>>> diff --git a/target/riscv/insn_trans/trans_privileged.c.inc >>>> b/target/riscv/insn_trans/trans_privileged.c.inc >>>> index 59501b2780..9305b18299 100644 >>>> --- a/target/riscv/insn_trans/trans_privileged.c.inc >>>> +++ b/target/riscv/insn_trans/trans_privileged.c.inc >>>> @@ -52,7 +52,7 @@ static bool trans_ebreak(DisasContext *ctx, >>>> arg_ebreak *a) >>>> * that no exception will be raised when fetching them. >>>> */ >>>> - if (semihosting_enabled(ctx->mem_idx < PRV_S) && >>>> + if (semihosting_enabled(ctx->priv < PRV_S) && >>>> (pre_addr & TARGET_PAGE_MASK) == (post_addr & >>>> TARGET_PAGE_MASK)) { >>>> pre = opcode_at(&ctx->base, pre_addr); >>>> ebreak = opcode_at(&ctx->base, ebreak_addr); >>>> diff --git a/target/riscv/insn_trans/trans_xthead.c.inc >>>> b/target/riscv/insn_trans/trans_xthead.c.inc >>>> index df504c3f2c..adfb53cb4c 100644 >>>> --- a/target/riscv/insn_trans/trans_xthead.c.inc >>>> +++ b/target/riscv/insn_trans/trans_xthead.c.inc >>>> @@ -265,12 +265,7 @@ static bool trans_th_tst(DisasContext *ctx, >>>> arg_th_tst *a) >>>> static inline int priv_level(DisasContext *ctx) >>>> { >>>> -#ifdef CONFIG_USER_ONLY >>>> - return PRV_U; >>>> -#else >>>> - /* Priv level is part of mem_idx. */ >>>> - return ctx->mem_idx & TB_FLAGS_PRIV_MMU_MASK; >>>> -#endif >>>> + return ctx->priv; >>>> } >>>> /* Test if priv level is M, S, or U (cannot fail). */ >>>> diff --git a/target/riscv/translate.c b/target/riscv/translate.c >>>> index 0ee8ee147d..e8880f9423 100644 >>>> --- a/target/riscv/translate.c >>>> +++ b/target/riscv/translate.c >>>> @@ -69,6 +69,7 @@ typedef struct DisasContext { >>>> uint32_t mstatus_hs_fs; >>>> uint32_t mstatus_hs_vs; >>>> uint32_t mem_idx; >>>> + uint32_t priv; >>>> /* Remember the rounding mode encoded in the previous fp >>>> instruction, >>>> which we have already installed into env->fp_status. Or >>>> -1 for >>>> no previous fp instruction. Note that we exit the TB when >>>> writing >>>> @@ -1162,8 +1163,10 @@ static void >>>> riscv_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs) >>>> } else { >>>> ctx->virt_enabled = false; >>>> } >>>> + ctx->priv = env->priv; >>> This is not right. You should put env->priv into tb flags before you use >>> it in translation. >>> >> I see some other env usages in this function, > Don't do it that way. It just be merged by accident. It will make review > harder and probably be wrong. >> when will env->priv and >> tb_flags.priv mismatch (assume we have recorded priv in tb_flags)? > > We always record the env->priv in tb flags if we don't merge your second > patch in this patch set. > After your second patch, we will not record the env->priv into tb flags > when SUM is 1. Thus we may execute a S-mode code when we actually in > M-mode, which is forbidden by RISC-V. > Do you mean the case of calling tb_lookup(flags) to reuse TB? priv should be part of flags or it finds the wrong TB, SUM not? Thanks, Fei. > Zhiwei > >> >> Thanks, >> Fei. >> >>> Zhiwei >>> >>>> #else >>>> ctx->virt_enabled = false; >>>> + ctx->priv = PRV_U; >>>> #endif >>>> ctx->misa_ext = env->misa_ext; >>>> ctx->frm = -1; /* unknown rounding mode */
On 3/22/23 19:44, Fei Wu wrote: > Currently it's assumed the 2 low bits of mmu_idx map to privilege mode, > this assumption won't last as we are about to add more mmu_idx. > > Signed-off-by: Fei Wu <fei2.wu@intel.com> > --- > target/riscv/cpu.h | 1 - > target/riscv/cpu_helper.c | 2 +- > target/riscv/insn_trans/trans_privileged.c.inc | 2 +- > target/riscv/insn_trans/trans_xthead.c.inc | 7 +------ > target/riscv/translate.c | 3 +++ > 5 files changed, 6 insertions(+), 9 deletions(-) > > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h > index 638e47c75a..66f7e3d1ba 100644 > --- a/target/riscv/cpu.h > +++ b/target/riscv/cpu.h > @@ -623,7 +623,6 @@ G_NORETURN void riscv_raise_exception(CPURISCVState *env, > target_ulong riscv_cpu_get_fflags(CPURISCVState *env); > void riscv_cpu_set_fflags(CPURISCVState *env, target_ulong); > > -#define TB_FLAGS_PRIV_MMU_MASK 3 > #define TB_FLAGS_PRIV_HYP_ACCESS_MASK (1 << 2) > #define TB_FLAGS_MSTATUS_FS MSTATUS_FS > #define TB_FLAGS_MSTATUS_VS MSTATUS_VS > diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c > index f88c503cf4..76e1b0100e 100644 > --- a/target/riscv/cpu_helper.c > +++ b/target/riscv/cpu_helper.c > @@ -762,7 +762,7 @@ static int get_physical_address(CPURISCVState *env, hwaddr *physical, > * (riscv_cpu_do_interrupt) is correct */ > MemTxResult res; > MemTxAttrs attrs = MEMTXATTRS_UNSPECIFIED; > - int mode = mmu_idx & TB_FLAGS_PRIV_MMU_MASK; > + int mode = env->priv; This is incorrect. You must map back from mmu_idx to priv. Recall the semantics of MPRV. > diff --git a/target/riscv/insn_trans/trans_xthead.c.inc b/target/riscv/insn_trans/trans_xthead.c.inc > index df504c3f2c..adfb53cb4c 100644 > --- a/target/riscv/insn_trans/trans_xthead.c.inc > +++ b/target/riscv/insn_trans/trans_xthead.c.inc > @@ -265,12 +265,7 @@ static bool trans_th_tst(DisasContext *ctx, arg_th_tst *a) > > static inline int priv_level(DisasContext *ctx) > { > -#ifdef CONFIG_USER_ONLY > - return PRV_U; > -#else > - /* Priv level is part of mem_idx. */ > - return ctx->mem_idx & TB_FLAGS_PRIV_MMU_MASK; > -#endif > + return ctx->priv; > } I guess we aren't expecting optimization to remove dead system code? That would be the only reason to keep the ifdef. This function should be hoisted to translate.c, or simply replaced by the field access. > @@ -1162,8 +1163,10 @@ static void riscv_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs) > } else { > ctx->virt_enabled = false; > } > + ctx->priv = env->priv; Incorrect, as Zhiwei pointed out. I gave you the changes required to TB_FLAGS... r~
On 3/22/23 23:00, Wu, Fei wrote: >>> + ctx->priv = env->priv; >> >> This is not right. You should put env->priv into tb flags before you use >> it in translation. >> > I see some other env usages in this function, when will env->priv and > tb_flags.priv mismatch (assume we have recorded priv in tb_flags)? You are correct that they should match, because of tb_flags, but it is bad form to read from env, as that leads to errors. Since you *can* read the same data from tb_flags, you should. The read of misa_ext and misa_mxl_max are correct, because they are constant set at cpu init/realize. The read of vstart is incorrect. The TB_FLAGS field is VL_EQ_VLMAX, which includes a test for vstart == 0, but the smaller vstart == 0 test is not extractable from that. Thus the usage in e.g. vext_check_reduction is incorrect. One would require a new TB_FLAGS bit to encode vstart ==/!= 0 alone. r~
On 3/23/2023 11:53 PM, Richard Henderson wrote: > On 3/22/23 19:44, Fei Wu wrote: >> Currently it's assumed the 2 low bits of mmu_idx map to privilege mode, >> this assumption won't last as we are about to add more mmu_idx. >> >> Signed-off-by: Fei Wu <fei2.wu@intel.com> >> --- >> target/riscv/cpu.h | 1 - >> target/riscv/cpu_helper.c | 2 +- >> target/riscv/insn_trans/trans_privileged.c.inc | 2 +- >> target/riscv/insn_trans/trans_xthead.c.inc | 7 +------ >> target/riscv/translate.c | 3 +++ >> 5 files changed, 6 insertions(+), 9 deletions(-) >> >> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h >> index 638e47c75a..66f7e3d1ba 100644 >> --- a/target/riscv/cpu.h >> +++ b/target/riscv/cpu.h >> @@ -623,7 +623,6 @@ G_NORETURN void >> riscv_raise_exception(CPURISCVState *env, >> target_ulong riscv_cpu_get_fflags(CPURISCVState *env); >> void riscv_cpu_set_fflags(CPURISCVState *env, target_ulong); >> -#define TB_FLAGS_PRIV_MMU_MASK 3 >> #define TB_FLAGS_PRIV_HYP_ACCESS_MASK (1 << 2) >> #define TB_FLAGS_MSTATUS_FS MSTATUS_FS >> #define TB_FLAGS_MSTATUS_VS MSTATUS_VS >> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c >> index f88c503cf4..76e1b0100e 100644 >> --- a/target/riscv/cpu_helper.c >> +++ b/target/riscv/cpu_helper.c >> @@ -762,7 +762,7 @@ static int get_physical_address(CPURISCVState >> *env, hwaddr *physical, >> * (riscv_cpu_do_interrupt) is correct */ >> MemTxResult res; >> MemTxAttrs attrs = MEMTXATTRS_UNSPECIFIED; >> - int mode = mmu_idx & TB_FLAGS_PRIV_MMU_MASK; >> + int mode = env->priv; > > This is incorrect. You must map back from mmu_idx to priv. > Recall the semantics of MPRV. > The following code (~20 lines below) takes MPRV into consideration: if (riscv_cpu_two_stage_lookup(mmu_idx)) { mode = get_field(env->hstatus, HSTATUS_SPVP); } else if (mode == PRV_M && access_type != MMU_INST_FETCH) { if (get_field(env->mstatus, MSTATUS_MPRV)) { mode = get_field(env->mstatus, MSTATUS_MPP); } } I think it's the same logic as the original one. >> diff --git a/target/riscv/insn_trans/trans_xthead.c.inc >> b/target/riscv/insn_trans/trans_xthead.c.inc >> index df504c3f2c..adfb53cb4c 100644 >> --- a/target/riscv/insn_trans/trans_xthead.c.inc >> +++ b/target/riscv/insn_trans/trans_xthead.c.inc >> @@ -265,12 +265,7 @@ static bool trans_th_tst(DisasContext *ctx, >> arg_th_tst *a) >> static inline int priv_level(DisasContext *ctx) >> { >> -#ifdef CONFIG_USER_ONLY >> - return PRV_U; >> -#else >> - /* Priv level is part of mem_idx. */ >> - return ctx->mem_idx & TB_FLAGS_PRIV_MMU_MASK; >> -#endif >> + return ctx->priv; >> } > > I guess we aren't expecting optimization to remove dead system code? > That would be the only reason to keep the ifdef. > > This function should be hoisted to translate.c, or simply replaced by > the field access. > I only want to replace mem_idx to priv here, Zhiwei might decide how to adjust the code further. Thanks, Fei. >> @@ -1162,8 +1163,10 @@ static void >> riscv_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs) >> } else { >> ctx->virt_enabled = false; >> } >> + ctx->priv = env->priv; > > Incorrect, as Zhiwei pointed out. > I gave you the changes required to TB_FLAGS... > > > r~
On 3/24/2023 12:07 AM, Richard Henderson wrote: > On 3/22/23 23:00, Wu, Fei wrote: >>>> + ctx->priv = env->priv; >>> >>> This is not right. You should put env->priv into tb flags before you use >>> it in translation. >>> >> I see some other env usages in this function, when will env->priv and >> tb_flags.priv mismatch (assume we have recorded priv in tb_flags)? > > You are correct that they should match, because of tb_flags, but it is > bad form to read from env, as that leads to errors. Since you *can* > read the same data from tb_flags, you should. > I lack some background here, why should tb_flags be preferred if env has the same info? Then for reading from tb_flags, we need to add it to tb_flags first. Correct me if I'm wrong. The only strong reason we add some flag to tb_flags is that it causes codegen generate different code because of this flag change, and it looks like priv is not one of them, neither is mmu_idx, the generated code does use mmu_idx, but no different code generated for it. I think here we have some other reasons to include more, e.g. reference env can be error-prone in some way. So there are minimal flags must be in tb_flags, but we think it's better to add more? Thanks, Fei. > The read of misa_ext and misa_mxl_max are correct, because they are > constant set at cpu init/realize. > > The read of vstart is incorrect. The TB_FLAGS field is VL_EQ_VLMAX, > which includes a test for vstart == 0, but the smaller vstart == 0 test > is not extractable from that. Thus the usage in e.g. > vext_check_reduction is incorrect. One would require a new TB_FLAGS bit > to encode vstart ==/!= 0 alone. > > > r~
On 3/24/2023 9:02 AM, Wu, Fei wrote: > On 3/23/2023 11:53 PM, Richard Henderson wrote: >> On 3/22/23 19:44, Fei Wu wrote: >>> Currently it's assumed the 2 low bits of mmu_idx map to privilege mode, >>> this assumption won't last as we are about to add more mmu_idx. >>> >>> Signed-off-by: Fei Wu <fei2.wu@intel.com> >>> --- >>> target/riscv/cpu.h | 1 - >>> target/riscv/cpu_helper.c | 2 +- >>> target/riscv/insn_trans/trans_privileged.c.inc | 2 +- >>> target/riscv/insn_trans/trans_xthead.c.inc | 7 +------ >>> target/riscv/translate.c | 3 +++ >>> 5 files changed, 6 insertions(+), 9 deletions(-) >>> >>> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h >>> index 638e47c75a..66f7e3d1ba 100644 >>> --- a/target/riscv/cpu.h >>> +++ b/target/riscv/cpu.h >>> @@ -623,7 +623,6 @@ G_NORETURN void >>> riscv_raise_exception(CPURISCVState *env, >>> target_ulong riscv_cpu_get_fflags(CPURISCVState *env); >>> void riscv_cpu_set_fflags(CPURISCVState *env, target_ulong); >>> -#define TB_FLAGS_PRIV_MMU_MASK 3 >>> #define TB_FLAGS_PRIV_HYP_ACCESS_MASK (1 << 2) >>> #define TB_FLAGS_MSTATUS_FS MSTATUS_FS >>> #define TB_FLAGS_MSTATUS_VS MSTATUS_VS >>> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c >>> index f88c503cf4..76e1b0100e 100644 >>> --- a/target/riscv/cpu_helper.c >>> +++ b/target/riscv/cpu_helper.c >>> @@ -762,7 +762,7 @@ static int get_physical_address(CPURISCVState >>> *env, hwaddr *physical, >>> * (riscv_cpu_do_interrupt) is correct */ >>> MemTxResult res; >>> MemTxAttrs attrs = MEMTXATTRS_UNSPECIFIED; >>> - int mode = mmu_idx & TB_FLAGS_PRIV_MMU_MASK; >>> + int mode = env->priv; >> >> This is incorrect. You must map back from mmu_idx to priv. >> Recall the semantics of MPRV. >> > The following code (~20 lines below) takes MPRV into consideration: > > if (riscv_cpu_two_stage_lookup(mmu_idx)) { > mode = get_field(env->hstatus, HSTATUS_SPVP); > } else if (mode == PRV_M && access_type != MMU_INST_FETCH) { > if (get_field(env->mstatus, MSTATUS_MPRV)) { > mode = get_field(env->mstatus, MSTATUS_MPP); > } > } > > I think it's the same logic as the original one. > >>> diff --git a/target/riscv/insn_trans/trans_xthead.c.inc >>> b/target/riscv/insn_trans/trans_xthead.c.inc >>> index df504c3f2c..adfb53cb4c 100644 >>> --- a/target/riscv/insn_trans/trans_xthead.c.inc >>> +++ b/target/riscv/insn_trans/trans_xthead.c.inc >>> @@ -265,12 +265,7 @@ static bool trans_th_tst(DisasContext *ctx, >>> arg_th_tst *a) >>> static inline int priv_level(DisasContext *ctx) >>> { >>> -#ifdef CONFIG_USER_ONLY >>> - return PRV_U; >>> -#else >>> - /* Priv level is part of mem_idx. */ >>> - return ctx->mem_idx & TB_FLAGS_PRIV_MMU_MASK; >>> -#endif >>> + return ctx->priv; >>> } >> >> I guess we aren't expecting optimization to remove dead system code? >> That would be the only reason to keep the ifdef. >> >> This function should be hoisted to translate.c, or simply replaced by >> the field access. >> > I only want to replace mem_idx to priv here, Zhiwei might decide how to > adjust the code further. > I'm not sure if this is good English, sorry for that if it's not. I mean I want to keep this patch small. Thanks, Fei. > Thanks, > Fei. > >>> @@ -1162,8 +1163,10 @@ static void >>> riscv_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs) >>> } else { >>> ctx->virt_enabled = false; >>> } >>> + ctx->priv = env->priv; >> >> Incorrect, as Zhiwei pointed out. >> I gave you the changes required to TB_FLAGS... >> >> >> r~ >
On 3/23/23 18:20, Wu, Fei wrote: > I lack some background here, why should tb_flags be preferred if env has > the same info? Then for reading from tb_flags, we need to add it to > tb_flags first. We read from tb_flags in translate because that proves we've added the data to tb_flags for the TB hash. It avoids errors like the one for vstart. > Correct me if I'm wrong. The only strong reason we add some flag to > tb_flags is that it causes codegen generate different code because of > this flag change, and it looks like priv is not one of them, neither is > mmu_idx, the generated code does use mmu_idx, but no different code > generated for it. PRIV definitely affects the generated code: for a supervisor instruction, such as REQUIRE_PRIV_MS, we emit gen_exception_illegal() if PRIV == U. MMU_IDX definitely affects the generated code, because that immediate value makes its way into the memory offsets in the softmmu lookup sequence. Have a look at the output of -d op_opt,out_asm. > I think here we have some other reasons to include more, e.g. reference > env can be error-prone in some way. So there are minimal flags must be > in tb_flags, but we think it's better to add more? We add the ones required for efficiency of execution. We had not originally added PRIV, because the (original) few instructions in trans_privileged.c.inc all call helpers anyway, so it was easy enough to check PRIV in the helper. Since then more uses have grown. We *could* turn those into helper functions as well, but every other guest arch includes the privilege level in tb_flags, and it seems natural to do so. Only if you completely run out of bits would I consider working hard to eliminate that one. r~
On 3/24/2023 10:37 AM, Richard Henderson wrote: > On 3/23/23 18:20, Wu, Fei wrote: >> I lack some background here, why should tb_flags be preferred if env has >> the same info? Then for reading from tb_flags, we need to add it to >> tb_flags first. > > We read from tb_flags in translate because that proves we've added the > data to tb_flags for the TB hash. It avoids errors like the one for > vstart. > Got it. >> Correct me if I'm wrong. The only strong reason we add some flag to >> tb_flags is that it causes codegen generate different code because of >> this flag change, and it looks like priv is not one of them, neither is >> mmu_idx, the generated code does use mmu_idx, but no different code >> generated for it. > > PRIV definitely affects the generated code: for a supervisor > instruction, such as REQUIRE_PRIV_MS, we emit gen_exception_illegal() if > PRIV == U. > You are right, another one is just mentioned semihosting_enabled() in trans_ebreak. > MMU_IDX definitely affects the generated code, because that immediate > value makes its way into the memory offsets in the softmmu lookup > sequence. Have a look at the output of -d op_opt,out_asm. > Yes, I think you mean the fast path for softmmu lookup. >> I think here we have some other reasons to include more, e.g. reference >> env can be error-prone in some way. So there are minimal flags must be >> in tb_flags, but we think it's better to add more? > > We add the ones required for efficiency of execution. > > We had not originally added PRIV, because the (original) few > instructions in trans_privileged.c.inc all call helpers anyway, so it > was easy enough to check PRIV in the helper. > > Since then more uses have grown. We *could* turn those into helper > functions as well, but every other guest arch includes the privilege > level in tb_flags, and it seems natural to do so. Only if you > completely run out of bits would I consider working hard to eliminate > that one. > It makes sense. This is very informative, I appreciate it very much. Thanks, Fei. > > r~
On 3/24/2023 9:02 AM, Wu, Fei wrote: > On 3/23/2023 11:53 PM, Richard Henderson wrote: >> On 3/22/23 19:44, Fei Wu wrote: >>> Currently it's assumed the 2 low bits of mmu_idx map to privilege mode, >>> this assumption won't last as we are about to add more mmu_idx. >>> >>> Signed-off-by: Fei Wu <fei2.wu@intel.com> >>> --- >>> target/riscv/cpu.h | 1 - >>> target/riscv/cpu_helper.c | 2 +- >>> target/riscv/insn_trans/trans_privileged.c.inc | 2 +- >>> target/riscv/insn_trans/trans_xthead.c.inc | 7 +------ >>> target/riscv/translate.c | 3 +++ >>> 5 files changed, 6 insertions(+), 9 deletions(-) >>> >>> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h >>> index 638e47c75a..66f7e3d1ba 100644 >>> --- a/target/riscv/cpu.h >>> +++ b/target/riscv/cpu.h >>> @@ -623,7 +623,6 @@ G_NORETURN void >>> riscv_raise_exception(CPURISCVState *env, >>> target_ulong riscv_cpu_get_fflags(CPURISCVState *env); >>> void riscv_cpu_set_fflags(CPURISCVState *env, target_ulong); >>> -#define TB_FLAGS_PRIV_MMU_MASK 3 >>> #define TB_FLAGS_PRIV_HYP_ACCESS_MASK (1 << 2) >>> #define TB_FLAGS_MSTATUS_FS MSTATUS_FS >>> #define TB_FLAGS_MSTATUS_VS MSTATUS_VS >>> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c >>> index f88c503cf4..76e1b0100e 100644 >>> --- a/target/riscv/cpu_helper.c >>> +++ b/target/riscv/cpu_helper.c >>> @@ -762,7 +762,7 @@ static int get_physical_address(CPURISCVState >>> *env, hwaddr *physical, >>> * (riscv_cpu_do_interrupt) is correct */ >>> MemTxResult res; >>> MemTxAttrs attrs = MEMTXATTRS_UNSPECIFIED; >>> - int mode = mmu_idx & TB_FLAGS_PRIV_MMU_MASK; >>> + int mode = env->priv; >> >> This is incorrect. You must map back from mmu_idx to priv. >> Recall the semantics of MPRV. >> > The following code (~20 lines below) takes MPRV into consideration: > > if (riscv_cpu_two_stage_lookup(mmu_idx)) { > mode = get_field(env->hstatus, HSTATUS_SPVP); > } else if (mode == PRV_M && access_type != MMU_INST_FETCH) { > if (get_field(env->mstatus, MSTATUS_MPRV)) { > mode = get_field(env->mstatus, MSTATUS_MPP); > } > } > > I think it's the same logic as the original one. > this logic is good? I'm not going to map it back if it's okay. >>> diff --git a/target/riscv/insn_trans/trans_xthead.c.inc >>> b/target/riscv/insn_trans/trans_xthead.c.inc >>> index df504c3f2c..adfb53cb4c 100644 >>> --- a/target/riscv/insn_trans/trans_xthead.c.inc >>> +++ b/target/riscv/insn_trans/trans_xthead.c.inc >>> @@ -265,12 +265,7 @@ static bool trans_th_tst(DisasContext *ctx, >>> arg_th_tst *a) >>> static inline int priv_level(DisasContext *ctx) >>> { >>> -#ifdef CONFIG_USER_ONLY >>> - return PRV_U; >>> -#else >>> - /* Priv level is part of mem_idx. */ >>> - return ctx->mem_idx & TB_FLAGS_PRIV_MMU_MASK; >>> -#endif >>> + return ctx->priv; >>> } >> >> I guess we aren't expecting optimization to remove dead system code? >> That would be the only reason to keep the ifdef. >> >> This function should be hoisted to translate.c, or simply replaced by >> the field access. >> > I only want to replace mem_idx to priv here, Zhiwei might decide how to > adjust the code further. > I will fix this. > Thanks, > Fei. > >>> @@ -1162,8 +1163,10 @@ static void >>> riscv_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs) >>> } else { >>> ctx->virt_enabled = false; >>> } >>> + ctx->priv = env->priv; >> >> Incorrect, as Zhiwei pointed out. >> I gave you the changes required to TB_FLAGS... >> >> Could you take a look at v5 patches? I added an extra PRIV into TB_FLAGS instead of the SUM flag, waste a bit in TB_FLAGS for a little clarity. I will switch it to PRIV + SUM if it's preferred. And if there is anything else I need to fix please let me do, I will fix it all in v6. Thanks, Fei. >> r~ >
diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h index 638e47c75a..66f7e3d1ba 100644 --- a/target/riscv/cpu.h +++ b/target/riscv/cpu.h @@ -623,7 +623,6 @@ G_NORETURN void riscv_raise_exception(CPURISCVState *env, target_ulong riscv_cpu_get_fflags(CPURISCVState *env); void riscv_cpu_set_fflags(CPURISCVState *env, target_ulong); -#define TB_FLAGS_PRIV_MMU_MASK 3 #define TB_FLAGS_PRIV_HYP_ACCESS_MASK (1 << 2) #define TB_FLAGS_MSTATUS_FS MSTATUS_FS #define TB_FLAGS_MSTATUS_VS MSTATUS_VS diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c index f88c503cf4..76e1b0100e 100644 --- a/target/riscv/cpu_helper.c +++ b/target/riscv/cpu_helper.c @@ -762,7 +762,7 @@ static int get_physical_address(CPURISCVState *env, hwaddr *physical, * (riscv_cpu_do_interrupt) is correct */ MemTxResult res; MemTxAttrs attrs = MEMTXATTRS_UNSPECIFIED; - int mode = mmu_idx & TB_FLAGS_PRIV_MMU_MASK; + int mode = env->priv; bool use_background = false; hwaddr ppn; RISCVCPU *cpu = env_archcpu(env); diff --git a/target/riscv/insn_trans/trans_privileged.c.inc b/target/riscv/insn_trans/trans_privileged.c.inc index 59501b2780..9305b18299 100644 --- a/target/riscv/insn_trans/trans_privileged.c.inc +++ b/target/riscv/insn_trans/trans_privileged.c.inc @@ -52,7 +52,7 @@ static bool trans_ebreak(DisasContext *ctx, arg_ebreak *a) * that no exception will be raised when fetching them. */ - if (semihosting_enabled(ctx->mem_idx < PRV_S) && + if (semihosting_enabled(ctx->priv < PRV_S) && (pre_addr & TARGET_PAGE_MASK) == (post_addr & TARGET_PAGE_MASK)) { pre = opcode_at(&ctx->base, pre_addr); ebreak = opcode_at(&ctx->base, ebreak_addr); diff --git a/target/riscv/insn_trans/trans_xthead.c.inc b/target/riscv/insn_trans/trans_xthead.c.inc index df504c3f2c..adfb53cb4c 100644 --- a/target/riscv/insn_trans/trans_xthead.c.inc +++ b/target/riscv/insn_trans/trans_xthead.c.inc @@ -265,12 +265,7 @@ static bool trans_th_tst(DisasContext *ctx, arg_th_tst *a) static inline int priv_level(DisasContext *ctx) { -#ifdef CONFIG_USER_ONLY - return PRV_U; -#else - /* Priv level is part of mem_idx. */ - return ctx->mem_idx & TB_FLAGS_PRIV_MMU_MASK; -#endif + return ctx->priv; } /* Test if priv level is M, S, or U (cannot fail). */ diff --git a/target/riscv/translate.c b/target/riscv/translate.c index 0ee8ee147d..e8880f9423 100644 --- a/target/riscv/translate.c +++ b/target/riscv/translate.c @@ -69,6 +69,7 @@ typedef struct DisasContext { uint32_t mstatus_hs_fs; uint32_t mstatus_hs_vs; uint32_t mem_idx; + uint32_t priv; /* Remember the rounding mode encoded in the previous fp instruction, which we have already installed into env->fp_status. Or -1 for no previous fp instruction. Note that we exit the TB when writing @@ -1162,8 +1163,10 @@ static void riscv_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs) } else { ctx->virt_enabled = false; } + ctx->priv = env->priv; #else ctx->virt_enabled = false; + ctx->priv = PRV_U; #endif ctx->misa_ext = env->misa_ext; ctx->frm = -1; /* unknown rounding mode */
Currently it's assumed the 2 low bits of mmu_idx map to privilege mode, this assumption won't last as we are about to add more mmu_idx. Signed-off-by: Fei Wu <fei2.wu@intel.com> --- target/riscv/cpu.h | 1 - target/riscv/cpu_helper.c | 2 +- target/riscv/insn_trans/trans_privileged.c.inc | 2 +- target/riscv/insn_trans/trans_xthead.c.inc | 7 +------ target/riscv/translate.c | 3 +++ 5 files changed, 6 insertions(+), 9 deletions(-)