Re: [PATCH v2 04/13] target/riscv: Replace riscv_cpu_is_32bit with riscv_cpu_mxl

[Richard Henderson]

On 10/14/21 12:08 AM, LIU Zhiwei wrote:
> On 2021/10/14 上午4:50, Richard Henderson wrote:
> > Shortly, the set of supported XL will not be just 32 and 64,
> > and representing that properly using the enumeration will be
> > imperative.
> >
> > Two places, booting and gdb, intentionally use misa_mxl_max
> > to emphasize the use of the reset value of misa.mxl, and not
> > the current cpu state.
> >
> > Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> > ---
> >   target/riscv/cpu.h            |  9 ++++++++-
> >   hw/riscv/boot.c               |  2 +-
> >   semihosting/arm-compat-semi.c |  2 +-
> >   target/riscv/cpu.c            | 24 ++++++++++++++----------
> >   target/riscv/cpu_helper.c     | 12 ++++++------
> >   target/riscv/csr.c            | 24 ++++++++++++------------
> >   target/riscv/gdbstub.c        |  2 +-
> >   target/riscv/monitor.c        |  4 ++--
> >   8 files changed, 45 insertions(+), 34 deletions(-)
> >
> > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> > index e708fcc168..87248b562a 100644
> > --- a/target/riscv/cpu.h
> > +++ b/target/riscv/cpu.h
> > @@ -396,7 +396,14 @@ FIELD(TB_FLAGS, VILL, 8, 1)
> >   FIELD(TB_FLAGS, HLSX, 9, 1)
> >   FIELD(TB_FLAGS, MSTATUS_HS_FS, 10, 2)
> > -bool riscv_cpu_is_32bit(CPURISCVState *env);
> > +#ifdef CONFIG_RISCV32
> > +#define riscv_cpu_mxl(env)      MXL_RV32
> > +#else
> > +static inline RISCVMXL riscv_cpu_mxl(CPURISCVState *env)
> > +{
> > +    return env->misa_mxl;
> > +}
> > +#endif
>
> Hi Richard,
>
> I don't know why we use CONFIG_RISCV32 here. I looked through the target source code. It 
> doesn't use this macro before.

Typo, should be TARGET_RISCV32.

But the reason to have the ifdef is so that riscv_cpu_mxl becomes a constant and the 
compiler can fold away more code.

> >   bool riscv_is_32bit(RISCVHartArrayState *harts)
> >   {
> > -    return riscv_cpu_is_32bit(&harts->harts[0].env);
> > +    return harts->harts[0].env.misa_mxl_max == MXL_RV32;
>
> Why not use  misa_mxl  here?  As this is just a replacement of riscv_cpu_is_32bit like 
> many other places.

It isn't clear to me that all uses of this are at boot time.  Once MXL may be changed at 
runtime, that (potentially) changes this test, and it seems unlikely that the board would 
really change based on the current status of the cpu.


r~