From: Christoph Müllner <christoph.muellner@vrull.eu>

Setting flags using OR might work, but is not optimal
for a couple of reasons:
* No way grep for stores to the field MEM_IDX.
* The return value of cpu_mmu_index() is not masked
  (not a real problem as long as cpu_mmu_index() returns only valid values).
* If the offset of MEM_IDX would get moved to non-0, then this code
  would not work anymore.

Let's use the FIELD_DP32() macro instead of the OR, which is already
used for most other flags.

Signed-off-by: Christoph Müllner <christoph.muellner@vrull.eu>
---
 target/riscv/cpu_helper.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index 278d163803..d68b6b351d 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -80,7 +80,8 @@ void cpu_get_tb_cpu_state(CPURISCVState *env, target_ulong *pc,
     flags |= TB_FLAGS_MSTATUS_FS;
     flags |= TB_FLAGS_MSTATUS_VS;
 #else
-    flags |= cpu_mmu_index(env, 0);
+    flags = FIELD_DP32(flags, TB_FLAGS, MEM_IDX, cpu_mmu_index(env, 0));
+
     if (riscv_cpu_fp_enabled(env)) {
         flags |= env->mstatus & MSTATUS_FS;
     }
-- 
2.38.1

On Fri, Dec 9, 2022 at 1:12 AM Christoph Muellner
<christoph.muellner@vrull.eu> wrote:
>
> From: Christoph Müllner <christoph.muellner@vrull.eu>
>
> Setting flags using OR might work, but is not optimal
> for a couple of reasons:
> * No way grep for stores to the field MEM_IDX.
> * The return value of cpu_mmu_index() is not masked
>   (not a real problem as long as cpu_mmu_index() returns only valid values).
> * If the offset of MEM_IDX would get moved to non-0, then this code
>   would not work anymore.
>
> Let's use the FIELD_DP32() macro instead of the OR, which is already
> used for most other flags.
>
> Signed-off-by: Christoph Müllner <christoph.muellner@vrull.eu>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  target/riscv/cpu_helper.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index 278d163803..d68b6b351d 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -80,7 +80,8 @@ void cpu_get_tb_cpu_state(CPURISCVState *env, target_ulong *pc,
>      flags |= TB_FLAGS_MSTATUS_FS;
>      flags |= TB_FLAGS_MSTATUS_VS;
>  #else
> -    flags |= cpu_mmu_index(env, 0);
> +    flags = FIELD_DP32(flags, TB_FLAGS, MEM_IDX, cpu_mmu_index(env, 0));
> +
>      if (riscv_cpu_fp_enabled(env)) {
>          flags |= env->mstatus & MSTATUS_FS;
>      }
> --
> 2.38.1
>
>

On 2022/12/8 23:11, Christoph Muellner wrote:
> From: Christoph Müllner <christoph.muellner@vrull.eu>
>
> Setting flags using OR might work, but is not optimal
> for a couple of reasons:
> * No way grep for stores to the field MEM_IDX.
> * The return value of cpu_mmu_index() is not masked
>    (not a real problem as long as cpu_mmu_index() returns only valid values).
> * If the offset of MEM_IDX would get moved to non-0, then this code
>    would not work anymore.
>
> Let's use the FIELD_DP32() macro instead of the OR, which is already
> used for most other flags.
>
> Signed-off-by: Christoph Müllner <christoph.muellner@vrull.eu>
> ---
>   target/riscv/cpu_helper.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index 278d163803..d68b6b351d 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -80,7 +80,8 @@ void cpu_get_tb_cpu_state(CPURISCVState *env, target_ulong *pc,
>       flags |= TB_FLAGS_MSTATUS_FS;
>       flags |= TB_FLAGS_MSTATUS_VS;
>   #else
> -    flags |= cpu_mmu_index(env, 0);
> +    flags = FIELD_DP32(flags, TB_FLAGS, MEM_IDX, cpu_mmu_index(env, 0));
> +
>       if (riscv_cpu_fp_enabled(env)) {
>           flags |= env->mstatus & MSTATUS_FS;
>       }

We may should rename cpu_mmu_index to cpu_mem_idx and 
TB_FLAGS_PRIV_MMU_MASK to TB_FLAGS_PRIV_MEM_MASK.

We can also remove the TB_FLAGS_PRIV_MMU_MASK as the position of MEM_IDX 
in tb_flags may change in the future.


Otherwise, this patch looks good to me,

Reviewed-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>