qemu-riscv
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: LIU Zhiwei
Subject: Re: [PATCH v4 1/2] target/riscv: separate priv from mmu_idx
Date: Thu, 23 Mar 2023 14:59:04 +0800
User-agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101 Thunderbird/102.9.0


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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]