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

[PATCH v3] 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-param.h  |  2 +-
 target/riscv/cpu.h        |  2 +-
 target/riscv/cpu_bits.h   |  1 +
 target/riscv/cpu_helper.c | 11 +++++++++++
 target/riscv/csr.c        |  2 +-
 5 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/target/riscv/cpu-param.h b/target/riscv/cpu-param.h
index ebaf26d26d..9e21b943f9 100644
--- a/target/riscv/cpu-param.h
+++ b/target/riscv/cpu-param.h
@@ -27,6 +27,6 @@
  *  - S mode HLV/HLVX/HSV 0b101
  *  - M mode HLV/HLVX/HSV 0b111
  */
-#define NB_MMU_MODES 8
+#define NB_MMU_MODES 16
 
 #endif
diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index 638e47c75a..ac8bee11a7 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -624,7 +624,7 @@ 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_PRIV_HYP_ACCESS_MASK   (1 << 3)
 #define TB_FLAGS_MSTATUS_FS MSTATUS_FS
 #define TB_FLAGS_MSTATUS_VS MSTATUS_VS
 
diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
index fca7ef0cef..dd9e62b6e4 100644
--- a/target/riscv/cpu_bits.h
+++ b/target/riscv/cpu_bits.h
@@ -606,6 +606,7 @@ typedef enum {
 #define PRV_S 1
 #define PRV_H 2 /* Reserved */
 #define PRV_M 3
+#define MMUIdx_S_SUM 5
 
 /* Virtulisation Register Fields */
 #define VIRT_ONOFF          1
diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index f88c503cf4..e52e9765d0 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -36,6 +36,17 @@ int riscv_cpu_mmu_index(CPURISCVState *env, bool ifetch)
 #ifdef CONFIG_USER_ONLY
     return 0;
 #else
+    if (ifetch) {
+        return env->priv;
+    }
+
+    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 env->priv;
 #endif
 }
diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index ab566639e5..eacc40e912 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -1246,7 +1246,7 @@ static RISCVException write_mstatus(CPURISCVState *env, int csrno,
 
     /* flush tlb on mstatus fields that affect VM */
     if ((val ^ mstatus) & (MSTATUS_MXR | MSTATUS_MPP | MSTATUS_MPV |
-            MSTATUS_MPRV | MSTATUS_SUM)) {
+            MSTATUS_MPRV)) {
         tlb_flush(env_cpu(env));
     }
     mask = MSTATUS_SIE | MSTATUS_SPIE | MSTATUS_MIE | MSTATUS_MPIE |
-- 
2.25.1

Re: [PATCH v3] target/riscv: reduce overhead of MSTATUS_SUM change

[liweiwei]

On 2023/3/22 20:12, Fei Wu wrote:
> 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-param.h  |  2 +-
>   target/riscv/cpu.h        |  2 +-
>   target/riscv/cpu_bits.h   |  1 +
>   target/riscv/cpu_helper.c | 11 +++++++++++
>   target/riscv/csr.c        |  2 +-
>   5 files changed, 15 insertions(+), 3 deletions(-)
>
> diff --git a/target/riscv/cpu-param.h b/target/riscv/cpu-param.h
> index ebaf26d26d..9e21b943f9 100644
> --- a/target/riscv/cpu-param.h
> +++ b/target/riscv/cpu-param.h
> @@ -27,6 +27,6 @@
>    *  - S mode HLV/HLVX/HSV 0b101
>    *  - M mode HLV/HLVX/HSV 0b111
>    */
> -#define NB_MMU_MODES 8
> +#define NB_MMU_MODES 16
>   
>   #endif
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 638e47c75a..ac8bee11a7 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -624,7 +624,7 @@ 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_PRIV_HYP_ACCESS_MASK   (1 << 3)
>   #define TB_FLAGS_MSTATUS_FS MSTATUS_FS
>   #define TB_FLAGS_MSTATUS_VS MSTATUS_VS
>   
> diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
> index fca7ef0cef..dd9e62b6e4 100644
> --- a/target/riscv/cpu_bits.h
> +++ b/target/riscv/cpu_bits.h
> @@ -606,6 +606,7 @@ typedef enum {
>   #define PRV_S 1
>   #define PRV_H 2 /* Reserved */
>   #define PRV_M 3
> +#define MMUIdx_S_SUM 5
>   
>   /* Virtulisation Register Fields */
>   #define VIRT_ONOFF          1
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index f88c503cf4..e52e9765d0 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -36,6 +36,17 @@ int riscv_cpu_mmu_index(CPURISCVState *env, bool ifetch)
>   #ifdef CONFIG_USER_ONLY
>       return 0;
>   #else
> +    if (ifetch) {
> +        return env->priv;
> +    }
> +
> +    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 env->priv;

If we return mode here, whether tlb needn't flush for changes to 
MSTATUS_MPRV and MSTATUS_MPP?

Regards,

Weiwei Li

>   #endif
>   }
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index ab566639e5..eacc40e912 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -1246,7 +1246,7 @@ static RISCVException write_mstatus(CPURISCVState *env, int csrno,
>   
>       /* flush tlb on mstatus fields that affect VM */
>       if ((val ^ mstatus) & (MSTATUS_MXR | MSTATUS_MPP | MSTATUS_MPV |
> -            MSTATUS_MPRV | MSTATUS_SUM)) {
> +            MSTATUS_MPRV)) {
>           tlb_flush(env_cpu(env));
>       }
>       mask = MSTATUS_SIE | MSTATUS_SPIE | MSTATUS_MIE | MSTATUS_MPIE |

