[PATCH v4 0/2] target/riscv: reduce MSTATUS_SUM overhead

[PATCH v4 0/2] target/riscv: reduce MSTATUS_SUM overhead

[Fei Wu]

v3 -> v4:
* seperate priv from mmu_idx
* use index 2 for S+SUM mmu_idx
* no tlb_flush for MPRV / MPP changes

Fei Wu (2):
  target/riscv: separate priv from mmu_idx
  target/riscv: reduce overhead of MSTATUS_SUM change

 target/riscv/cpu.h                            |  2 --
 target/riscv/cpu_helper.c                     | 19 ++++++++++++++++---
 target/riscv/csr.c                            |  3 +--
 .../riscv/insn_trans/trans_privileged.c.inc   |  2 +-
 target/riscv/insn_trans/trans_rvh.c.inc       |  4 ++--
 target/riscv/insn_trans/trans_xthead.c.inc    |  7 +------
 target/riscv/internals.h                      | 14 ++++++++++++++
 target/riscv/op_helper.c                      |  5 +++--
 target/riscv/translate.c                      |  3 +++
 9 files changed, 41 insertions(+), 18 deletions(-)

-- 
2.25.1

[PATCH v4 1/2] target/riscv: separate priv from mmu_idx

[Fei Wu]

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;
     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 */
-- 
2.25.1

Re: [PATCH v4 1/2] target/riscv: separate priv from mmu_idx

[LIU Zhiwei]

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 */

Re: [PATCH v4 1/2] target/riscv: separate priv from mmu_idx

[Wu, Fei]

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 */

Re: [PATCH v4 1/2] target/riscv: separate priv from mmu_idx

[Wu, Fei]

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 */
> 

Re: [PATCH v4 1/2] target/riscv: separate priv from mmu_idx

[LIU Zhiwei]

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 */

Re: [PATCH v4 1/2] target/riscv: separate priv from mmu_idx

[Wu, Fei]

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 */

Re: [PATCH v4 1/2] target/riscv: separate priv from mmu_idx

[Richard Henderson]

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~

Re: [PATCH v4 1/2] target/riscv: separate priv from mmu_idx

[Wu, Fei]

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~

Re: [PATCH v4 1/2] target/riscv: separate priv from mmu_idx

[Richard Henderson]

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~

Re: [PATCH v4 1/2] target/riscv: separate priv from mmu_idx

[Wu, Fei]

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~

Re: [PATCH v4 1/2] target/riscv: separate priv from mmu_idx

[Richard Henderson]

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~

Re: [PATCH v4 1/2] target/riscv: separate priv from mmu_idx

[Wu, Fei]

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~

Re: [PATCH v4 1/2] target/riscv: separate priv from mmu_idx

[Wu, Fei]

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~
> 

Re: [PATCH v4 1/2] target/riscv: separate priv from mmu_idx

[Wu, Fei]

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~
> 

[PATCH v4 2/2] target/riscv: reduce overhead of MSTATUS_SUM change

[Fei Wu]

Kernel needs to access user mode memory e.g. during syscalls, the window
is usually opened up for a very limited time through MSTATUS.SUM, the
overhead is too much if tlb_flush() gets called for every SUM change.

This patch creates a separate MMU index for S+SUM, so that it's not
necessary to flush tlb anymore when SUM changes. This is similar to how
ARM handles Privileged Access Never (PAN).

Result of 'pipe 10' from unixbench boosts from 223656 to 1705006. Many
other syscalls benefit a lot from this too.

Signed-off-by: Fei Wu <fei2.wu@intel.com>
---
 target/riscv/cpu.h                      |  1 -
 target/riscv/cpu_helper.c               | 17 +++++++++++++++--
 target/riscv/csr.c                      |  3 +--
 target/riscv/insn_trans/trans_rvh.c.inc |  4 ++--
 target/riscv/internals.h                | 14 ++++++++++++++
 target/riscv/op_helper.c                |  5 +++--
 6 files changed, 35 insertions(+), 9 deletions(-)

diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index 66f7e3d1ba..d65eeb3c85 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_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 76e1b0100e..bbc612badf 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -21,6 +21,7 @@
 #include "qemu/log.h"
 #include "qemu/main-loop.h"
 #include "cpu.h"
+#include "internals.h"
 #include "pmu.h"
 #include "exec/exec-all.h"
 #include "instmap.h"
@@ -36,7 +37,19 @@ int riscv_cpu_mmu_index(CPURISCVState *env, bool ifetch)
 #ifdef CONFIG_USER_ONLY
     return 0;
 #else
-    return env->priv;
+    if (ifetch) {
+        return env->priv;
+    }
+
+    /* All priv -> mmu_idx mapping are here */
+    int mode = env->priv;
+    if (mode == PRV_M && get_field(env->mstatus, MSTATUS_MPRV)) {
+        mode = get_field(env->mstatus, MSTATUS_MPP);
+    }
+    if (mode == PRV_S && get_field(env->mstatus, MSTATUS_SUM)) {
+        return MMUIdx_S_SUM;
+    }
+    return mode;
 #endif
 }
 
