[v3] target/riscv: Align the data type of reset vector address

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

Posted by Dylan Jhong 3 years ago

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
     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

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

Posted by Bin Meng 3 years ago

On Thu, Mar 25, 2021 at 5:42 PM 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(-)
>

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

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

Posted by Alistair Francis 3 years ago

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

>      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
>
>

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

Posted by Dylan Jhong 3 years ago

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
> >
> >

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

Posted by Peter Maydell 3 years ago

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.

thanks
-- PMM

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

Posted by Alistair Francis 3 years ago

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