Re: [PATCH v3] target/riscv: reduce overhead of MSTATUS_SUM change

[Wu, Fei]

On 3/22/2023 8:37 PM, liweiwei wrote:
> 
> On 2023/3/22 20:12, Fei Wu wrote:
>> 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-param.h  |  2 +-
>>   target/riscv/cpu.h        |  2 +-
>>   target/riscv/cpu_bits.h   |  1 +
>>   target/riscv/cpu_helper.c | 11 +++++++++++
>>   target/riscv/csr.c        |  2 +-
>>   5 files changed, 15 insertions(+), 3 deletions(-)
>>
>> diff --git a/target/riscv/cpu-param.h b/target/riscv/cpu-param.h
>> index ebaf26d26d..9e21b943f9 100644
>> --- a/target/riscv/cpu-param.h
>> +++ b/target/riscv/cpu-param.h
>> @@ -27,6 +27,6 @@
>>    *  - S mode HLV/HLVX/HSV 0b101
>>    *  - M mode HLV/HLVX/HSV 0b111
>>    */
>> -#define NB_MMU_MODES 8
>> +#define NB_MMU_MODES 16
>>     #endif
>> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
>> index 638e47c75a..ac8bee11a7 100644
>> --- a/target/riscv/cpu.h
>> +++ b/target/riscv/cpu.h
>> @@ -624,7 +624,7 @@ 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_PRIV_HYP_ACCESS_MASK   (1 << 3)
>>   #define TB_FLAGS_MSTATUS_FS MSTATUS_FS
>>   #define TB_FLAGS_MSTATUS_VS MSTATUS_VS
>>   diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
>> index fca7ef0cef..dd9e62b6e4 100644
>> --- a/target/riscv/cpu_bits.h
>> +++ b/target/riscv/cpu_bits.h
>> @@ -606,6 +606,7 @@ typedef enum {
>>   #define PRV_S 1
>>   #define PRV_H 2 /* Reserved */
>>   #define PRV_M 3
>> +#define MMUIdx_S_SUM 5
>>     /* Virtulisation Register Fields */
>>   #define VIRT_ONOFF          1
>> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
>> index f88c503cf4..e52e9765d0 100644
>> --- a/target/riscv/cpu_helper.c
>> +++ b/target/riscv/cpu_helper.c
>> @@ -36,6 +36,17 @@ int riscv_cpu_mmu_index(CPURISCVState *env, bool
>> ifetch)
>>   #ifdef CONFIG_USER_ONLY
>>       return 0;
>>   #else
>> +    if (ifetch) {
>> +        return env->priv;
>> +    }
>> +
>> +    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 env->priv;
> 
> If we return mode here, whether tlb needn't flush for changes to
> MSTATUS_MPRV and MSTATUS_MPP?
> 
Good point. Besides performance improvement, it sounds more reasonable
to return the effective privilege mode instead of the plain priv, why it
should use the M-mode tlb index when it behaves as S-mode.

Thanks,
Fei.

> Regards,
> 
> Weiwei Li
> 
>>   #endif
>>   }
>> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
>> index ab566639e5..eacc40e912 100644
>> --- a/target/riscv/csr.c
>> +++ b/target/riscv/csr.c
>> @@ -1246,7 +1246,7 @@ static RISCVException
>> write_mstatus(CPURISCVState *env, int csrno,
>>         /* flush tlb on mstatus fields that affect VM */
>>       if ((val ^ mstatus) & (MSTATUS_MXR | MSTATUS_MPP | MSTATUS_MPV |
>> -            MSTATUS_MPRV | MSTATUS_SUM)) {
>> +            MSTATUS_MPRV)) {
>>           tlb_flush(env_cpu(env));
>>       }
>>       mask = MSTATUS_SIE | MSTATUS_SPIE | MSTATUS_MIE | MSTATUS_MPIE |
> 

