1. Patchew
  2. QEMU
  3. [PATCH v2 0/9] RISC-V Add the OpenTitan Machine
  4. View patch

[PATCH v2 2/9] target/riscv: Don't overwrite the reset vector

Alistair Francis posted 9 patches 5 years, 9 months ago
Maintainers: Paolo Bonzini <pbonzini@redhat.com>, Alistair Francis <Alistair.Francis@wdc.com>, Sagar Karandikar <sagark@eecs.berkeley.edu>, Bastian Koppelmann <kbastian@mail.uni-paderborn.de>, Palmer Dabbelt <palmer@dabbelt.com>, "Marc-André Lureau" <marcandre.lureau@redhat.com>
There is a newer version of this series
[PATCH v2 2/9] target/riscv: Don't overwrite the reset vector
Posted by Alistair Francis 5 years, 9 months ago 
If the reset vector is set in the init function don't set it again in
realise.

Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
---
 target/riscv/cpu.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 059d71f2c7..8f837edf8d 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -111,6 +111,14 @@ static void set_feature(CPURISCVState *env, int feature)
     env->features |= (1ULL << feature);
 }
 
+static int get_resetvec(CPURISCVState *env)
+{
+#ifndef CONFIG_USER_ONLY
+    return env->resetvec;
+#endif
+    return 0;
+}
+
 static void set_resetvec(CPURISCVState *env, int resetvec)
 {
 #ifndef CONFIG_USER_ONLY
@@ -123,7 +131,6 @@ static void riscv_any_cpu_init(Object *obj)
     CPURISCVState *env = &RISCV_CPU(obj)->env;
     set_misa(env, RVXLEN | RVI | RVM | RVA | RVF | RVD | RVC | RVU);
     set_priv_version(env, PRIV_VERSION_1_11_0);
-    set_resetvec(env, DEFAULT_RSTVEC);
 }
 
 #if defined(TARGET_RISCV32)
@@ -140,7 +147,6 @@ static void rv32gcsu_priv1_09_1_cpu_init(Object *obj)
     CPURISCVState *env = &RISCV_CPU(obj)->env;
     set_misa(env, RV32 | RVI | RVM | RVA | RVF | RVD | RVC | RVS | RVU);
     set_priv_version(env, PRIV_VERSION_1_09_1);
-    set_resetvec(env, DEFAULT_RSTVEC);
     set_feature(env, RISCV_FEATURE_MMU);
     set_feature(env, RISCV_FEATURE_PMP);
 }
@@ -150,7 +156,6 @@ static void rv32gcsu_priv1_10_0_cpu_init(Object *obj)
     CPURISCVState *env = &RISCV_CPU(obj)->env;
     set_misa(env, RV32 | RVI | RVM | RVA | RVF | RVD | RVC | RVS | RVU);
     set_priv_version(env, PRIV_VERSION_1_10_0);
-    set_resetvec(env, DEFAULT_RSTVEC);
     set_feature(env, RISCV_FEATURE_MMU);
     set_feature(env, RISCV_FEATURE_PMP);
 }
@@ -160,7 +165,6 @@ static void rv32imacu_nommu_cpu_init(Object *obj)
     CPURISCVState *env = &RISCV_CPU(obj)->env;
     set_misa(env, RV32 | RVI | RVM | RVA | RVC | RVU);
     set_priv_version(env, PRIV_VERSION_1_10_0);
-    set_resetvec(env, DEFAULT_RSTVEC);
     set_feature(env, RISCV_FEATURE_PMP);
 }
 
@@ -169,7 +173,6 @@ static void rv32imafcu_nommu_cpu_init(Object *obj)
     CPURISCVState *env = &RISCV_CPU(obj)->env;
     set_misa(env, RV32 | RVI | RVM | RVA | RVF | RVC | RVU);
     set_priv_version(env, PRIV_VERSION_1_10_0);
-    set_resetvec(env, DEFAULT_RSTVEC);
     set_feature(env, RISCV_FEATURE_PMP);
 }
 
@@ -187,7 +190,6 @@ static void rv64gcsu_priv1_09_1_cpu_init(Object *obj)
     CPURISCVState *env = &RISCV_CPU(obj)->env;
     set_misa(env, RV64 | RVI | RVM | RVA | RVF | RVD | RVC | RVS | RVU);
     set_priv_version(env, PRIV_VERSION_1_09_1);
-    set_resetvec(env, DEFAULT_RSTVEC);
     set_feature(env, RISCV_FEATURE_MMU);
     set_feature(env, RISCV_FEATURE_PMP);
 }
@@ -197,7 +199,6 @@ static void rv64gcsu_priv1_10_0_cpu_init(Object *obj)
     CPURISCVState *env = &RISCV_CPU(obj)->env;
     set_misa(env, RV64 | RVI | RVM | RVA | RVF | RVD | RVC | RVS | RVU);
     set_priv_version(env, PRIV_VERSION_1_10_0);
-    set_resetvec(env, DEFAULT_RSTVEC);
     set_feature(env, RISCV_FEATURE_MMU);
     set_feature(env, RISCV_FEATURE_PMP);
 }
@@ -207,7 +208,6 @@ static void rv64imacu_nommu_cpu_init(Object *obj)
     CPURISCVState *env = &RISCV_CPU(obj)->env;
     set_misa(env, RV64 | RVI | RVM | RVA | RVC | RVU);
     set_priv_version(env, PRIV_VERSION_1_10_0);
-    set_resetvec(env, DEFAULT_RSTVEC);
     set_feature(env, RISCV_FEATURE_PMP);
 }
 
@@ -399,7 +399,9 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
     }
 
     set_priv_version(env, priv_version);
