Re: [PATCH for-8.1 03/17] target/riscv/cpu.c: remove set_priv_version()
On Fri, Mar 10, 2023 at 2:23 AM Daniel Henrique Barboza 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 > >> --- > >> 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 >
Re: [PATCH for-8.1 03/17] target/riscv/cpu.c: remove set_priv_version()
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 --- 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
Re: [PATCH for-8.1 03/17] target/riscv/cpu.c: remove set_priv_version()
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 --- 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 >=
[PATCH for-8.1 03/17] target/riscv/cpu.c: remove set_priv_version()
The setter is doing nothing special. Just set env->priv_ver directly. Signed-off-by: Daniel Henrique Barboza --- 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.