@@ -596,7 +609,7 @@ void riscv_cpu_set_virt_enabled(CPURISCVState *env, bool enable)
 
 bool riscv_cpu_two_stage_lookup(int mmu_idx)
 {
-    return mmu_idx & TB_FLAGS_PRIV_HYP_ACCESS_MASK;
+    return mmu_idx & MMU_HYP_ACCESS_BIT;
 }
 
 int riscv_cpu_claim_interrupts(RISCVCPU *cpu, uint64_t interrupts)
diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index d522efc0b6..f74e40e66d 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -1246,8 +1246,7 @@ static RISCVException write_mstatus(CPURISCVState *env, int csrno,
     RISCVMXL xl = riscv_cpu_mxl(env);
 
     /* flush tlb on mstatus fields that affect VM */
-    if ((val ^ mstatus) & (MSTATUS_MXR | MSTATUS_MPP | MSTATUS_MPV |
-            MSTATUS_MPRV | MSTATUS_SUM)) {
+    if ((val ^ mstatus) & (MSTATUS_MXR | MSTATUS_MPV)) {
         tlb_flush(env_cpu(env));
     }
     mask = MSTATUS_SIE | MSTATUS_SPIE | MSTATUS_MIE | MSTATUS_MPIE |
diff --git a/target/riscv/insn_trans/trans_rvh.c.inc b/target/riscv/insn_trans/trans_rvh.c.inc
index 9248b48c36..15842f4282 100644
--- a/target/riscv/insn_trans/trans_rvh.c.inc
+++ b/target/riscv/insn_trans/trans_rvh.c.inc
@@ -40,7 +40,7 @@ static bool do_hlv(DisasContext *ctx, arg_r2 *a, MemOp mop)
     if (check_access(ctx)) {
         TCGv dest = dest_gpr(ctx, a->rd);
         TCGv addr = get_gpr(ctx, a->rs1, EXT_NONE);
-        int mem_idx = ctx->mem_idx | TB_FLAGS_PRIV_HYP_ACCESS_MASK;
+        int mem_idx = ctx->mem_idx | MMU_HYP_ACCESS_BIT;
         tcg_gen_qemu_ld_tl(dest, addr, mem_idx, mop);
         gen_set_gpr(ctx, a->rd, dest);
     }
@@ -87,7 +87,7 @@ static bool do_hsv(DisasContext *ctx, arg_r2_s *a, MemOp mop)
     if (check_access(ctx)) {
         TCGv addr = get_gpr(ctx, a->rs1, EXT_NONE);
         TCGv data = get_gpr(ctx, a->rs2, EXT_NONE);
-        int mem_idx = ctx->mem_idx | TB_FLAGS_PRIV_HYP_ACCESS_MASK;
+        int mem_idx = ctx->mem_idx | MMU_HYP_ACCESS_BIT;
         tcg_gen_qemu_st_tl(data, addr, mem_idx, mop);
     }
     return true;
diff --git a/target/riscv/internals.h b/target/riscv/internals.h
index 5620fbffb6..b55152a7dc 100644
--- a/target/riscv/internals.h
+++ b/target/riscv/internals.h
@@ -21,6 +21,20 @@
 
 #include "hw/registerfields.h"
 
+/*
+ * The current MMU Modes are:
+ *  - U                 0b000
+ *  - S                 0b001
+ *  - S+SUM             0b010
+ *  - M                 0b011
+ *  - HLV/HLVX/HSV adds 0b100
+ */
+#define MMUIdx_U            0
+#define MMUIdx_S            1
+#define MMUIdx_S_SUM        2
+#define MMUIdx_M            3
+#define MMU_HYP_ACCESS_BIT  (1 << 2)
+
 /* share data between vector helpers and decode code */
 FIELD(VDATA, VM, 0, 1)
 FIELD(VDATA, LMUL, 1, 3)
diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c
index 84ee018f7d..962a061228 100644
--- a/target/riscv/op_helper.c
+++ b/target/riscv/op_helper.c
@@ -20,6 +20,7 @@
 
 #include "qemu/osdep.h"
 #include "cpu.h"
+#include "internals.h"
 #include "qemu/main-loop.h"
 #include "exec/exec-all.h"
 #include "exec/helper-proto.h"
@@ -428,14 +429,14 @@ void helper_hyp_gvma_tlb_flush(CPURISCVState *env)
 
 target_ulong helper_hyp_hlvx_hu(CPURISCVState *env, target_ulong address)
 {
-    int mmu_idx = cpu_mmu_index(env, true) | TB_FLAGS_PRIV_HYP_ACCESS_MASK;
+    int mmu_idx = cpu_mmu_index(env, true) | MMU_HYP_ACCESS_BIT;
 
     return cpu_lduw_mmuidx_ra(env, address, mmu_idx, GETPC());
 }
 
 target_ulong helper_hyp_hlvx_wu(CPURISCVState *env, target_ulong address)
 {
-    int mmu_idx = cpu_mmu_index(env, true) | TB_FLAGS_PRIV_HYP_ACCESS_MASK;
+    int mmu_idx = cpu_mmu_index(env, true) | MMU_HYP_ACCESS_BIT;
 
     return cpu_ldl_mmuidx_ra(env, address, mmu_idx, GETPC());
 }
-- 
2.25.1