-    set_resetvec(env, DEFAULT_RSTVEC);
+    if (!get_resetvec(env)) {
+        set_resetvec(env, DEFAULT_RSTVEC);
+    }
 
     if (cpu->cfg.mmu) {
         set_feature(env, RISCV_FEATURE_MMU);
-- 
2.26.2
Re: [PATCH v2 2/9] target/riscv: Don't overwrite the reset vector
Posted by Philippe Mathieu-Daudé 5 years, 9 months ago 
On 5/7/20 9:13 PM, Alistair Francis wrote:
> If the reset vector is set in the init function don't set it again in
> realise.

typo "realize".

> 
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
>   target/riscv/cpu.c | 20 +++++++++++---------
>   1 file changed, 11 insertions(+), 9 deletions(-)
> 
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 059d71f2c7..8f837edf8d 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -111,6 +111,14 @@ static void set_feature(CPURISCVState *env, int feature)
>       env->features |= (1ULL << feature);
>   }
>   
> +static int get_resetvec(CPURISCVState *env)
> +{
> +#ifndef CONFIG_USER_ONLY
> +    return env->resetvec;
> +#endif
> +    return 0;

Don't you get an error about double return? Maybe use #else?

> +}
> +
>   static void set_resetvec(CPURISCVState *env, int resetvec)
>   {
>   #ifndef CONFIG_USER_ONLY
> @@ -123,7 +131,6 @@ static void riscv_any_cpu_init(Object *obj)
>       CPURISCVState *env = &RISCV_CPU(obj)->env;
>       set_misa(env, RVXLEN | RVI | RVM | RVA | RVF | RVD | RVC | RVU);
>       set_priv_version(env, PRIV_VERSION_1_11_0);
> -    set_resetvec(env, DEFAULT_RSTVEC);
>   }
>   
>   #if defined(TARGET_RISCV32)
> @@ -140,7 +147,6 @@ static void rv32gcsu_priv1_09_1_cpu_init(Object *obj)
>       CPURISCVState *env = &RISCV_CPU(obj)->env;
>       set_misa(env, RV32 | RVI | RVM | RVA | RVF | RVD | RVC | RVS | RVU);
>       set_priv_version(env, PRIV_VERSION_1_09_1);
> -    set_resetvec(env, DEFAULT_RSTVEC);
>       set_feature(env, RISCV_FEATURE_MMU);
>       set_feature(env, RISCV_FEATURE_PMP);
>   }
> @@ -150,7 +156,6 @@ static void rv32gcsu_priv1_10_0_cpu_init(Object *obj)
>       CPURISCVState *env = &RISCV_CPU(obj)->env;
>       set_misa(env, RV32 | RVI | RVM | RVA | RVF | RVD | RVC | RVS | RVU);
>       set_priv_version(env, PRIV_VERSION_1_10_0);
> -    set_resetvec(env, DEFAULT_RSTVEC);
>       set_feature(env, RISCV_FEATURE_MMU);
>       set_feature(env, RISCV_FEATURE_PMP);
>   }
> @@ -160,7 +165,6 @@ static void rv32imacu_nommu_cpu_init(Object *obj)
>       CPURISCVState *env = &RISCV_CPU(obj)->env;
>       set_misa(env, RV32 | RVI | RVM | RVA | RVC | RVU);
>       set_priv_version(env, PRIV_VERSION_1_10_0);
> -    set_resetvec(env, DEFAULT_RSTVEC);
>       set_feature(env, RISCV_FEATURE_PMP);
>   }
>   
> @@ -169,7 +173,6 @@ static void rv32imafcu_nommu_cpu_init(Object *obj)
>       CPURISCVState *env = &RISCV_CPU(obj)->env;
>       set_misa(env, RV32 | RVI | RVM | RVA | RVF | RVC | RVU);
>       set_priv_version(env, PRIV_VERSION_1_10_0);
> -    set_resetvec(env, DEFAULT_RSTVEC);
>       set_feature(env, RISCV_FEATURE_PMP);
>   }
>   
> @@ -187,7 +190,6 @@ static void rv64gcsu_priv1_09_1_cpu_init(Object *obj)
>       CPURISCVState *env = &RISCV_CPU(obj)->env;
>       set_misa(env, RV64 | RVI | RVM | RVA | RVF | RVD | RVC | RVS | RVU);
>       set_priv_version(env, PRIV_VERSION_1_09_1);
> -    set_resetvec(env, DEFAULT_RSTVEC);
>       set_feature(env, RISCV_FEATURE_MMU);
>       set_feature(env, RISCV_FEATURE_PMP);
>   }
> @@ -197,7 +199,6 @@ static void rv64gcsu_priv1_10_0_cpu_init(Object *obj)
>       CPURISCVState *env = &RISCV_CPU(obj)->env;
>       set_misa(env, RV64 | RVI | RVM | RVA | RVF | RVD | RVC | RVS | RVU);
>       set_priv_version(env, PRIV_VERSION_1_10_0);
> -    set_resetvec(env, DEFAULT_RSTVEC);
>       set_feature(env, RISCV_FEATURE_MMU);
>       set_feature(env, RISCV_FEATURE_PMP);
>   }
> @@ -207,7 +208,6 @@ static void rv64imacu_nommu_cpu_init(Object *obj)
>       CPURISCVState *env = &RISCV_CPU(obj)->env;
>       set_misa(env, RV64 | RVI | RVM | RVA | RVC | RVU);
>       set_priv_version(env, PRIV_VERSION_1_10_0);
> -    set_resetvec(env, DEFAULT_RSTVEC);
>       set_feature(env, RISCV_FEATURE_PMP);
>   }
>   
> @@ -399,7 +399,9 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
>       }
>   
>       set_priv_version(env, priv_version);
> -    set_resetvec(env, DEFAULT_RSTVEC);
> +    if (!get_resetvec(env)) {
> +        set_resetvec(env, DEFAULT_RSTVEC);
> +    }
>   
>       if (cpu->cfg.mmu) {
>           set_feature(env, RISCV_FEATURE_MMU);
>
Re: [PATCH v2 2/9] target/riscv: Don't overwrite the reset vector
Posted by Alistair Francis 5 years, 9 months ago 
On Thu, May 14, 2020 at 10:54 AM Philippe Mathieu-Daudé
<philmd@redhat.com> wrote:
>
> On 5/7/20 9:13 PM, Alistair Francis wrote:
> > If the reset vector is set in the init function don't set it again in
> > realise.
>
> typo "realize".

It's not a typo, just correct English :)

I have changed it.

>
> >
> > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> > ---
> >   target/riscv/cpu.c | 20 +++++++++++---------
> >   1 file changed, 11 insertions(+), 9 deletions(-)
> >
> > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > index 059d71f2c7..8f837edf8d 100644
> > --- a/target/riscv/cpu.c
> > +++ b/target/riscv/cpu.c
> > @@ -111,6 +111,14 @@ static void set_feature(CPURISCVState *env, int feature)
> >       env->features |= (1ULL << feature);
> >   }
> >
> > +static int get_resetvec(CPURISCVState *env)
> > +{
> > +#ifndef CONFIG_USER_ONLY
> > +    return env->resetvec;
> > +#endif
> > +    return 0;
>
> Don't you get an error about double return? Maybe use #else?

Apparently not, I have changed it though.

Alistair

>
> > +}
> > +
> >   static void set_resetvec(CPURISCVState *env, int resetvec)
> >   {
> >   #ifndef CONFIG_USER_ONLY
> > @@ -123,7 +131,6 @@ static void riscv_any_cpu_init(Object *obj)
> >       CPURISCVState *env = &RISCV_CPU(obj)->env;
> >       set_misa(env, RVXLEN | RVI | RVM | RVA | RVF | RVD | RVC | RVU);
> >       set_priv_version(env, PRIV_VERSION_1_11_0);
> > -    set_resetvec(env, DEFAULT_RSTVEC);
> >   }
> >
> >   #if defined(TARGET_RISCV32)
> > @@ -140,7 +147,6 @@ static void rv32gcsu_priv1_09_1_cpu_init(Object *obj)
> >       CPURISCVState *env = &RISCV_CPU(obj)->env;
> >       set_misa(env, RV32 | RVI | RVM | RVA | RVF | RVD | RVC | RVS | RVU);
> >       set_priv_version(env, PRIV_VERSION_1_09_1);
> > -    set_resetvec(env, DEFAULT_RSTVEC);
> >       set_feature(env, RISCV_FEATURE_MMU);
> >       set_feature(env, RISCV_FEATURE_PMP);
> >   }
> > @@ -150,7 +156,6 @@ static void rv32gcsu_priv1_10_0_cpu_init(Object *obj)
> >       CPURISCVState *env = &RISCV_CPU(obj)->env;
> >       set_misa(env, RV32 | RVI | RVM | RVA | RVF | RVD | RVC | RVS | RVU);
> >       set_priv_version(env, PRIV_VERSION_1_10_0);
> > -    set_resetvec(env, DEFAULT_RSTVEC);
> >       set_feature(env, RISCV_FEATURE_MMU);
> >       set_feature(env, RISCV_FEATURE_PMP);
> >   }
> > @@ -160,7 +165,6 @@ static void rv32imacu_nommu_cpu_init(Object *obj)
> >       CPURISCVState *env = &RISCV_CPU(obj)->env;
> >       set_misa(env, RV32 | RVI | RVM | RVA | RVC | RVU);
> >       set_priv_version(env, PRIV_VERSION_1_10_0);
> > -    set_resetvec(env, DEFAULT_RSTVEC);
> >       set_feature(env, RISCV_FEATURE_PMP);
> >   }
> >
> > @@ -169,7 +173,6 @@ static void rv32imafcu_nommu_cpu_init(Object *obj)
> >       CPURISCVState *env = &RISCV_CPU(obj)->env;
> >       set_misa(env, RV32 | RVI | RVM | RVA | RVF | RVC | RVU);
> >       set_priv_version(env, PRIV_VERSION_1_10_0);
> > -    set_resetvec(env, DEFAULT_RSTVEC);
> >       set_feature(env, RISCV_FEATURE_PMP);
> >   }
> >
> > @@ -187,7 +190,6 @@ static void rv64gcsu_priv1_09_1_cpu_init(Object *obj)
> >       CPURISCVState *env = &RISCV_CPU(obj)->env;
> >       set_misa(env, RV64 | RVI | RVM | RVA | RVF | RVD | RVC | RVS | RVU);
> >       set_priv_version(env, PRIV_VERSION_1_09_1);
> > -    set_resetvec(env, DEFAULT_RSTVEC);
> >       set_feature(env, RISCV_FEATURE_MMU);
> >       set_feature(env, RISCV_FEATURE_PMP);
> >   }
> > @@ -197,7 +199,6 @@ static void rv64gcsu_priv1_10_0_cpu_init(Object *obj)
> >       CPURISCVState *env = &RISCV_CPU(obj)->env;
> >       set_misa(env, RV64 | RVI | RVM | RVA | RVF | RVD | RVC | RVS | RVU);
> >       set_priv_version(env, PRIV_VERSION_1_10_0);
> > -    set_resetvec(env, DEFAULT_RSTVEC);
> >       set_feature(env, RISCV_FEATURE_MMU);
> >       set_feature(env, RISCV_FEATURE_PMP);
> >   }
> > @@ -207,7 +208,6 @@ static void rv64imacu_nommu_cpu_init(Object *obj)
> >       CPURISCVState *env = &RISCV_CPU(obj)->env;
> >       set_misa(env, RV64 | RVI | RVM | RVA | RVC | RVU);
> >       set_priv_version(env, PRIV_VERSION_1_10_0);
> > -    set_resetvec(env, DEFAULT_RSTVEC);
> >       set_feature(env, RISCV_FEATURE_PMP);
> >   }
> >
> > @@ -399,7 +399,9 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
> >       }
> >
> >       set_priv_version(env, priv_version);
> > -    set_resetvec(env, DEFAULT_RSTVEC);
> > +    if (!get_resetvec(env)) {
> > +        set_resetvec(env, DEFAULT_RSTVEC);
> > +    }
> >
> >       if (cpu->cfg.mmu) {
> >           set_feature(env, RISCV_FEATURE_MMU);
> >
>
Re: [PATCH v2 2/9] target/riscv: Don't overwrite the reset vector
Posted by Bin Meng 5 years, 9 months ago 
On Fri, May 15, 2020 at 5:51 AM Alistair Francis <alistair23@gmail.com> wrote:
>
> On Thu, May 14, 2020 at 10:54 AM Philippe Mathieu-Daudé
> <philmd@redhat.com> wrote:
> >
> > On 5/7/20 9:13 PM, Alistair Francis wrote:
> > > If the reset vector is set in the init function don't set it again in
> > > realise.
> >
> > typo "realize".
>
> It's not a typo, just correct English :)
>
> I have changed it.
>
> >
> > >
> > > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> > > ---
> > >   target/riscv/cpu.c | 20 +++++++++++---------
> > >   1 file changed, 11 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > > index 059d71f2c7..8f837edf8d 100644
> > > --- a/target/riscv/cpu.c
> > > +++ b/target/riscv/cpu.c
> > > @@ -111,6 +111,14 @@ static void set_feature(CPURISCVState *env, int feature)
> > >       env->features |= (1ULL << feature);
> > >   }
> > >
> > > +static int get_resetvec(CPURISCVState *env)
> > > +{
> > > +#ifndef CONFIG_USER_ONLY
> > > +    return env->resetvec;
> > > +#endif
> > > +    return 0;
> >
> > Don't you get an error about double return? Maybe use #else?
>
> Apparently not, I have changed it though.
>
> Alistair
>
> >
> > > +}
> > > +
> > >   static void set_resetvec(CPURISCVState *env, int resetvec)
> > >   {
> > >   #ifndef CONFIG_USER_ONLY
> > > @@ -123,7 +131,6 @@ static void riscv_any_cpu_init(Object *obj)
> > >       CPURISCVState *env = &RISCV_CPU(obj)->env;
> > >       set_misa(env, RVXLEN | RVI | RVM | RVA | RVF | RVD | RVC | RVU);
> > >       set_priv_version(env, PRIV_VERSION_1_11_0);
> > > -    set_resetvec(env, DEFAULT_RSTVEC);
> > >   }
> > >
> > >   #if defined(TARGET_RISCV32)
> > > @@ -140,7 +147,6 @@ static void rv32gcsu_priv1_09_1_cpu_init(Object *obj)
> > >       CPURISCVState *env = &RISCV_CPU(obj)->env;
> > >       set_misa(env, RV32 | RVI | RVM | RVA | RVF | RVD | RVC | RVS | RVU);
> > >       set_priv_version(env, PRIV_VERSION_1_09_1);
> > > -    set_resetvec(env, DEFAULT_RSTVEC);
> > >       set_feature(env, RISCV_FEATURE_MMU);
> > >       set_feature(env, RISCV_FEATURE_PMP);
> > >   }
> > > @@ -150,7 +156,6 @@ static void rv32gcsu_priv1_10_0_cpu_init(Object *obj)
> > >       CPURISCVState *env = &RISCV_CPU(obj)->env;
> > >       set_misa(env, RV32 | RVI | RVM | RVA | RVF | RVD | RVC | RVS | RVU);
> > >       set_priv_version(env, PRIV_VERSION_1_10_0);
> > > -    set_resetvec(env, DEFAULT_RSTVEC);
> > >       set_feature(env, RISCV_FEATURE_MMU);
> > >       set_feature(env, RISCV_FEATURE_PMP);
> > >   }
> > > @@ -160,7 +165,6 @@ static void rv32imacu_nommu_cpu_init(Object *obj)
> > >       CPURISCVState *env = &RISCV_CPU(obj)->env;
> > >       set_misa(env, RV32 | RVI | RVM | RVA | RVC | RVU);
> > >       set_priv_version(env, PRIV_VERSION_1_10_0);
> > > -    set_resetvec(env, DEFAULT_RSTVEC);
> > >       set_feature(env, RISCV_FEATURE_PMP);
> > >   }
> > >
> > > @@ -169,7 +173,6 @@ static void rv32imafcu_nommu_cpu_init(Object *obj)
> > >       CPURISCVState *env = &RISCV_CPU(obj)->env;
> > >       set_misa(env, RV32 | RVI | RVM | RVA | RVF | RVC | RVU);
> > >       set_priv_version(env, PRIV_VERSION_1_10_0);
> > > -    set_resetvec(env, DEFAULT_RSTVEC);
> > >       set_feature(env, RISCV_FEATURE_PMP);
> > >   }
> > >
> > > @@ -187,7 +190,6 @@ static void rv64gcsu_priv1_09_1_cpu_init(Object *obj)
> > >       CPURISCVState *env = &RISCV_CPU(obj)->env;
> > >       set_misa(env, RV64 | RVI | RVM | RVA | RVF | RVD | RVC | RVS | RVU);
> > >       set_priv_version(env, PRIV_VERSION_1_09_1);
> > > -    set_resetvec(env, DEFAULT_RSTVEC);
> > >       set_feature(env, RISCV_FEATURE_MMU);
> > >       set_feature(env, RISCV_FEATURE_PMP);
> > >   }
> > > @@ -197,7 +199,6 @@ static void rv64gcsu_priv1_10_0_cpu_init(Object *obj)
> > >       CPURISCVState *env = &RISCV_CPU(obj)->env;
> > >       set_misa(env, RV64 | RVI | RVM | RVA | RVF | RVD | RVC | RVS | RVU);
> > >       set_priv_version(env, PRIV_VERSION_1_10_0);
> > > -    set_resetvec(env, DEFAULT_RSTVEC);
> > >       set_feature(env, RISCV_FEATURE_MMU);
> > >       set_feature(env, RISCV_FEATURE_PMP);
> > >   }
> > > @@ -207,7 +208,6 @@ static void rv64imacu_nommu_cpu_init(Object *obj)
> > >       CPURISCVState *env = &RISCV_CPU(obj)->env;
> > >       set_misa(env, RV64 | RVI | RVM | RVA | RVC | RVU);
> > >       set_priv_version(env, PRIV_VERSION_1_10_0);
> > > -    set_resetvec(env, DEFAULT_RSTVEC);
> > >       set_feature(env, RISCV_FEATURE_PMP);
> > >   }
> > >
> > > @@ -399,7 +399,9 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
> > >       }
> > >
> > >       set_priv_version(env, priv_version);
> > > -    set_resetvec(env, DEFAULT_RSTVEC);
> > > +    if (!get_resetvec(env)) {

What if we have a RISC-V CPU whose reset vector is at address 0?

> > > +        set_resetvec(env, DEFAULT_RSTVEC);
> > > +    }
> > >
> > >       if (cpu->cfg.mmu) {
> > >           set_feature(env, RISCV_FEATURE_MMU);
> > >

Regards,
Bin
Re: [PATCH v2 2/9] target/riscv: Don't overwrite the reset vector
Posted by Alistair Francis 5 years, 9 months ago 
On Thu, May 14, 2020 at 9:54 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> On Fri, May 15, 2020 at 5:51 AM Alistair Francis <alistair23@gmail.com> wrote:
> >
> > On Thu, May 14, 2020 at 10:54 AM Philippe Mathieu-Daudé
> > <philmd@redhat.com> wrote:
> > >
> > > On 5/7/20 9:13 PM, Alistair Francis wrote:
> > > > If the reset vector is set in the init function don't set it again in
> > > > realise.
> > >
> > > typo "realize".
> >
> > It's not a typo, just correct English :)
> >
> > I have changed it.
> >
> > >
> > > >
> > > > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> > > > ---
> > > >   target/riscv/cpu.c | 20 +++++++++++---------
> > > >   1 file changed, 11 insertions(+), 9 deletions(-)
> > > >
> > > > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > > > index 059d71f2c7..8f837edf8d 100644
> > > > --- a/target/riscv/cpu.c
> > > > +++ b/target/riscv/cpu.c
> > > > @@ -111,6 +111,14 @@ static void set_feature(CPURISCVState *env, int feature)
> > > >       env->features |= (1ULL << feature);
> > > >   }
> > > >
> > > > +static int get_resetvec(CPURISCVState *env)
> > > > +{
> > > > +#ifndef CONFIG_USER_ONLY
> > > > +    return env->resetvec;
> > > > +#endif
> > > > +    return 0;
> > >
> > > Don't you get an error about double return? Maybe use #else?
> >
> > Apparently not, I have changed it though.
> >
> > Alistair
> >
> > >
> > > > +}
> > > > +
> > > >   static void set_resetvec(CPURISCVState *env, int resetvec)
> > > >   {
> > > >   #ifndef CONFIG_USER_ONLY
> > > > @@ -123,7 +131,6 @@ static void riscv_any_cpu_init(Object *obj)
> > > >       CPURISCVState *env = &RISCV_CPU(obj)->env;
> > > >       set_misa(env, RVXLEN | RVI | RVM | RVA | RVF | RVD | RVC | RVU);
> > > >       set_priv_version(env, PRIV_VERSION_1_11_0);
> > > > -    set_resetvec(env, DEFAULT_RSTVEC);
> > > >   }
> > > >
> > > >   #if defined(TARGET_RISCV32)
> > > > @@ -140,7 +147,6 @@ static void rv32gcsu_priv1_09_1_cpu_init(Object *obj)
> > > >       CPURISCVState *env = &RISCV_CPU(obj)->env;
> > > >       set_misa(env, RV32 | RVI | RVM | RVA | RVF | RVD | RVC | RVS | RVU);
> > > >       set_priv_version(env, PRIV_VERSION_1_09_1);
> > > > -    set_resetvec(env, DEFAULT_RSTVEC);
> > > >       set_feature(env, RISCV_FEATURE_MMU);
> > > >       set_feature(env, RISCV_FEATURE_PMP);
> > > >   }
> > > > @@ -150,7 +156,6 @@ static void rv32gcsu_priv1_10_0_cpu_init(Object *obj)
> > > >       CPURISCVState *env = &RISCV_CPU(obj)->env;
> > > >       set_misa(env, RV32 | RVI | RVM | RVA | RVF | RVD | RVC | RVS | RVU);
> > > >       set_priv_version(env, PRIV_VERSION_1_10_0);
> > > > -    set_resetvec(env, DEFAULT_RSTVEC);
> > > >       set_feature(env, RISCV_FEATURE_MMU);
> > > >       set_feature(env, RISCV_FEATURE_PMP);
> > > >   }
> > > > @@ -160,7 +165,6 @@ static void rv32imacu_nommu_cpu_init(Object *obj)
> > > >       CPURISCVState *env = &RISCV_CPU(obj)->env;
> > > >       set_misa(env, RV32 | RVI | RVM | RVA | RVC | RVU);
> > > >       set_priv_version(env, PRIV_VERSION_1_10_0);
> > > > -    set_resetvec(env, DEFAULT_RSTVEC);
> > > >       set_feature(env, RISCV_FEATURE_PMP);
> > > >   }
> > > >
> > > > @@ -169,7 +173,6 @@ static void rv32imafcu_nommu_cpu_init(Object *obj)
> > > >       CPURISCVState *env = &RISCV_CPU(obj)->env;
> > > >       set_misa(env, RV32 | RVI | RVM | RVA | RVF | RVC | RVU);
> > > >       set_priv_version(env, PRIV_VERSION_1_10_0);
> > > > -    set_resetvec(env, DEFAULT_RSTVEC);
> > > >       set_feature(env, RISCV_FEATURE_PMP);
> > > >   }
> > > >
> > > > @@ -187,7 +190,6 @@ static void rv64gcsu_priv1_09_1_cpu_init(Object *obj)
> > > >       CPURISCVState *env = &RISCV_CPU(obj)->env;
> > > >       set_misa(env, RV64 | RVI | RVM | RVA | RVF | RVD | RVC | RVS | RVU);
> > > >       set_priv_version(env, PRIV_VERSION_1_09_1);
> > > > -    set_resetvec(env, DEFAULT_RSTVEC);
> > > >       set_feature(env, RISCV_FEATURE_MMU);
> > > >       set_feature(env, RISCV_FEATURE_PMP);
> > > >   }
> > > > @@ -197,7 +199,6 @@ static void rv64gcsu_priv1_10_0_cpu_init(Object *obj)
> > > >       CPURISCVState *env = &RISCV_CPU(obj)->env;
> > > >       set_misa(env, RV64 | RVI | RVM | RVA | RVF | RVD | RVC | RVS | RVU);
> > > >       set_priv_version(env, PRIV_VERSION_1_10_0);
> > > > -    set_resetvec(env, DEFAULT_RSTVEC);
> > > >       set_feature(env, RISCV_FEATURE_MMU);
> > > >       set_feature(env, RISCV_FEATURE_PMP);
> > > >   }
> > > > @@ -207,7 +208,6 @@ static void rv64imacu_nommu_cpu_init(Object *obj)
> > > >       CPURISCVState *env = &RISCV_CPU(obj)->env;
> > > >       set_misa(env, RV64 | RVI | RVM | RVA | RVC | RVU);
> > > >       set_priv_version(env, PRIV_VERSION_1_10_0);
> > > > -    set_resetvec(env, DEFAULT_RSTVEC);
> > > >       set_feature(env, RISCV_FEATURE_PMP);
> > > >   }
> > > >
> > > > @@ -399,7 +399,9 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
> > > >       }
> > > >
> > > >       set_priv_version(env, priv_version);
> > > > -    set_resetvec(env, DEFAULT_RSTVEC);
> > > > +    if (!get_resetvec(env)) {
>
> What if we have a RISC-V CPU whose reset vector is at address 0?

That won't work then. I think if that happens we could swap to a
negative number.

Alistair

>
> > > > +        set_resetvec(env, DEFAULT_RSTVEC);
> > > > +    }
> > > >
> > > >       if (cpu->cfg.mmu) {
> > > >           set_feature(env, RISCV_FEATURE_MMU);
> > > >
>
> Regards,
> Bin
Re: [PATCH v2 2/9] target/riscv: Don't overwrite the reset vector
Posted by Bin Meng 5 years, 8 months ago 
On Sat, May 16, 2020 at 3:51 AM Alistair Francis <alistair23@gmail.com> wrote:
>
> On Thu, May 14, 2020 at 9:54 PM Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > On Fri, May 15, 2020 at 5:51 AM Alistair Francis <alistair23@gmail.com> wrote:
> > >
> > > On Thu, May 14, 2020 at 10:54 AM Philippe Mathieu-Daudé
> > > <philmd@redhat.com> wrote:
> > > >
> > > > On 5/7/20 9:13 PM, Alistair Francis wrote:
> > > > > If the reset vector is set in the init function don't set it again in
> > > > > realise.
> > > >
> > > > typo "realize".
> > >
> > > It's not a typo, just correct English :)
> > >
> > > I have changed it.
> > >
> > > >
> > > > >
> > > > > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> > > > > ---
> > > > >   target/riscv/cpu.c | 20 +++++++++++---------
> > > > >   1 file changed, 11 insertions(+), 9 deletions(-)
> > > > >
> > > > > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > > > > index 059d71f2c7..8f837edf8d 100644
> > > > > --- a/target/riscv/cpu.c
> > > > > +++ b/target/riscv/cpu.c
> > > > > @@ -111,6 +111,14 @@ static void set_feature(CPURISCVState *env, int feature)
> > > > >       env->features |= (1ULL << feature);
> > > > >   }
> > > > >
> > > > > +static int get_resetvec(CPURISCVState *env)
> > > > > +{
> > > > > +#ifndef CONFIG_USER_ONLY
> > > > > +    return env->resetvec;
> > > > > +#endif
> > > > > +    return 0;
> > > >
> > > > Don't you get an error about double return? Maybe use #else?
> > >
> > > Apparently not, I have changed it though.
> > >
> > > Alistair
> > >
> > > >
> > > > > +}
> > > > > +
> > > > >   static void set_resetvec(CPURISCVState *env, int resetvec)
> > > > >   {
> > > > >   #ifndef CONFIG_USER_ONLY
> > > > > @@ -123,7 +131,6 @@ static void riscv_any_cpu_init(Object *obj)
> > > > >       CPURISCVState *env = &RISCV_CPU(obj)->env;
> > > > >       set_misa(env, RVXLEN | RVI | RVM | RVA | RVF | RVD | RVC | RVU);
> > > > >       set_priv_version(env, PRIV_VERSION_1_11_0);
> > > > > -    set_resetvec(env, DEFAULT_RSTVEC);
> > > > >   }
> > > > >
> > > > >   #if defined(TARGET_RISCV32)
> > > > > @@ -140,7 +147,6 @@ static void rv32gcsu_priv1_09_1_cpu_init(Object *obj)
> > > > >       CPURISCVState *env = &RISCV_CPU(obj)->env;
> > > > >       set_misa(env, RV32 | RVI | RVM | RVA | RVF | RVD | RVC | RVS | RVU);
> > > > >       set_priv_version(env, PRIV_VERSION_1_09_1);
> > > > > -    set_resetvec(env, DEFAULT_RSTVEC);
> > > > >       set_feature(env, RISCV_FEATURE_MMU);
> > > > >       set_feature(env, RISCV_FEATURE_PMP);
> > > > >   }
> > > > > @@ -150,7 +156,6 @@ static void rv32gcsu_priv1_10_0_cpu_init(Object *obj)
> > > > >       CPURISCVState *env = &RISCV_CPU(obj)->env;
> > > > >       set_misa(env, RV32 | RVI | RVM | RVA | RVF | RVD | RVC | RVS | RVU);
> > > > >       set_priv_version(env, PRIV_VERSION_1_10_0);
> > > > > -    set_resetvec(env, DEFAULT_RSTVEC);
> > > > >       set_feature(env, RISCV_FEATURE_MMU);
> > > > >       set_feature(env, RISCV_FEATURE_PMP);
> > > > >   }
> > > > > @@ -160,7 +165,6 @@ static void rv32imacu_nommu_cpu_init(Object *obj)
> > > > >       CPURISCVState *env = &RISCV_CPU(obj)->env;
> > > > >       set_misa(env, RV32 | RVI | RVM | RVA | RVC | RVU);
> > > > >       set_priv_version(env, PRIV_VERSION_1_10_0);
> > > > > -    set_resetvec(env, DEFAULT_RSTVEC);
> > > > >       set_feature(env, RISCV_FEATURE_PMP);
> > > > >   }
> > > > >
> > > > > @@ -169,7 +173,6 @@ static void rv32imafcu_nommu_cpu_init(Object *obj)
> > > > >       CPURISCVState *env = &RISCV_CPU(obj)->env;
> > > > >       set_misa(env, RV32 | RVI | RVM | RVA | RVF | RVC | RVU);
> > > > >       set_priv_version(env, PRIV_VERSION_1_10_0);
> > > > > -    set_resetvec(env, DEFAULT_RSTVEC);
> > > > >       set_feature(env, RISCV_FEATURE_PMP);
> > > > >   }
> > > > >
> > > > > @@ -187,7 +190,6 @@ static void rv64gcsu_priv1_09_1_cpu_init(Object *obj)
> > > > >       CPURISCVState *env = &RISCV_CPU(obj)->env;
> > > > >       set_misa(env, RV64 | RVI | RVM | RVA | RVF | RVD | RVC | RVS | RVU);
> > > > >       set_priv_version(env, PRIV_VERSION_1_09_1);
> > > > > -    set_resetvec(env, DEFAULT_RSTVEC);
> > > > >       set_feature(env, RISCV_FEATURE_MMU);
> > > > >       set_feature(env, RISCV_FEATURE_PMP);
> > > > >   }
> > > > > @@ -197,7 +199,6 @@ static void rv64gcsu_priv1_10_0_cpu_init(Object *obj)
> > > > >       CPURISCVState *env = &RISCV_CPU(obj)->env;
> > > > >       set_misa(env, RV64 | RVI | RVM | RVA | RVF | RVD | RVC | RVS | RVU);
> > > > >       set_priv_version(env, PRIV_VERSION_1_10_0);
> > > > > -    set_resetvec(env, DEFAULT_RSTVEC);
> > > > >       set_feature(env, RISCV_FEATURE_MMU);
> > > > >       set_feature(env, RISCV_FEATURE_PMP);
> > > > >   }
> > > > > @@ -207,7 +208,6 @@ static void rv64imacu_nommu_cpu_init(Object *obj)
> > > > >       CPURISCVState *env = &RISCV_CPU(obj)->env;
> > > > >       set_misa(env, RV64 | RVI | RVM | RVA | RVC | RVU);
> > > > >       set_priv_version(env, PRIV_VERSION_1_10_0);
> > > > > -    set_resetvec(env, DEFAULT_RSTVEC);
> > > > >       set_feature(env, RISCV_FEATURE_PMP);
> > > > >   }
> > > > >
> > > > > @@ -399,7 +399,9 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
> > > > >       }
> > > > >
> > > > >       set_priv_version(env, priv_version);
> > > > > -    set_resetvec(env, DEFAULT_RSTVEC);
> > > > > +    if (!get_resetvec(env)) {
> >
> > What if we have a RISC-V CPU whose reset vector is at address 0?
>
> That won't work then. I think if that happens we could swap to a
> negative number.
>

env->resetvec is declared as target_ulong so negative number does not work here.

How about just remove set_resetvec() in riscv_cpu_realize()? Or
introduce a new config parameter for the reset vector, and only
override it if the config parameter is given, like other properties?

Regards,
Bin
Re: [PATCH v2 2/9] target/riscv: Don't overwrite the reset vector
Posted by Alistair Francis 5 years, 8 months ago 
On Sat, May 16, 2020 at 2:03 AM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> On Sat, May 16, 2020 at 3:51 AM Alistair Francis <alistair23@gmail.com> wrote:
> >
> > On Thu, May 14, 2020 at 9:54 PM Bin Meng <bmeng.cn@gmail.com> wrote:
> > >
> > > On Fri, May 15, 2020 at 5:51 AM Alistair Francis <alistair23@gmail.com> wrote:
> > > >
> > > > On Thu, May 14, 2020 at 10:54 AM Philippe Mathieu-Daudé
> > > > <philmd@redhat.com> wrote:
> > > > >
> > > > > On 5/7/20 9:13 PM, Alistair Francis wrote:
> > > > > > If the reset vector is set in the init function don't set it again in
> > > > > > realise.
> > > > >
> > > > > typo "realize".
> > > >
> > > > It's not a typo, just correct English :)
> > > >
> > > > I have changed it.
> > > >
> > > > >
> > > > > >
> > > > > > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> > > > > > ---
> > > > > >   target/riscv/cpu.c | 20 +++++++++++---------
> > > > > >   1 file changed, 11 insertions(+), 9 deletions(-)
> > > > > >
> > > > > > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > > > > > index 059d71f2c7..8f837edf8d 100644
> > > > > > --- a/target/riscv/cpu.c
> > > > > > +++ b/target/riscv/cpu.c
> > > > > > @@ -111,6 +111,14 @@ static void set_feature(CPURISCVState *env, int feature)
> > > > > >       env->features |= (1ULL << feature);
> > > > > >   }
> > > > > >
> > > > > > +static int get_resetvec(CPURISCVState *env)
> > > > > > +{
> > > > > > +#ifndef CONFIG_USER_ONLY
> > > > > > +    return env->resetvec;
> > > > > > +#endif
> > > > > > +    return 0;
> > > > >
> > > > > Don't you get an error about double return? Maybe use #else?
> > > >
> > > > Apparently not, I have changed it though.
> > > >
> > > > Alistair
> > > >
> > > > >
> > > > > > +}
> > > > > > +
> > > > > >   static void set_resetvec(CPURISCVState *env, int resetvec)
> > > > > >   {
> > > > > >   #ifndef CONFIG_USER_ONLY
> > > > > > @@ -123,7 +131,6 @@ static void riscv_any_cpu_init(Object *obj)
> > > > > >       CPURISCVState *env = &RISCV_CPU(obj)->env;
> > > > > >       set_misa(env, RVXLEN | RVI | RVM | RVA | RVF | RVD | RVC | RVU);
> > > > > >       set_priv_version(env, PRIV_VERSION_1_11_0);
> > > > > > -    set_resetvec(env, DEFAULT_RSTVEC);
> > > > > >   }
> > > > > >
> > > > > >   #if defined(TARGET_RISCV32)
> > > > > > @@ -140,7 +147,6 @@ static void rv32gcsu_priv1_09_1_cpu_init(Object *obj)
> > > > > >       CPURISCVState *env = &RISCV_CPU(obj)->env;
> > > > > >       set_misa(env, RV32 | RVI | RVM | RVA | RVF | RVD | RVC | RVS | RVU);
> > > > > >       set_priv_version(env, PRIV_VERSION_1_09_1);
> > > > > > -    set_resetvec(env, DEFAULT_RSTVEC);
> > > > > >       set_feature(env, RISCV_FEATURE_MMU);
> > > > > >       set_feature(env, RISCV_FEATURE_PMP);
> > > > > >   }
> > > > > > @@ -150,7 +156,6 @@ static void rv32gcsu_priv1_10_0_cpu_init(Object *obj)
> > > > > >       CPURISCVState *env = &RISCV_CPU(obj)->env;
> > > > > >       set_misa(env, RV32 | RVI | RVM | RVA | RVF | RVD | RVC | RVS | RVU);
> > > > > >       set_priv_version(env, PRIV_VERSION_1_10_0);
> > > > > > -    set_resetvec(env, DEFAULT_RSTVEC);
> > > > > >       set_feature(env, RISCV_FEATURE_MMU);
> > > > > >       set_feature(env, RISCV_FEATURE_PMP);
> > > > > >   }
> > > > > > @@ -160,7 +165,6 @@ static void rv32imacu_nommu_cpu_init(Object *obj)
> > > > > >       CPURISCVState *env = &RISCV_CPU(obj)->env;
> > > > > >       set_misa(env, RV32 | RVI | RVM | RVA | RVC | RVU);
> > > > > >       set_priv_version(env, PRIV_VERSION_1_10_0);
> > > > > > -    set_resetvec(env, DEFAULT_RSTVEC);
> > > > > >       set_feature(env, RISCV_FEATURE_PMP);
> > > > > >   }
> > > > > >
> > > > > > @@ -169,7 +173,6 @@ static void rv32imafcu_nommu_cpu_init(Object *obj)
> > > > > >       CPURISCVState *env = &RISCV_CPU(obj)->env;
> > > > > >       set_misa(env, RV32 | RVI | RVM | RVA | RVF | RVC | RVU);
> > > > > >       set_priv_version(env, PRIV_VERSION_1_10_0);
> > > > > > -    set_resetvec(env, DEFAULT_RSTVEC);
> > > > > >       set_feature(env, RISCV_FEATURE_PMP);
> > > > > >   }
> > > > > >
> > > > > > @@ -187,7 +190,6 @@ static void rv64gcsu_priv1_09_1_cpu_init(Object *obj)
> > > > > >       CPURISCVState *env = &RISCV_CPU(obj)->env;
> > > > > >       set_misa(env, RV64 | RVI | RVM | RVA | RVF | RVD | RVC | RVS | RVU);
> > > > > >       set_priv_version(env, PRIV_VERSION_1_09_1);
> > > > > > -    set_resetvec(env, DEFAULT_RSTVEC);
> > > > > >       set_feature(env, RISCV_FEATURE_MMU);
> > > > > >       set_feature(env, RISCV_FEATURE_PMP);
> > > > > >   }
> > > > > > @@ -197,7 +199,6 @@ static void rv64gcsu_priv1_10_0_cpu_init(Object *obj)
> > > > > >       CPURISCVState *env = &RISCV_CPU(obj)->env;
> > > > > >       set_misa(env, RV64 | RVI | RVM | RVA | RVF | RVD | RVC | RVS | RVU);
> > > > > >       set_priv_version(env, PRIV_VERSION_1_10_0);
> > > > > > -    set_resetvec(env, DEFAULT_RSTVEC);
> > > > > >       set_feature(env, RISCV_FEATURE_MMU);
> > > > > >       set_feature(env, RISCV_FEATURE_PMP);
> > > > > >   }
> > > > > > @@ -207,7 +208,6 @@ static void rv64imacu_nommu_cpu_init(Object *obj)
> > > > > >       CPURISCVState *env = &RISCV_CPU(obj)->env;
> > > > > >       set_misa(env, RV64 | RVI | RVM | RVA | RVC | RVU);
> > > > > >       set_priv_version(env, PRIV_VERSION_1_10_0);
> > > > > > -    set_resetvec(env, DEFAULT_RSTVEC);
> > > > > >       set_feature(env, RISCV_FEATURE_PMP);
> > > > > >   }
> > > > > >
> > > > > > @@ -399,7 +399,9 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
> > > > > >       }
> > > > > >
> > > > > >       set_priv_version(env, priv_version);
> > > > > > -    set_resetvec(env, DEFAULT_RSTVEC);
> > > > > > +    if (!get_resetvec(env)) {
> > >
> > > What if we have a RISC-V CPU whose reset vector is at address 0?
> >
> > That won't work then. I think if that happens we could swap to a
> > negative number.
> >
>
> env->resetvec is declared as target_ulong so negative number does not work here.
>
> How about just remove set_resetvec() in riscv_cpu_realize()? Or
> introduce a new config parameter for the reset vector, and only
> override it if the config parameter is given, like other properties?

We need to set the default reset veector though. I can just set all of
the values in the init function instead.

Alistair

>
> Regards,
> Bin

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.