diff mbox series

firmware: Do not allow harts with different XLEN to boot

Message ID 20210602014201.246899-1-atish.patra@wdc.com
State New
Headers show
Series firmware: Do not allow harts with different XLEN to boot | expand

Commit Message

Atish Patra June 2, 2021, 1:42 a.m. UTC
OpenSBI is built separately for RV32/RV64. Thus, it shouldn't try to boot
harts with a different XLEN compared to what it is compiled with.

This patch fixes the issue observed in beagleV which has two U74(RV64) and
one E24 (RV32) harts.

Signed-off-by: Atish Patra <atish.patra@wdc.com>
---
 firmware/fw_base.S | 10 ++++++++++
 1 file changed, 10 insertions(+)

Comments

Jessica Clarke June 2, 2021, 1:54 a.m. UTC | #1
On 2 Jun 2021, at 02:42, Atish Patra <atish.patra@wdc.com> wrote:
> 
> OpenSBI is built separately for RV32/RV64. Thus, it shouldn't try to boot
> harts with a different XLEN compared to what it is compiled with.
> 
> This patch fixes the issue observed in beagleV which has two U74(RV64) and
> one E24 (RV32) harts.

Is this really necessary? Can the different clusters not boot different
OpenSBIs? This is a really gross hack for what seems like a stupid
hardware decision if they both blindly load and run from the same place.

Jess
Anup Patel June 2, 2021, 11:51 a.m. UTC | #2
> -----Original Message-----
> From: Jessica Clarke <jrtc27@jrtc27.com>
> Sent: 02 June 2021 07:25
> To: Atish Patra <Atish.Patra@wdc.com>
> Cc: opensbi@lists.infradead.org; Anup Patel <Anup.Patel@wdc.com>
> Subject: Re: [PATCH] firmware: Do not allow harts with different XLEN to boot
> 
> On 2 Jun 2021, at 02:42, Atish Patra <atish.patra@wdc.com> wrote:
> >
> > OpenSBI is built separately for RV32/RV64. Thus, it shouldn't try to
> > boot harts with a different XLEN compared to what it is compiled with.
> >
> > This patch fixes the issue observed in beagleV which has two U74(RV64)
> > and one E24 (RV32) harts.
> 
> Is this really necessary? Can the different clusters not boot different
> OpenSBIs? This is a really gross hack for what seems like a stupid hardware
> decision if they both blindly load and run from the same place.

Ideally, a platform should boot different OpenSBI firmwares for clusters
with differing XLEN.

In any case, if a RV32 HART enters RV64 OpenSBI firmware (or vice-versa)
then the HART should enter a WFI hang loop very early.

This patch is more of a precautionary check for platforms like BeagleV
where HART with differing XLEN jumps to OpenSBI firmware.

Regards,
Anup
Anup Patel June 2, 2021, 11:57 a.m. UTC | #3
> -----Original Message-----
> From: Atish Patra <atish.patra@wdc.com>
> Sent: 02 June 2021 07:12
> To: opensbi@lists.infradead.org
> Cc: Anup Patel <Anup.Patel@wdc.com>; Atish Patra <Atish.Patra@wdc.com>
> Subject: [PATCH] firmware: Do not allow harts with different XLEN to boot
> 
> OpenSBI is built separately for RV32/RV64. Thus, it shouldn't try to boot harts
> with a different XLEN compared to what it is compiled with.
> 
> This patch fixes the issue observed in beagleV which has two U74(RV64) and
> one E24 (RV32) harts.
> 
> Signed-off-by: Atish Patra <atish.patra@wdc.com>
> ---
>  firmware/fw_base.S | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/firmware/fw_base.S b/firmware/fw_base.S index
> a5ce946aa3fe..ae12a126ca66 100644
> --- a/firmware/fw_base.S
> +++ b/firmware/fw_base.S
> @@ -47,6 +47,16 @@
>  	.globl _start
>  	.globl _start_warm
>  _start:
> +	csrr   t0, misa
> +	slti   t1, t0, 0
> +	slli   t1, t1, 1
> +	slli   t0, t0, 1
> +	slti   t0, t0, 0
> +	add    t2, t0, t1
> +	li     t3, __riscv_xlen
> +	srli   t3, t3, 5
> +	bne    t2, t3, _start_hang
> +

Unfortunately, MISA can be wired to zero in which case
platform will provide non-standard hooks to check
extensions.

How about this ?

	li    t0, 1
	slli t0, t0, 32
#if __riscv_xlen == 32
	bne t0, zero, _start_hang
#else
	beq t0, zero, _start_hang
#endif

Probably above can be ASM macro so that we can call
this macro at start of both _start() and _start_warm()

>  	/* Find preferred boot HART id */
>  	MOV_3R	s0, a0, s1, a1, s2, a2
>  	call	fw_boot_hart
> --
> 2.31.1