Re: [PATCH v3] target/riscv: reduce overhead of MSTATUS_SUM change

[Richard Henderson]

On 3/22/23 05:12, Fei Wu wrote:
> 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-param.h  |  2 +-
>   target/riscv/cpu.h        |  2 +-
>   target/riscv/cpu_bits.h   |  1 +
>   target/riscv/cpu_helper.c | 11 +++++++++++
>   target/riscv/csr.c        |  2 +-
>   5 files changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/target/riscv/cpu-param.h b/target/riscv/cpu-param.h
> index ebaf26d26d..9e21b943f9 100644
> --- a/target/riscv/cpu-param.h
> +++ b/target/riscv/cpu-param.h
> @@ -27,6 +27,6 @@
>    *  - S mode HLV/HLVX/HSV 0b101
>    *  - M mode HLV/HLVX/HSV 0b111
>    */
> -#define NB_MMU_MODES 8
> +#define NB_MMU_MODES 16

This line no longer exists on master.
The comment above should be updated, and perhaps moved.

>   #define TB_FLAGS_PRIV_MMU_MASK                3
> -#define TB_FLAGS_PRIV_HYP_ACCESS_MASK   (1 << 2)
> +#define TB_FLAGS_PRIV_HYP_ACCESS_MASK   (1 << 3)

You can't do this, as you're now overlapping

FIELD(TB_FLAGS, LMUL, 3, 3)

You'd need to shift all other fields up to do this.
There is room, to be sure.

Or you could reuse MMU mode number 2.  For that you'd need to separate 
DisasContext.mem_idx from priv.  Which should probably be done anyway, because tests such as

insn_trans/trans_privileged.c.inc:    if (semihosting_enabled(ctx->mem_idx < PRV_S) &&

are already borderline wrong.

I suggest

- #define TB_FLAGS_PRIV_MMU_MASK                3
- #define TB_FLAGS_PRIV_HYP_ACCESS_MASK   (1 << 2)

HYP_ACCESS_MASK never needed to be part of TB_FLAGS; it is only set directly by the hyp 
access instruction translation.  Drop the PRIV mask and represent that directly:

- FIELD(TB_FLAGS, MEM_IDX, 0, 3)
+ FIELD(TB_FLAGS, PRIV, 0, 2)
+ FIELD(TB_FLAGS, SUM, 2, 1)

Let SUM occupy the released bit.

In internals.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)


In riscv_tr_init_disas_context:

     ctx->priv = FIELD_EX32(tb_flags, TB_FLAGS, PRIV);
     ctx->mmu_idx = ctx->priv;
     if (ctx->mmu_idx == PRV_S && FIELD_EX32(tb_flags, TB_FLAGS, SUM)) {
         ctx->mmu_idx = MMUIdx_S_SUM;
     }

and similarly in riscv_cpu_mmu_index.

Fix all uses of ctx->mmu_idx that are not specifically for memory operations.


r~

Re: [PATCH v3] target/riscv: reduce overhead of MSTATUS_SUM change

[Wu, Fei]

