Re: [PATCH 02/13] target/riscv: Extend pc for runtime pc write

2021-11-02 Thread Richard Henderson 

On 11/1/21 9:48 PM, LIU Zhiwei wrote:


On 2021/11/1 下午6:33, Richard Henderson wrote:

On 11/1/21 6:01 AM, LIU Zhiwei wrote:

In some cases, we must restore the guest PC to the address of the start of
the TB, such as when the instruction counter hit zero. So extend pc register
according to current xlen for these cases.

Signed-off-by: LIU Zhiwei 
---
  target/riscv/cpu.c    | 20 +---
  target/riscv/cpu.h    |  2 ++
  target/riscv/cpu_helper.c |  2 +-
  3 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 7d53125dbc..7eefd4f6a6 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -319,7 +319,12 @@ static void riscv_cpu_set_pc(CPUState *cs, vaddr value)
  {
  RISCVCPU *cpu = RISCV_CPU(cs);
  CPURISCVState *env = &cpu->env;
-    env->pc = value;
+
+    if (cpu_get_xl(env) == MXL_RV32) {
+    env->pc = (int32_t)value;
+    } else {
+    env->pc = value;
+    }
  }


Good.


  static void riscv_cpu_synchronize_from_tb(CPUState *cs,
@@ -327,7 +332,12 @@ static void riscv_cpu_synchronize_from_tb(CPUState *cs,
  {
  RISCVCPU *cpu = RISCV_CPU(cs);
  CPURISCVState *env = &cpu->env;
-    env->pc = tb->pc;
+
+    if (cpu_get_xl(env) == MXL_RV32) {
+    env->pc = (int32_t)tb->pc;
+    } else {
+    env->pc = tb->pc;
+    }


Bad, since TB->PC should be extended properly.
Though this waits on a change to cpu_get_tb_cpu_state.


Should the env->pc always hold the sign-extend result? In cpu_get_tb_cpu_state, we just 
truncate to the XLEN bits.


Oops, I mis-read patch 3; I thought that sign-extended.  Hmm.

I guess we need to zero-extend the pc for patch 3, to get the correct result in translate 
when we read from memory.  Therefore we need to sign-extend here to get the correct value 
back in env->pc.


Oh, let's not re-compute cpu_get_xl here, and restore_state_to_opc; it is in

FIELD_EX32(tb->flags, TB_FLAGS, XL)


r~

Re: [PATCH 02/13] target/riscv: Extend pc for runtime pc write

2021-11-01 Thread LIU Zhiwei 



On 2021/11/1 下午6:33, Richard Henderson wrote:

On 11/1/21 6:01 AM, LIU Zhiwei wrote:
In some cases, we must restore the guest PC to the address of the 
start of
the TB, such as when the instruction counter hit zero. So extend pc 
register

according to current xlen for these cases.

Signed-off-by: LIU Zhiwei 
---
  target/riscv/cpu.c    | 20 +---
  target/riscv/cpu.h    |  2 ++
  target/riscv/cpu_helper.c |  2 +-
  3 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 7d53125dbc..7eefd4f6a6 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -319,7 +319,12 @@ static void riscv_cpu_set_pc(CPUState *cs, vaddr 
value)

  {
  RISCVCPU *cpu = RISCV_CPU(cs);
  CPURISCVState *env = &cpu->env;
-    env->pc = value;
+
+    if (cpu_get_xl(env) == MXL_RV32) {
+    env->pc = (int32_t)value;
+    } else {
+    env->pc = value;
+    }
  }


Good.


  static void riscv_cpu_synchronize_from_tb(CPUState *cs,
@@ -327,7 +332,12 @@ static void 
riscv_cpu_synchronize_from_tb(CPUState *cs,

  {
  RISCVCPU *cpu = RISCV_CPU(cs);
  CPURISCVState *env = &cpu->env;
-    env->pc = tb->pc;
+
+    if (cpu_get_xl(env) == MXL_RV32) {
+    env->pc = (int32_t)tb->pc;
+    } else {
+    env->pc = tb->pc;
+    }


Bad, since TB->PC should be extended properly.
Though this waits on a change to cpu_get_tb_cpu_state.


Should the env->pc always hold the sign-extend result? In 
cpu_get_tb_cpu_state, we just truncate to the XLEN bits.


Thanks,
Zhiwei




@@ -348,7 +358,11 @@ static bool riscv_cpu_has_work(CPUState *cs)
  void restore_state_to_opc(CPURISCVState *env, TranslationBlock *tb,
    target_ulong *data)
  {
-    env->pc = data[0];
+   if (cpu_get_xl(env) == MXL_RV32) {
+    env->pc = (int32_t)data[0];
+    } else {
+    env->pc = data[0];
+    }


Likewise.


r~

Re: [PATCH 02/13] target/riscv: Extend pc for runtime pc write

2021-11-01 Thread Richard Henderson 

On 11/1/21 6:01 AM, LIU Zhiwei wrote:

In some cases, we must restore the guest PC to the address of the start of
the TB, such as when the instruction counter hit zero. So extend pc register
according to current xlen for these cases.

Signed-off-by: LIU Zhiwei 
---
  target/riscv/cpu.c| 20 +---
  target/riscv/cpu.h|  2 ++
  target/riscv/cpu_helper.c |  2 +-
  3 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 7d53125dbc..7eefd4f6a6 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -319,7 +319,12 @@ static void riscv_cpu_set_pc(CPUState *cs, vaddr value)
  {
  RISCVCPU *cpu = RISCV_CPU(cs);
  CPURISCVState *env = &cpu->env;
-env->pc = value;
+
+if (cpu_get_xl(env) == MXL_RV32) {
+env->pc = (int32_t)value;
+} else {
+env->pc = value;
+}
  }
  


Good.


  static void riscv_cpu_synchronize_from_tb(CPUState *cs,
@@ -327,7 +332,12 @@ static void riscv_cpu_synchronize_from_tb(CPUState *cs,
  {
  RISCVCPU *cpu = RISCV_CPU(cs);
  CPURISCVState *env = &cpu->env;
-env->pc = tb->pc;
+
+if (cpu_get_xl(env) == MXL_RV32) {
+env->pc = (int32_t)tb->pc;
+} else {
+env->pc = tb->pc;
+}


Bad, since TB->PC should be extended properly.
Though this waits on a change to cpu_get_tb_cpu_state.


@@ -348,7 +358,11 @@ static bool riscv_cpu_has_work(CPUState *cs)
  void restore_state_to_opc(CPURISCVState *env, TranslationBlock *tb,
target_ulong *data)
  {
-env->pc = data[0];
+   if (cpu_get_xl(env) == MXL_RV32) {
+env->pc = (int32_t)data[0];
+} else {
+env->pc = data[0];
+}


Likewise.


r~

[PATCH 02/13] target/riscv: Extend pc for runtime pc write

2021-11-01 Thread LIU Zhiwei 
In some cases, we must restore the guest PC to the address of the start of
the TB, such as when the instruction counter hit zero. So extend pc register
according to current xlen for these cases.

Signed-off-by: LIU Zhiwei 
---
 target/riscv/cpu.c| 20 +---
 target/riscv/cpu.h|  2 ++
 target/riscv/cpu_helper.c |  2 +-
 3 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 7d53125dbc..7eefd4f6a6 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -319,7 +319,12 @@ static void riscv_cpu_set_pc(CPUState *cs, vaddr value)
 {
 RISCVCPU *cpu = RISCV_CPU(cs);
 CPURISCVState *env = &cpu->env;
-env->pc = value;
+
+if (cpu_get_xl(env) == MXL_RV32) {
+env->pc = (int32_t)value;
+} else {
+env->pc = value;
+}
 }
 
 static void riscv_cpu_synchronize_from_tb(CPUState *cs,
@@ -327,7 +332,12 @@ static void riscv_cpu_synchronize_from_tb(CPUState *cs,
 {
 RISCVCPU *cpu = RISCV_CPU(cs);
 CPURISCVState *env = &cpu->env;
-env->pc = tb->pc;
+
+if (cpu_get_xl(env) == MXL_RV32) {
+env->pc = (int32_t)tb->pc;
+} else {
+env->pc = tb->pc;
+}
 }
 
 static bool riscv_cpu_has_work(CPUState *cs)
@@ -348,7 +358,11 @@ static bool riscv_cpu_has_work(CPUState *cs)
 void restore_state_to_opc(CPURISCVState *env, TranslationBlock *tb,
   target_ulong *data)
 {
-env->pc = data[0];
+   if (cpu_get_xl(env) == MXL_RV32) {
+env->pc = (int32_t)data[0];
+} else {
+env->pc = data[0];
+}
 }
 
 static void riscv_cpu_reset(DeviceState *dev)
diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index 0760c0af93..8befff0166 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -420,6 +420,8 @@ static inline RISCVMXL riscv_cpu_mxl(CPURISCVState *env)
 }
 #endif
 
+RISCVMXL cpu_get_xl(CPURISCVState *env);
+
 /*
  * A simplification for VLMAX
  * = (1 << LMUL) * VLEN / (8 * (1 << SEW))
diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index f30ff672f8..7d0aee6769 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -35,7 +35,7 @@ int riscv_cpu_mmu_index(CPURISCVState *env, bool ifetch)
 #endif
 }
 
-static RISCVMXL cpu_get_xl(CPURISCVState *env)
+RISCVMXL cpu_get_xl(CPURISCVState *env)
 {
 #if defined(TARGET_RISCV32)
 return MXL_RV32;
-- 
2.25.1