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

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

Posted by Dylan Jhong 3 years, 1 month ago

Although the AE350 has not been upstream (preparing for v2),
the reset vector of the AE350 is known to be at the 2G position,
so this patch is corrected in advance.

Signed-off-by: Dylan Jhong <dylan@andestech.com>
Signed-off-by: Ruinland ChuanTzu Tsai <ruinland@andestech.com>
---
 target/riscv/cpu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 2a990f6253..0236abf169 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, uint64_t resetvec)
 {
 #ifndef CONFIG_USER_ONLY
     env->resetvec = resetvec;
-- 
2.17.1

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

Posted by Alistair Francis 3 years, 1 month ago

On Tue, Mar 23, 2021 at 5:15 AM Dylan Jhong <dylan@andestech.com> wrote:
>
> Although the AE350 has not been upstream (preparing for v2),
> the reset vector of the AE350 is known to be at the 2G position,
> so this patch is corrected in advance.
>
> Signed-off-by: Dylan Jhong <dylan@andestech.com>
> Signed-off-by: Ruinland ChuanTzu Tsai <ruinland@andestech.com>
> ---
>  target/riscv/cpu.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 2a990f6253..0236abf169 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, uint64_t resetvec)

resetvec in env is a target_ulong so this should be as well (instead
of a uint64_t).

Alistair

>  {
>  #ifndef CONFIG_USER_ONLY
>      env->resetvec = resetvec;
> --
> 2.17.1
>
>

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

Posted by Dylan Jhong 3 years, 1 month ago

On Wed, Mar 24, 2021 at 10:59:55PM +0800, Alistair Francis wrote:
> On Tue, Mar 23, 2021 at 5:15 AM Dylan Jhong <dylan@andestech.com> wrote:
> >
> > Although the AE350 has not been upstream (preparing for v2),
> > the reset vector of the AE350 is known to be at the 2G position,
> > so this patch is corrected in advance.
> >
> > Signed-off-by: Dylan Jhong <dylan@andestech.com>
> > Signed-off-by: Ruinland ChuanTzu Tsai <ruinland@andestech.com>
> > ---
> >  target/riscv/cpu.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > index 2a990f6253..0236abf169 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, uint64_t resetvec)
> 
> resetvec in env is a target_ulong so this should be as well (instead
> of a uint64_t).
> 
> Alistair
>

Hi Alistar,

Thanks for your comments.

Indeed resetvec should use target_ulong instead of uint64_t.
But in target/riscv/cpu.h:306, there is also a resetvec in struct RISCVCPU but it is defined as uint64_t.
Do you think I should change it to target_ulong together?

ref: 
commit 9b4c9b2b2a50fe4eb90d0ac2d8723b46ecb42511
https://www.mail-archive.com/qemu-devel@nongnu.org/msg730077.html

> >  {
> >  #ifndef CONFIG_USER_ONLY
> >      env->resetvec = resetvec;
> > --
> > 2.17.1
> >
> >

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

Posted by Bin Meng 3 years, 1 month ago

On Thu, Mar 25, 2021 at 11:32 AM Dylan Jhong <dylan@andestech.com> wrote:
>
> On Wed, Mar 24, 2021 at 10:59:55PM +0800, Alistair Francis wrote:
> > On Tue, Mar 23, 2021 at 5:15 AM Dylan Jhong <dylan@andestech.com> wrote:
> > >
> > > Although the AE350 has not been upstream (preparing for v2),
> > > the reset vector of the AE350 is known to be at the 2G position,
> > > so this patch is corrected in advance.
> > >
> > > Signed-off-by: Dylan Jhong <dylan@andestech.com>
> > > Signed-off-by: Ruinland ChuanTzu Tsai <ruinland@andestech.com>
> > > ---
> > >  target/riscv/cpu.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > > index 2a990f6253..0236abf169 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, uint64_t resetvec)
> >
> > resetvec in env is a target_ulong so this should be as well (instead
> > of a uint64_t).
> >
> > Alistair
> >
>
> Hi Alistar,
>
> Thanks for your comments.
>
> Indeed resetvec should use target_ulong instead of uint64_t.

resetvec being target_ulong means that rv32 cannot have a reset vector
beyond 4GiB. I don't think the spec disallow this.

> But in target/riscv/cpu.h:306, there is also a resetvec in struct RISCVCPU but it is defined as uint64_t.
> Do you think I should change it to target_ulong together?
>
> ref:
> commit 9b4c9b2b2a50fe4eb90d0ac2d8723b46ecb42511
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg730077.html

Regards,
Bin

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

Posted by Bin Meng 3 years, 1 month ago

On Thu, Mar 25, 2021 at 11:40 AM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> On Thu, Mar 25, 2021 at 11:32 AM Dylan Jhong <dylan@andestech.com> wrote:
> >
> > On Wed, Mar 24, 2021 at 10:59:55PM +0800, Alistair Francis wrote:
> > > On Tue, Mar 23, 2021 at 5:15 AM Dylan Jhong <dylan@andestech.com> wrote:
> > > >
> > > > Although the AE350 has not been upstream (preparing for v2),
> > > > the reset vector of the AE350 is known to be at the 2G position,
> > > > so this patch is corrected in advance.
> > > >
> > > > Signed-off-by: Dylan Jhong <dylan@andestech.com>
> > > > Signed-off-by: Ruinland ChuanTzu Tsai <ruinland@andestech.com>
> > > > ---
> > > >  target/riscv/cpu.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > > > index 2a990f6253..0236abf169 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, uint64_t resetvec)
> > >
> > > resetvec in env is a target_ulong so this should be as well (instead
> > > of a uint64_t).
> > >
> > > Alistair
> > >
> >
> > Hi Alistar,
> >
> > Thanks for your comments.
> >
> > Indeed resetvec should use target_ulong instead of uint64_t.
>
> resetvec being target_ulong means that rv32 cannot have a reset vector
> beyond 4GiB. I don't think the spec disallow this.

Ah, I was wrong. The spec says: "the pc is set to an implementation-de
ned reset vector" and pc is XLEN wide.

So for rv32 the reset vector cannot beyond 4GiB. We should change it
to target_ulong.

Regards,
Bin