On 3/22/2023 9:19 PM, Richard Henderson wrote:
> On 3/22/23 05:12, Fei Wu wrote:
>> 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-param.h  |  2 +-
>>   target/riscv/cpu.h        |  2 +-
>>   target/riscv/cpu_bits.h   |  1 +
>>   target/riscv/cpu_helper.c | 11 +++++++++++
>>   target/riscv/csr.c        |  2 +-
>>   5 files changed, 15 insertions(+), 3 deletions(-)
>>
>> diff --git a/target/riscv/cpu-param.h b/target/riscv/cpu-param.h
>> index ebaf26d26d..9e21b943f9 100644
>> --- a/target/riscv/cpu-param.h
>> +++ b/target/riscv/cpu-param.h
>> @@ -27,6 +27,6 @@
>>    *  - S mode HLV/HLVX/HSV 0b101
>>    *  - M mode HLV/HLVX/HSV 0b111
>>    */
>> -#define NB_MMU_MODES 8
>> +#define NB_MMU_MODES 16
> 
> This line no longer exists on master.
> The comment above should be updated, and perhaps moved.
> 
>>   #define TB_FLAGS_PRIV_MMU_MASK                3
>> -#define TB_FLAGS_PRIV_HYP_ACCESS_MASK   (1 << 2)
>> +#define TB_FLAGS_PRIV_HYP_ACCESS_MASK   (1 << 3)
> 
> You can't do this, as you're now overlapping
> 
As you mentioned below HYP_ACCESS_MASK is set directly by hyp
instruction translation, there is no overlapping if it's not part of
TB_FLAGS.

> FIELD(TB_FLAGS, LMUL, 3, 3)
> 
> You'd need to shift all other fields up to do this.
> There is room, to be sure.
> 
> Or you could reuse MMU mode number 2.  For that you'd need to separate
> DisasContext.mem_idx from priv.  Which should probably be done anyway,
> because tests such as
> 
Yes, it looks good to reuse number 2. I tried this v3 patch again with a
different MMUIdx_S_SUM number, only 5 is okay below 8, for the other
number there is no kernel message from guest after opensbi output. I
need to find it out.

> insn_trans/trans_privileged.c.inc:    if
> (semihosting_enabled(ctx->mem_idx < PRV_S) &&
> 
> are already borderline wrong.
>Yes, it's better not to compare idx to priv.

> I suggest
> 
> - #define TB_FLAGS_PRIV_MMU_MASK                3
> - #define TB_FLAGS_PRIV_HYP_ACCESS_MASK   (1 << 2)
> 
> HYP_ACCESS_MASK never needed to be part of TB_FLAGS; it is only set
> directly by the hyp access instruction translation.  Drop the PRIV mask
> and represent that directly:
> 
> - FIELD(TB_FLAGS, MEM_IDX, 0, 3)
> + FIELD(TB_FLAGS, PRIV, 0, 2)
> + FIELD(TB_FLAGS, SUM, 2, 1)
> 
> Let SUM occupy the released bit.
> 
> In internals.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)
> 
> 
> In riscv_tr_init_disas_context:
> 
>     ctx->priv = FIELD_EX32(tb_flags, TB_FLAGS, PRIV);
>     ctx->mmu_idx = ctx->priv;
>     if (ctx->mmu_idx == PRV_S && FIELD_EX32(tb_flags, TB_FLAGS, SUM)) {
>         ctx->mmu_idx = MMUIdx_S_SUM;
>     }
> 
There is MSTATUS_MPRV and MSTATUS_MPP kind of thing, priv+sum is not
able to represent all of the status, probably we can just add an extra
'priv' at the back of TB_FLAGS?

Thanks,
Fei.

> and similarly in riscv_cpu_mmu_index.
> 
> Fix all uses of ctx->mmu_idx that are not specifically for memory
> operations.
> 
> 
> r~

Re: [PATCH v3] target/riscv: reduce overhead of MSTATUS_SUM change

[Wu, Fei]

