Re: [PATCH V3] target/riscv: Align the data type of reset vector address

[Dylan Jhong]

On Fri, Mar 26, 2021 at 04:19:09AM +0800, Alistair Francis wrote:
> On Thu, Mar 25, 2021 at 5:43 AM Dylan Jhong <dylan@andestech.com> wrote:
> >
> > Signed-off-by: Dylan Jhong <dylan@andestech.com>
> > Signed-off-by: Ruinland ChuanTzu Tsai <ruinland@andestech.com>
> > ---
> >  target/riscv/cpu.c | 6 +++++-
> >  target/riscv/cpu.h | 2 +-
> >  2 files changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > index 7d6ed80f6b..8a5f18bcb0 100644
> > --- a/target/riscv/cpu.c
> > +++ b/target/riscv/cpu.c
> > @@ -137,7 +137,7 @@ static void set_feature(CPURISCVState *env, int feature)
> >      env->features |= (1ULL << feature);
> >  }
> >
> > -static void set_resetvec(CPURISCVState *env, int resetvec)
> > +static void set_resetvec(CPURISCVState *env, target_ulong resetvec)
> >  {
> >  #ifndef CONFIG_USER_ONLY
> >      env->resetvec = resetvec;
> > @@ -554,7 +554,11 @@ static Property riscv_cpu_properties[] = {
> >      DEFINE_PROP_UINT16("elen", RISCVCPU, cfg.elen, 64),
> >      DEFINE_PROP_BOOL("mmu", RISCVCPU, cfg.mmu, true),
> >      DEFINE_PROP_BOOL("pmp", RISCVCPU, cfg.pmp, true),
> > +#if defined(TARGET_RISCV32)
> > +    DEFINE_PROP_UINT32("resetvec", RISCVCPU, cfg.resetvec, DEFAULT_RSTVEC),
> > +#elif defined(TARGET_RISCV64)
> >      DEFINE_PROP_UINT64("resetvec", RISCVCPU, cfg.resetvec, DEFAULT_RSTVEC),
> > +#endif
> 
> Thanks for the patch!
> 
> I don't want to introduce any more define(TARGET_* macros as we are
> trying to make RISC-V QEMU xlen independent.
> 
> The hexagon port has an example of how you can use target_ulong here:
> 
>     DEFINE_PROP_UNSIGNED("lldb-stack-adjust", HexagonCPU, lldb_stack_adjust,
>                          0, qdev_prop_uint32, target_ulong);
> 
> can you do something like that instead?
> 
> Alistair
>

Hi Alistair,

Thanks for the comments.
But so far I did not see a way to satisfy both 32/64bit reset vector define.

The problem occurs in the 5th parameter of DEFINE_PROP_UNSIGNED(_name, _state, 
_field, _defval, _prop, _type).
We need to specify the _prop parameter to one of the PropertyInfo struct as 
shown below:

    extern const PropertyInfo qdev_prop_bit;
    extern const PropertyInfo qdev_prop_bit64;
    extern const PropertyInfo qdev_prop_bool;
    extern const PropertyInfo qdev_prop_enum;
    extern const PropertyInfo qdev_prop_uint8;
    extern const PropertyInfo qdev_prop_uint16;
    extern const PropertyInfo qdev_prop_uint32;
    extern const PropertyInfo qdev_prop_int32;
    extern const PropertyInfo qdev_prop_uint64;
    extern const PropertyInfo qdev_prop_int64;
    extern const PropertyInfo qdev_prop_size;
    extern const PropertyInfo qdev_prop_string;
    extern const PropertyInfo qdev_prop_on_off_auto;
    extern const PropertyInfo qdev_prop_size32;
    extern const PropertyInfo qdev_prop_arraylen;
    extern const PropertyInfo qdev_prop_link;

Currently, there is no structure like "qdev_prop_target_ulong".
So, we still need to use an if-else condition to determine the attributes of 
the 5th parameter.
Something like this:
    #if defined(TARGET_RISCV32)
        DEFINE_PROP_UNSIGNED("resetvec", RISCVCPU, cfg.resetvec, 
DEFAULT_RSTVEC, qdev_prop_uint32 target_ulong),
    #elif defined(TARGET_RISCV64)
        DEFINE_PROP_UNSIGNED("resetvec", RISCVCPU, cfg.resetvec, 
DEFAULT_RSTVEC, qdev_prop_uint64 target_ulong),
    #endif
I think this is not be what you meant.

The other architectures seem to ignore this, they just choose one of the 
attributes(qdev_prop_uint32/64) as their parameter.

So now we have 2 options:
1. Use "qdev_prop_uint64" as the 5th parameter
    DEFINE_PROP_UNSIGNED("resetvec", RISCVCPU, cfg.resetvec, DEFAULT_RSTVEC, 
qdev_prop_uint64 target_ulong),

2. Use if-else condition
    [patch v3]

Or if you have other opinions, please bring them up and discuss them together.

Thanks,
Dylan
 
> >      DEFINE_PROP_END_OF_LIST(),
> >  };
> >
> > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> > index 0a33d387ba..d9d7891666 100644
> > --- a/target/riscv/cpu.h
> > +++ b/target/riscv/cpu.h
> > @@ -303,7 +303,7 @@ struct RISCVCPU {
> >          uint16_t elen;
> >          bool mmu;
> >          bool pmp;
> > -        uint64_t resetvec;
> > +        target_ulong resetvec;
> >      } cfg;
> >  };
> >
> > --
> > 2.17.1
> >
> >