Re: [PATCH for-8.1 03/17] target/riscv/cpu.c: remove set_priv_version()

[Daniel Henrique Barboza]

On 3/9/23 04:28, LIU Zhiwei wrote:

On 2023/3/9 4:19, Daniel Henrique Barboza wrote:

The setter is doing nothing special. Just set env->priv_ver directly.

IMHO, No better than the older implementation.

In the current context having a setter means that the function is doing
something else other than simply setting the attr. Because we're setting a
lot of other 'env' attrs directly: env->pc, env->priv, env->menvcfg and
so on. So a setter is a special function (e.g. set_misa()).

But then set_priv_version() and set_vext_version() are just setting
env->priv_ver/env->vext_version and nothing else. This means that every
time we read

"set_priv_version(env, val)"

We're either required to remember that this is just a simple setter or we spend
a few seconds looking it up to see that it's a simple setter. We could, instead,
just read

"env->priv_ver = val"

and moved on.


I really think we should get rid of all these kind of setters in the code. It's 
not
like these are user facing APIs that needs encapsulation.


Thanks,


Daniel

Zhiwei

Signed-off-by: Daniel Henrique Barboza <dbarb...@ventanamicro.com>
---
  target/riscv/cpu.c | 30 +++++++++++++-----------------
  1 file changed, 13 insertions(+), 17 deletions(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 0baed79ec2..964817b9d2 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -240,11 +240,6 @@ static void set_misa(CPURISCVState *env, RISCVMXL mxl, 
uint32_t ext)
      env->misa_ext_mask = env->misa_ext = ext;
  }
-static void set_priv_version(CPURISCVState *env, int priv_ver)
-{
-    env->priv_ver = priv_ver;
-}
-
  #ifndef CONFIG_USER_ONLY
  static uint8_t satp_mode_from_str(const char *satp_mode_str)
  {
@@ -343,7 +338,7 @@ static void riscv_any_cpu_init(Object *obj)
                                      VM_1_10_SV32 : VM_1_10_SV57);
  #endif
-    set_priv_version(env, PRIV_VERSION_1_12_0);
+    env->priv_ver = PRIV_VERSION_1_12_0;
      register_cpu_props(obj);
  }
@@ -355,7 +350,7 @@ static void rv64_base_cpu_init(Object *obj)
      set_misa(env, MXL_RV64, 0);
      register_cpu_props(obj);
      /* Set latest version of privileged specification */
-    set_priv_version(env, PRIV_VERSION_1_12_0);
+    env->priv_ver = PRIV_VERSION_1_12_0;
  #ifndef CONFIG_USER_ONLY
      set_satp_mode_max_supported(RISCV_CPU(obj), VM_1_10_SV57);
  #endif
@@ -366,7 +361,7 @@ static void rv64_sifive_u_cpu_init(Object *obj)
      CPURISCVState *env = &RISCV_CPU(obj)->env;
      set_misa(env, MXL_RV64, RVI | RVM | RVA | RVF | RVD | RVC | RVS | RVU);
      register_cpu_props(obj);
-    set_priv_version(env, PRIV_VERSION_1_10_0);
+    env->priv_ver = PRIV_VERSION_1_10_0;
  #ifndef CONFIG_USER_ONLY
      set_satp_mode_max_supported(RISCV_CPU(obj), VM_1_10_SV39);
  #endif
@@ -379,7 +374,7 @@ static void rv64_sifive_e_cpu_init(Object *obj)
      set_misa(env, MXL_RV64, RVI | RVM | RVA | RVC | RVU);
      register_cpu_props(obj);
-    set_priv_version(env, PRIV_VERSION_1_10_0);
+    env->priv_ver = PRIV_VERSION_1_10_0;
      cpu->cfg.mmu = false;
  #ifndef CONFIG_USER_ONLY
      set_satp_mode_max_supported(cpu, VM_1_10_MBARE);