Regards,
Anup
Xiang W June 2, 2021, 12:25 p.m. UTC | #4
在 2021-06-02星期三的 11:51 +0000,Anup Patel写道:
> 
> 
> > -----Original Message-----
> > From: Jessica Clarke <jrtc27@jrtc27.com>
> > Sent: 02 June 2021 07:25
> > To: Atish Patra <Atish.Patra@wdc.com>
> > Cc: opensbi@lists.infradead.org; Anup Patel <Anup.Patel@wdc.com>
> > Subject: Re: [PATCH] firmware: Do not allow harts with different
> > XLEN to boot
> > 
> > On 2 Jun 2021, at 02:42, Atish Patra <atish.patra@wdc.com> wrote:
> > > 
> > > OpenSBI is built separately for RV32/RV64. Thus, it shouldn't try
> > > to
> > > boot harts with a different XLEN compared to what it is compiled
> > > with.
> > > 
> > > This patch fixes the issue observed in beagleV which has two
> > > U74(RV64)
> > > and one E24 (RV32) harts.
> > 
> > Is this really necessary? Can the different clusters not boot
> > different
> > OpenSBIs? This is a really gross hack for what seems like a stupid
> > hardware
> > decision if they both blindly load and run from the same place.
> 
> Ideally, a platform should boot different OpenSBI firmwares for
> clusters
> with differing XLEN.
> 
> In any case, if a RV32 HART enters RV64 OpenSBI firmware (or vice-
> versa)
> then the HART should enter a WFI hang loop very early.
> 
> This patch is more of a precautionary check for platforms like
> BeagleV
> where HART with differing XLEN jumps to OpenSBI firmware.
> 
> Regards,
> Anup
> 
MXL can be modified, if it does not match, whether to try to modify
MXL, and then continue to run

Regards,
Xiang W
Atish Patra June 3, 2021, 5:48 p.m. UTC | #5
On Wed, Jun 2, 2021 at 5:22 AM Anup Patel <Anup.Patel@wdc.com> wrote:
>
>
>
> > -----Original Message-----
> > From: Atish Patra <atish.patra@wdc.com>
> > Sent: 02 June 2021 07:12
> > To: opensbi@lists.infradead.org
> > Cc: Anup Patel <Anup.Patel@wdc.com>; Atish Patra <Atish.Patra@wdc.com>
> > Subject: [PATCH] firmware: Do not allow harts with different XLEN to boot
> >
> > OpenSBI is built separately for RV32/RV64. Thus, it shouldn't try to boot harts
> > with a different XLEN compared to what it is compiled with.
> >
> > This patch fixes the issue observed in beagleV which has two U74(RV64) and
> > one E24 (RV32) harts.
> >
> > Signed-off-by: Atish Patra <atish.patra@wdc.com>
> > ---
> >  firmware/fw_base.S | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> >
> > diff --git a/firmware/fw_base.S b/firmware/fw_base.S index
> > a5ce946aa3fe..ae12a126ca66 100644
> > --- a/firmware/fw_base.S
> > +++ b/firmware/fw_base.S
> > @@ -47,6 +47,16 @@
> >       .globl _start
> >       .globl _start_warm
> >  _start:
> > +     csrr   t0, misa
> > +     slti   t1, t0, 0
> > +     slli   t1, t1, 1
> > +     slli   t0, t0, 1
> > +     slti   t0, t0, 0
> > +     add    t2, t0, t1
> > +     li     t3, __riscv_xlen
> > +     srli   t3, t3, 5
> > +     bne    t2, t3, _start_hang
> > +
>
> Unfortunately, MISA can be wired to zero in which case
> platform will provide non-standard hooks to check
> extensions.
>
> How about this ?
>
>         li    t0, 1
>         slli t0, t0, 32
> #if __riscv_xlen == 32
>         bne t0, zero, _start_hang
> #else
>         beq t0, zero, _start_hang
> #endif
>
> Probably above can be ASM macro so that we can call
> this macro at start of both _start() and _start_warm()
>

Sure. I will do that.

Just to clarify, we don't need this for Beagleboard.
After talking to StarFive engineers, it was confirmed that the U74 and
E24 are in different clusters and both contain hart0.
However,  E24 never enters OpenSBI as it executes code compiled for RV32 only.

This patch will just serve as an improved sanity check.

> >       /* Find preferred boot HART id */
> >       MOV_3R  s0, a0, s1, a1, s2, a2
> >       call    fw_boot_hart
> > --
> > 2.31.1
>
> Regards,
> Anup
>
>
> --
> opensbi mailing list
> opensbi@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/opensbi
diff mbox series

Patch

diff --git a/firmware/fw_base.S b/firmware/fw_base.S
index a5ce946aa3fe..ae12a126ca66 100644
--- a/firmware/fw_base.S
+++ b/firmware/fw_base.S
@@ -47,6 +47,16 @@ 
 	.globl _start
 	.globl _start_warm
 _start:
+	csrr   t0, misa
+	slti   t1, t0, 0
+	slli   t1, t1, 1
+	slli   t0, t0, 1
+	slti   t0, t0, 0
+	add    t2, t0, t1
+	li     t3, __riscv_xlen
+	srli   t3, t3, 5
+	bne    t2, t3, _start_hang
+
 	/* Find preferred boot HART id */
 	MOV_3R	s0, a0, s1, a1, s2, a2
 	call	fw_boot_hart