On 3/23/2023 8:38 AM, Wu, Fei wrote:
> On 3/22/2023 9:19 PM, Richard Henderson wrote:
>> On 3/22/23 05:12, Fei Wu wrote:
>>> 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-param.h  |  2 +-
>>>   target/riscv/cpu.h        |  2 +-
>>>   target/riscv/cpu_bits.h   |  1 +
>>>   target/riscv/cpu_helper.c | 11 +++++++++++
>>>   target/riscv/csr.c        |  2 +-
>>>   5 files changed, 15 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/target/riscv/cpu-param.h b/target/riscv/cpu-param.h
>>> index ebaf26d26d..9e21b943f9 100644
>>> --- a/target/riscv/cpu-param.h
>>> +++ b/target/riscv/cpu-param.h
>>> @@ -27,6 +27,6 @@
>>>    *  - S mode HLV/HLVX/HSV 0b101
>>>    *  - M mode HLV/HLVX/HSV 0b111
>>>    */
>>> -#define NB_MMU_MODES 8
>>> +#define NB_MMU_MODES 16
>>
>> This line no longer exists on master.
>> The comment above should be updated, and perhaps moved.
>>
>>>   #define TB_FLAGS_PRIV_MMU_MASK                3
>>> -#define TB_FLAGS_PRIV_HYP_ACCESS_MASK   (1 << 2)
>>> +#define TB_FLAGS_PRIV_HYP_ACCESS_MASK   (1 << 3)
>>
>> You can't do this, as you're now overlapping
>>
> As you mentioned below HYP_ACCESS_MASK is set directly by hyp
> instruction translation, there is no overlapping if it's not part of
> TB_FLAGS.
> 
>> FIELD(TB_FLAGS, LMUL, 3, 3)
>>
>> You'd need to shift all other fields up to do this.
>> There is room, to be sure.
>>
>> Or you could reuse MMU mode number 2.  For that you'd need to separate
>> DisasContext.mem_idx from priv.  Which should probably be done anyway,
>> because tests such as
>>
> Yes, it looks good to reuse number 2. I tried this v3 patch again with a
> different MMUIdx_S_SUM number, only 5 is okay below 8, for the other
> number there is no kernel message from guest after opensbi output. I
> need to find it out.
> 
In get_physical_address():
    int mode = mmu_idx & TB_FLAGS_PRIV_MMU_MASK;

We do need separate priv from idx.

Thanks,
Fei.

>> insn_trans/trans_privileged.c.inc:    if
>> (semihosting_enabled(ctx->mem_idx < PRV_S) &&
>>
>> are already borderline wrong.
>> Yes, it's better not to compare idx to priv.
> 
>> I suggest
>>
>> - #define TB_FLAGS_PRIV_MMU_MASK                3
>> - #define TB_FLAGS_PRIV_HYP_ACCESS_MASK   (1 << 2)
>>
>> HYP_ACCESS_MASK never needed to be part of TB_FLAGS; it is only set
>> directly by the hyp access instruction translation.  Drop the PRIV mask
>> and represent that directly:
>>
>> - FIELD(TB_FLAGS, MEM_IDX, 0, 3)
>> + FIELD(TB_FLAGS, PRIV, 0, 2)
>> + FIELD(TB_FLAGS, SUM, 2, 1)
>>
>> Let SUM occupy the released bit.
>>
>> In internals.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)
>>
>>
>> In riscv_tr_init_disas_context:
>>
>>     ctx->priv = FIELD_EX32(tb_flags, TB_FLAGS, PRIV);
>>     ctx->mmu_idx = ctx->priv;
>>     if (ctx->mmu_idx == PRV_S && FIELD_EX32(tb_flags, TB_FLAGS, SUM)) {
>>         ctx->mmu_idx = MMUIdx_S_SUM;
>>     }
>>
> There is MSTATUS_MPRV and MSTATUS_MPP kind of thing, priv+sum is not
> able to represent all of the status, probably we can just add an extra
> 'priv' at the back of TB_FLAGS?
> 
> Thanks,
> Fei.
> 
>> and similarly in riscv_cpu_mmu_index.
>>
>> Fix all uses of ctx->mmu_idx that are not specifically for memory
>> operations.
>>
>>
>> r~
> 

Re: [PATCH v3] target/riscv: reduce overhead of MSTATUS_SUM change

[Wu, Fei]

