| Message ID | 20230216215550.1011637-3-dbarboza@ventanamicro.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | make write_misa a no-op and FEATURE_* cleanups | expand |
On Fri, Feb 17, 2023 at 5:57 AM Daniel Henrique Barboza <dbarboza@ventanamicro.com> wrote: > > We're going to do changes that requires accessing the RISCVCPUConfig > struct from the RISCVCPU, having access only to a CPURISCVState 'env' > pointer. Add a helper to make the code easier to read. > > Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com> > --- > target/riscv/cpu.h | 5 +++++ > 1 file changed, 5 insertions(+) > Looks like the RB tag is missing somehow? Reviewed-by: Bin Meng <bmeng@tinylab.org>
On Fri, Feb 17, 2023 at 8:45 AM Bin Meng <bmeng.cn@gmail.com> wrote: > > On Fri, Feb 17, 2023 at 5:57 AM Daniel Henrique Barboza > <dbarboza@ventanamicro.com> wrote: > > > > We're going to do changes that requires accessing the RISCVCPUConfig > > struct from the RISCVCPU, having access only to a CPURISCVState 'env' > > pointer. Add a helper to make the code easier to read. > > > > Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com> > > --- > > target/riscv/cpu.h | 5 +++++ > > 1 file changed, 5 insertions(+) > > > > Looks like the RB tag is missing somehow? Never mind. I see the difference :) Regards, Bin
On 2023/2/17 5:55, Daniel Henrique Barboza wrote: > We're going to do changes that requires accessing the RISCVCPUConfig > struct from the RISCVCPU, having access only to a CPURISCVState 'env' > pointer. Add a helper to make the code easier to read. > > Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com> > --- > target/riscv/cpu.h | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h > index 01803a020d..5e9626837b 100644 > --- a/target/riscv/cpu.h > +++ b/target/riscv/cpu.h > @@ -653,6 +653,11 @@ static inline RISCVMXL riscv_cpu_mxl(CPURISCVState *env) > #endif > #define riscv_cpu_mxl_bits(env) (1UL << (4 + riscv_cpu_mxl(env))) > > +static inline const RISCVCPUConfig *riscv_cpu_cfg(CPURISCVState *env) > +{ > + return &env_archcpu(env)->cfg; > +} > + > #if defined(TARGET_RISCV32) > #define cpu_recompute_xl(env) ((void)(env), MXL_RV32) > #else Reviewed-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com> Zhiwei
On 2023/2/17 5:55, Daniel Henrique Barboza wrote: > We're going to do changes that requires accessing the RISCVCPUConfig > struct from the RISCVCPU, having access only to a CPURISCVState 'env' > pointer. Add a helper to make the code easier to read. > > Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com> > --- > target/riscv/cpu.h | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h > index 01803a020d..5e9626837b 100644 > --- a/target/riscv/cpu.h > +++ b/target/riscv/cpu.h > @@ -653,6 +653,11 @@ static inline RISCVMXL riscv_cpu_mxl(CPURISCVState *env) > #endif > #define riscv_cpu_mxl_bits(env) (1UL << (4 + riscv_cpu_mxl(env))) > > +static inline const RISCVCPUConfig *riscv_cpu_cfg(CPURISCVState *env) Maybe we should static inline const* RISCVCPUConfig riscv_cpu_cfg(CPURISCVState *env) or just static inline RISCVCPUConfig *riscv_cpu_cfg(CPURISCVState *env) Zhiwei > +{ > + return &env_archcpu(env)->cfg; > +} > + > #if defined(TARGET_RISCV32) > #define cpu_recompute_xl(env) ((void)(env), MXL_RV32) > #else
On 2023/2/17 9:50, LIU Zhiwei wrote: > > On 2023/2/17 5:55, Daniel Henrique Barboza wrote: >> We're going to do changes that requires accessing the RISCVCPUConfig >> struct from the RISCVCPU, having access only to a CPURISCVState 'env' >> pointer. Add a helper to make the code easier to read. >> >> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com> >> --- >> target/riscv/cpu.h | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h >> index 01803a020d..5e9626837b 100644 >> --- a/target/riscv/cpu.h >> +++ b/target/riscv/cpu.h >> @@ -653,6 +653,11 @@ static inline RISCVMXL >> riscv_cpu_mxl(CPURISCVState *env) >> #endif >> #define riscv_cpu_mxl_bits(env) (1UL << (4 + riscv_cpu_mxl(env))) >> +static inline const RISCVCPUConfig *riscv_cpu_cfg(CPURISCVState *env) > > Maybe we should > > static inline const* RISCVCPUConfig riscv_cpu_cfg(CPURISCVState *env) > or just > static inline RISCVCPUConfig *riscv_cpu_cfg(CPURISCVState *env) Ignore this comment. I see that you never change the fields from this pointer. Zhiwei > > Zhiwei > >> +{ >> + return &env_archcpu(env)->cfg; >> +} >> + >> #if defined(TARGET_RISCV32) >> #define cpu_recompute_xl(env) ((void)(env), MXL_RV32) >> #else
On 2023/2/17 05:55, Daniel Henrique Barboza wrote: > We're going to do changes that requires accessing the RISCVCPUConfig > struct from the RISCVCPU, having access only to a CPURISCVState 'env' > pointer. Add a helper to make the code easier to read. > > Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com> Reviewed-by: Weiwei Li <liweiwei@iscas.ac.cn> Regards, Weiwei Li > --- > target/riscv/cpu.h | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h > index 01803a020d..5e9626837b 100644 > --- a/target/riscv/cpu.h > +++ b/target/riscv/cpu.h > @@ -653,6 +653,11 @@ static inline RISCVMXL riscv_cpu_mxl(CPURISCVState *env) > #endif > #define riscv_cpu_mxl_bits(env) (1UL << (4 + riscv_cpu_mxl(env))) > > +static inline const RISCVCPUConfig *riscv_cpu_cfg(CPURISCVState *env) > +{ > + return &env_archcpu(env)->cfg; > +} > + > #if defined(TARGET_RISCV32) > #define cpu_recompute_xl(env) ((void)(env), MXL_RV32) > #else
On 2023/2/17 5:55, Daniel Henrique Barboza wrote: > We're going to do changes that requires accessing the RISCVCPUConfig > struct from the RISCVCPU, having access only to a CPURISCVState 'env' > pointer. Add a helper to make the code easier to read. > > Signed-off-by: Daniel Henrique Barboza<dbarboza@ventanamicro.com> > --- > target/riscv/cpu.h | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h > index 01803a020d..5e9626837b 100644 > --- a/target/riscv/cpu.h > +++ b/target/riscv/cpu.h > @@ -653,6 +653,11 @@ static inline RISCVMXL riscv_cpu_mxl(CPURISCVState *env) > #endif > #define riscv_cpu_mxl_bits(env) (1UL << (4 + riscv_cpu_mxl(env))) > > +static inline const RISCVCPUConfig *riscv_cpu_cfg(CPURISCVState *env) > +{ > + return &env_archcpu(env)->cfg; > +} > + There many places in branch should use this interface, not just in this patch set. For example, static RISCVException seed(CPURISCVState *env, int csrno) { RISCVCPU *cpu = env_archcpu(env); if (!cpu->cfg.ext_zkr) { return RISCV_EXCP_ILLEGAL_INST; } The cpu here will not be used, except referring to the cfg. Do you mind to unify the use? Zhiwei > #if defined(TARGET_RISCV32) > #define cpu_recompute_xl(env) ((void)(env), MXL_RV32) > #else
On 2/16/23 11:55, Daniel Henrique Barboza wrote: > We're going to do changes that requires accessing the RISCVCPUConfig > struct from the RISCVCPU, having access only to a CPURISCVState 'env' > pointer. Add a helper to make the code easier to read. > > Signed-off-by: Daniel Henrique Barboza<dbarboza@ventanamicro.com> > --- > target/riscv/cpu.h | 5 +++++ > 1 file changed, 5 insertions(+) Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
On 2/16/23 23:55, LIU Zhiwei wrote: > > On 2023/2/17 5:55, Daniel Henrique Barboza wrote: >> We're going to do changes that requires accessing the RISCVCPUConfig >> struct from the RISCVCPU, having access only to a CPURISCVState 'env' >> pointer. Add a helper to make the code easier to read. >> >> Signed-off-by: Daniel Henrique Barboza<dbarboza@ventanamicro.com> >> --- >> target/riscv/cpu.h | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h >> index 01803a020d..5e9626837b 100644 >> --- a/target/riscv/cpu.h >> +++ b/target/riscv/cpu.h >> @@ -653,6 +653,11 @@ static inline RISCVMXL riscv_cpu_mxl(CPURISCVState *env) >> #endif >> #define riscv_cpu_mxl_bits(env) (1UL << (4 + riscv_cpu_mxl(env))) >> >> +static inline const RISCVCPUConfig *riscv_cpu_cfg(CPURISCVState *env) >> +{ >> + return &env_archcpu(env)->cfg; >> +} >> + > > There many places in branch should use this interface, not just in this patch set. > > For example, > > static RISCVException seed(CPURISCVState *env, int csrno) > { > RISCVCPU *cpu = env_archcpu(env); > > if (!cpu->cfg.ext_zkr) { > return RISCV_EXCP_ILLEGAL_INST; > } > > The cpu here will not be used, except referring to the cfg. > > Do you mind to unify the use? Sure, I can look into that to make the cfg access more uniform across the code. I believe this would be too much to do in this series though. Let's do it as a follow-up. Thanks. Daniel > > Zhiwei > >> #if defined(TARGET_RISCV32) >> #define cpu_recompute_xl(env) ((void)(env), MXL_RV32) >> #else
diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h index 01803a020d..5e9626837b 100644 --- a/target/riscv/cpu.h +++ b/target/riscv/cpu.h @@ -653,6 +653,11 @@ static inline RISCVMXL riscv_cpu_mxl(CPURISCVState *env) #endif #define riscv_cpu_mxl_bits(env) (1UL << (4 + riscv_cpu_mxl(env))) +static inline const RISCVCPUConfig *riscv_cpu_cfg(CPURISCVState *env) +{ + return &env_archcpu(env)->cfg; +} + #if defined(TARGET_RISCV32) #define cpu_recompute_xl(env) ((void)(env), MXL_RV32) #else
We're going to do changes that requires accessing the RISCVCPUConfig struct from the RISCVCPU, having access only to a CPURISCVState 'env' pointer. Add a helper to make the code easier to read. Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com> --- target/riscv/cpu.h | 5 +++++ 1 file changed, 5 insertions(+)