@@ -392,7 +387,7 @@ static void rv64_thead_c906_cpu_init(Object *obj)
      RISCVCPU *cpu = RISCV_CPU(obj);
      set_misa(env, MXL_RV64, RVI | RVM | RVA | RVF | RVD | RVC | RVS | RVU);
-    set_priv_version(env, PRIV_VERSION_1_11_0);
+    env->priv_ver = PRIV_VERSION_1_11_0;
      cpu->cfg.ext_g = true;
      cpu->cfg.ext_c = true;
@@ -431,7 +426,7 @@ static void rv128_base_cpu_init(Object *obj)
      set_misa(env, MXL_RV128, 0);
      register_cpu_props(obj);
      /* Set latest version of privileged specification */
-    set_priv_version(env, PRIV_VERSION_1_12_0);
+    env->priv_ver = PRIV_VERSION_1_12_0;
  #ifndef CONFIG_USER_ONLY
      set_satp_mode_max_supported(RISCV_CPU(obj), VM_1_10_SV57);
  #endif
@@ -444,7 +439,7 @@ static void rv32_base_cpu_init(Object *obj)
      set_misa(env, MXL_RV32, 0);
      register_cpu_props(obj);
      /* Set latest version of privileged specification */
-    set_priv_version(env, PRIV_VERSION_1_12_0);
+    env->priv_ver = PRIV_VERSION_1_12_0;
  #ifndef CONFIG_USER_ONLY
      set_satp_mode_max_supported(RISCV_CPU(obj), VM_1_10_SV32);
  #endif
@@ -454,8 +449,9 @@ static void rv32_sifive_u_cpu_init(Object *obj)
  {
      CPURISCVState *env = &RISCV_CPU(obj)->env;
      set_misa(env, MXL_RV32, RVI | RVM | RVA | RVF | RVD | RVC | RVS | RVU);
+
      register_cpu_props(obj);
-    set_priv_version(env, PRIV_VERSION_1_10_0);
+    env->priv_ver = PRIV_VERSION_1_10_0;
  #ifndef CONFIG_USER_ONLY
      set_satp_mode_max_supported(RISCV_CPU(obj), VM_1_10_SV32);
  #endif
@@ -468,7 +464,7 @@ static void rv32_sifive_e_cpu_init(Object *obj)
      set_misa(env, MXL_RV32, RVI | RVM | RVA | RVC | RVU);
      register_cpu_props(obj);
-    set_priv_version(env, PRIV_VERSION_1_10_0);
+    env->priv_ver = PRIV_VERSION_1_10_0;
      cpu->cfg.mmu = false;
  #ifndef CONFIG_USER_ONLY
      set_satp_mode_max_supported(cpu, VM_1_10_MBARE);
@@ -482,7 +478,7 @@ static void rv32_ibex_cpu_init(Object *obj)
      set_misa(env, MXL_RV32, RVI | RVM | RVC | RVU);
      register_cpu_props(obj);
-    set_priv_version(env, PRIV_VERSION_1_11_0);
+    env->priv_ver = PRIV_VERSION_1_11_0;
      cpu->cfg.mmu = false;
  #ifndef CONFIG_USER_ONLY
      set_satp_mode_max_supported(cpu, VM_1_10_MBARE);
@@ -497,7 +493,7 @@ static void rv32_imafcu_nommu_cpu_init(Object *obj)
      set_misa(env, MXL_RV32, RVI | RVM | RVA | RVF | RVC | RVU);
      register_cpu_props(obj);
-    set_priv_version(env, PRIV_VERSION_1_10_0);
+    env->priv_ver = PRIV_VERSION_1_10_0;
      cpu->cfg.mmu = false;
  #ifndef CONFIG_USER_ONLY
      set_satp_mode_max_supported(cpu, VM_1_10_MBARE);
@@ -1159,7 +1155,7 @@ static void riscv_cpu_realize(DeviceState *dev, Error 
**errp)
      }
      if (priv_version >= PRIV_VERSION_1_10_0) {
-        set_priv_version(env, priv_version);
+        env->priv_ver = priv_version;
      }
      /* Force disable extensions if priv spec version does not match */