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

[Alistair Francis]

On Fri, Mar 26, 2021 at 7:11 AM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Fri, 26 Mar 2021 at 10:21, Dylan Jhong <dylan@andestech.com> wrote:
> > 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.
>
> I would recommend that you just use DEFINE_PROP_UINT64 for this property
> (and store the value in a uint64_t cfg.resetvec) regardless of whether the
> guest CPU happens to be 32 or 64 bit. In the case where it's 32 bits you
> can just ignore the top 32 bits (or if you're feeling enthusiastic, report
> an error if they're non-zero). This is simpler to code, avoids ifdefs,
> and tends to mean that most code doesn't need to care about the 32-vs-64
> difference.

That sounds good to me.

Alistair

>
> thanks
> -- PMM