On 3/23/2023 8:38 AM, Wu, Fei wrote:
> On 3/22/2023 9:19 PM, Richard Henderson wrote:
>> On 3/22/23 05:12, Fei Wu wrote:
>>> 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-param.h  |  2 +-
>>>   target/riscv/cpu.h        |  2 +-
>>>   target/riscv/cpu_bits.h   |  1 +
>>>   target/riscv/cpu_helper.c | 11 +++++++++++
>>>   target/riscv/csr.c        |  2 +-
>>>   5 files changed, 15 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/target/riscv/cpu-param.h b/target/riscv/cpu-param.h
>>> index ebaf26d26d..9e21b943f9 100644
>>> --- a/target/riscv/cpu-param.h
>>> +++ b/target/riscv/cpu-param.h
>>> @@ -27,6 +27,6 @@
>>>    *  - S mode HLV/HLVX/HSV 0b101
>>>    *  - M mode HLV/HLVX/HSV 0b111
>>>    */
>>> -#define NB_MMU_MODES 8
>>> +#define NB_MMU_MODES 16
>>
>> This line no longer exists on master.
>> The comment above should be updated, and perhaps moved.
>>
>>>   #define TB_FLAGS_PRIV_MMU_MASK                3
>>> -#define TB_FLAGS_PRIV_HYP_ACCESS_MASK   (1 << 2)
>>> +#define TB_FLAGS_PRIV_HYP_ACCESS_MASK   (1 << 3)
>>
>> You can't do this, as you're now overlapping
>>
> As you mentioned below HYP_ACCESS_MASK is set directly by hyp
> instruction translation, there is no overlapping if it's not part of
> TB_FLAGS.
> 
>> FIELD(TB_FLAGS, LMUL, 3, 3)
>>
>> You'd need to shift all other fields up to do this.
>> There is room, to be sure.
>>
>> Or you could reuse MMU mode number 2.  For that you'd need to separate
>> DisasContext.mem_idx from priv.  Which should probably be done anyway,
>> because tests such as
>>
> Yes, it looks good to reuse number 2. I tried this v3 patch again with a
> different MMUIdx_S_SUM number, only 5 is okay below 8, for the other
> number there is no kernel message from guest after opensbi output. I
> need to find it out.
> 
>> insn_trans/trans_privileged.c.inc:    if
>> (semihosting_enabled(ctx->mem_idx < PRV_S) &&
>>
>> are already borderline wrong.
>> Yes, it's better not to compare idx to priv.
> 
>> I suggest
>>
>> - #define TB_FLAGS_PRIV_MMU_MASK                3
>> - #define TB_FLAGS_PRIV_HYP_ACCESS_MASK   (1 << 2)
>>
>> HYP_ACCESS_MASK never needed to be part of TB_FLAGS; it is only set
>> directly by the hyp access instruction translation.  Drop the PRIV mask
>> and represent that directly:
>>
>> - FIELD(TB_FLAGS, MEM_IDX, 0, 3)
>> + FIELD(TB_FLAGS, PRIV, 0, 2)
>> + FIELD(TB_FLAGS, SUM, 2, 1)
>>
>> Let SUM occupy the released bit.
>>
>> In internals.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)
>>
>>
>> In riscv_tr_init_disas_context:
>>
>>     ctx->priv = FIELD_EX32(tb_flags, TB_FLAGS, PRIV);
>>     ctx->mmu_idx = ctx->priv;
>>     if (ctx->mmu_idx == PRV_S && FIELD_EX32(tb_flags, TB_FLAGS, SUM)) {
>>         ctx->mmu_idx = MMUIdx_S_SUM;
>>     }
>>
> There is MSTATUS_MPRV and MSTATUS_MPP kind of thing, priv+sum is not
> able to represent all of the status, probably we can just add an extra
> 'priv' at the back of TB_FLAGS?
> 
MPRV+MPP looks not necessary be in TB_FLAGS, it's just used to calculate
the mmu_idx. Is it necessary to encode SUM into TB_FLAGS?

Thanks,
Fei.

> Thanks,
> Fei.
> 
>> and similarly in riscv_cpu_mmu_index.
>>
>> Fix all uses of ctx->mmu_idx that are not specifically for memory
>> operations.
>>
>>
>> r~
> 

Re: [PATCH v3] target/riscv: reduce overhead of MSTATUS_SUM change

[Richard Henderson]

On 3/22/23 17:38, Wu, Fei wrote:
> There is MSTATUS_MPRV and MSTATUS_MPP kind of thing, priv+sum is not
> able to represent all of the status, probably we can just add an extra
> 'priv' at the back of TB_FLAGS?

Yes, I think that's required.


r~

Re: [PATCH v3] target/riscv: reduce overhead of MSTATUS_SUM change

[Richard Henderson]

On 3/23/23 06:24, Wu, Fei wrote:
>> There is MSTATUS_MPRV and MSTATUS_MPP kind of thing, priv+sum is not
>> able to represent all of the status, probably we can just add an extra
>> 'priv' at the back of TB_FLAGS?
>>
> MPRV+MPP looks not necessary be in TB_FLAGS, it's just used to calculate
> the mmu_idx. Is it necessary to encode SUM into TB_FLAGS?

We need priv separate from midx for priv_level() and similar uses.
We do not want MPRV=1, MPP=U to prevent M-mode from executing M instructions.

With PRIV and MIDX in TB_FLAGS, we do not need to separately encode MPRV+MPP+SUM, because 
all of those are composited into MIDX.


r~