1. Patchew
  2. QEMU
  3. [PATCH for-8.1 00/17] centralize CPU extensions logic
  4. View patch

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

Daniel Henrique Barboza posted 17 patches 2 years, 11 months ago
Maintainers: Palmer Dabbelt <palmer@dabbelt.com>, Alistair Francis <alistair.francis@wdc.com>, Bin Meng <bin.meng@windriver.com>, Weiwei Li <liweiwei@iscas.ac.cn>, Daniel Henrique Barboza <dbarboza@ventanamicro.com>, Liu Zhiwei <zhiwei_liu@linux.alibaba.com>
[PATCH for-8.1 03/17] target/riscv/cpu.c: remove set_priv_version()
Posted by Daniel Henrique Barboza 2 years, 11 months ago 
The setter is doing nothing special. Just set env->priv_ver directly.

Signed-off-by: Daniel Henrique Barboza <dbarboza@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 */
-- 
2.39.2
Re: [PATCH for-8.1 03/17] target/riscv/cpu.c: remove set_priv_version()
Posted by LIU Zhiwei 2 years, 11 months ago 
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.

Zhiwei

>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@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 */
Re: [PATCH for-8.1 03/17] target/riscv/cpu.c: remove set_priv_version()
Posted by Daniel Henrique Barboza 2 years, 11 months ago 

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 <dbarboza@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 */
Re: [PATCH for-8.1 03/17] target/riscv/cpu.c: remove set_priv_version()
Posted by Alistair Francis 2 years, 11 months ago 
On Fri, Mar 10, 2023 at 2:23 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
>
>
> 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.

I tend to agree. I don't think they add anything. I guess you could
debate they are kind of self commenting as the function name describes
what is happening, but I think in a lot of cases it's pretty clear as
is.

Alistair

>
>
> Thanks,
>
>
> Daniel
>
>
>
> >
> > Zhiwei
> >
> >>
> >> Signed-off-by: Daniel Henrique Barboza <dbarboza@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 */
>

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.