diff mbox series

[U-Boot,v3,1/3] riscv: Add kconfig option to run u-boot in S-mode

Message ID 20181121034112.7136-2-anup@brainfault.org
State Superseded
Delegated to: Andes
Headers show
Series [U-Boot,v3,1/3] riscv: Add kconfig option to run u-boot in S-mode | expand

Commit Message

Anup Patel Nov. 21, 2018, 3:41 a.m. UTC 
This patch adds kconfig option RISCV_SMODE to run u-boot in
S-mode. When this opition is enabled we use s<xyz> CSRs instead
of m<xyz> CSRs.

It is important to note that there is no equivalent S-mode CSR
for misa and mhartid CSRs so we expect M-mode runtime firmware
(BBL or equivalent) to emulate misa and mhartid CSR read.

In-future, we will have more patches to avoid accessing misa and
mhartid CSRs from S-mode.

Signed-off-by: Anup Patel <anup@brainfault.org>
Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
Tested-by: Bin Meng <bmeng.cn@gmail.com>
---
 arch/riscv/Kconfig     |  5 +++++
 arch/riscv/cpu/start.S | 33 +++++++++++++++++++++++++++++++++
 2 files changed, 38 insertions(+)

Comments

Lukas Auer Nov. 21, 2018, 1:47 p.m. UTC | #1 
Hi Anup,

On Wed, 2018-11-21 at 09:11 +0530, Anup Patel wrote:
> This patch adds kconfig option RISCV_SMODE to run u-boot in

nit: U-Boot (also in the first line of the commit message)

> S-mode. When this opition is enabled we use s<xyz> CSRs instead
> of m<xyz> CSRs.
> 
> It is important to note that there is no equivalent S-mode CSR
> for misa and mhartid CSRs so we expect M-mode runtime firmware
> (BBL or equivalent) to emulate misa and mhartid CSR read.
> 
> In-future, we will have more patches to avoid accessing misa and
> mhartid CSRs from S-mode.
> 
> Signed-off-by: Anup Patel <anup@brainfault.org>
> Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
> Tested-by: Bin Meng <bmeng.cn@gmail.com>
> ---
>  arch/riscv/Kconfig     |  5 +++++
>  arch/riscv/cpu/start.S | 33 +++++++++++++++++++++++++++++++++
>  2 files changed, 38 insertions(+)
> 
> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> index 3e0af55e71..8f2139ff60 100644
> --- a/arch/riscv/Kconfig
> +++ b/arch/riscv/Kconfig
> @@ -55,6 +55,11 @@ config RISCV_ISA_C
>  config RISCV_ISA_A
>  	def_bool y
>  
> +config RISCV_SMODE
> +	bool "Run in S-Mode"
> +	help
> +	  Enable this option to build an U-Boot for RISC-V S-Mode

nit: there should be no "an" here

> +
>  config 32BIT
>  	bool
>  
> diff --git a/arch/riscv/cpu/start.S b/arch/riscv/cpu/start.S
> index 5af189b338..e4276e8e19 100644
> --- a/arch/riscv/cpu/start.S
> +++ b/arch/riscv/cpu/start.S
> @@ -39,10 +39,18 @@ _start:
>  	mv	s1, a1
>  
>  	la	t0, trap_entry
> +#ifdef CONFIG_RISCV_SMODE

It might make the code more readable by using defines for the CSRs,
which are set to the correct values at the start of start.S depending
on if CONFIG_RISCV_SMODE is set (similar to what we do for the load and
store instructions).
This is just a thought, I think this way is perfectly fine as well. :)

Reviewed-by: Lukas Auer <lukas.auer@aisec.fraunhofer.de>

Thanks,
Lukas

> +	csrw	stvec, t0
> +#else
>  	csrw	mtvec, t0
> +#endif
>  
>  	/* mask all interrupts */
> +#ifdef CONFIG_RISCV_SMODE
> +	csrw	sie, zero
> +#else
>  	csrw	mie, zero
> +#endif
>  
>  	/* Enable cache */
>  	jal	icache_enable
> @@ -164,7 +172,11 @@ fix_rela_dyn:
>  */
>  	la	t0, trap_entry
>  	add	t0, t0, t6
> +#ifdef CONFIG_RISCV_SMODE
> +	csrw	stvec, t0
> +#else
>  	csrw	mtvec, t0
> +#endif
>  
>  clear_bss:
>  	la	t0, __bss_start		/* t0 <- rel __bss_start in
> FLASH */
> @@ -236,17 +248,34 @@ trap_entry:
>  	SREG	x29, 29*REGBYTES(sp)
>  	SREG	x30, 30*REGBYTES(sp)
>  	SREG	x31, 31*REGBYTES(sp)
> +#ifdef CONFIG_RISCV_SMODE
> +	csrr	a0, scause
> +	csrr	a1, sepc
> +#else
>  	csrr	a0, mcause
>  	csrr	a1, mepc
> +#endif
>  	mv	a2, sp
>  	jal	handle_trap
> +#ifdef CONFIG_RISCV_SMODE
> +	csrw	sepc, a0
> +#else
>  	csrw	mepc, a0
> +#endif
>  
> +#ifdef CONFIG_RISCV_SMODE
> +/*
> + * Remain in S-mode after sret
> + */
> +	li	t0, SSTATUS_SPP
> +	csrs	sstatus, t0
> +#else
>  /*
>   * Remain in M-mode after mret
>   */
>  	li	t0, MSTATUS_MPP
>  	csrs	mstatus, t0
> +#endif
>  	LREG	x1, 1*REGBYTES(sp)
>  	LREG	x2, 2*REGBYTES(sp)
>  	LREG	x3, 3*REGBYTES(sp)
> @@ -279,4 +308,8 @@ trap_entry:
>  	LREG	x30, 30*REGBYTES(sp)
>  	LREG	x31, 31*REGBYTES(sp)
>  	addi	sp, sp, 32*REGBYTES
> +#ifdef CONFIG_RISCV_SMODE
> +	sret
> +#else
>  	mret
> +#endif
Palmer Dabbelt Nov. 21, 2018, 4:28 p.m. UTC | #2 
On Tue, 20 Nov 2018 19:41:10 PST (-0800), anup@brainfault.org wrote:
> This patch adds kconfig option RISCV_SMODE to run u-boot in
> S-mode. When this opition is enabled we use s<xyz> CSRs instead
> of m<xyz> CSRs.
>
> It is important to note that there is no equivalent S-mode CSR
> for misa and mhartid CSRs so we expect M-mode runtime firmware
> (BBL or equivalent) to emulate misa and mhartid CSR read.
>
> In-future, we will have more patches to avoid accessing misa and
> mhartid CSRs from S-mode.

Ya, I don't like this.  Our current boot protocol puts mhartid in a1 upon 
entering the supervisor, and allows the detection of misa via a device tree 
pointer provided in a0.  

As long as everyone agrees this isn't what we're actually looking for then I'm 
fine with the patch, I just don't want to end up requiring this "M mode 
emulates some CSRs for S mode" interface.  Since we don't have an actual 
platform spec we've got to be careful to avoid a bunch of defacto interfaces 
that we'll need to support later.

>
> Signed-off-by: Anup Patel <anup@brainfault.org>
> Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
> Tested-by: Bin Meng <bmeng.cn@gmail.com>
> ---
>  arch/riscv/Kconfig     |  5 +++++
>  arch/riscv/cpu/start.S | 33 +++++++++++++++++++++++++++++++++
>  2 files changed, 38 insertions(+)
>
> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> index 3e0af55e71..8f2139ff60 100644
> --- a/arch/riscv/Kconfig
> +++ b/arch/riscv/Kconfig
> @@ -55,6 +55,11 @@ config RISCV_ISA_C
>  config RISCV_ISA_A
>  	def_bool y
>
> +config RISCV_SMODE
> +	bool "Run in S-Mode"
> +	help
> +	  Enable this option to build an U-Boot for RISC-V S-Mode
> +
>  config 32BIT
>  	bool
>
> diff --git a/arch/riscv/cpu/start.S b/arch/riscv/cpu/start.S
> index 5af189b338..e4276e8e19 100644
> --- a/arch/riscv/cpu/start.S
> +++ b/arch/riscv/cpu/start.S
> @@ -39,10 +39,18 @@ _start:
>  	mv	s1, a1
>
>  	la	t0, trap_entry
> +#ifdef CONFIG_RISCV_SMODE
> +	csrw	stvec, t0
> +#else
>  	csrw	mtvec, t0
> +#endif
>
>  	/* mask all interrupts */
> +#ifdef CONFIG_RISCV_SMODE
> +	csrw	sie, zero
> +#else
>  	csrw	mie, zero
> +#endif
>
>  	/* Enable cache */
>  	jal	icache_enable
> @@ -164,7 +172,11 @@ fix_rela_dyn:
>  */
>  	la	t0, trap_entry
>  	add	t0, t0, t6
> +#ifdef CONFIG_RISCV_SMODE
> +	csrw	stvec, t0
> +#else
>  	csrw	mtvec, t0
> +#endif
>
>  clear_bss:
>  	la	t0, __bss_start		/* t0 <- rel __bss_start in FLASH */
> @@ -236,17 +248,34 @@ trap_entry:
>  	SREG	x29, 29*REGBYTES(sp)
>  	SREG	x30, 30*REGBYTES(sp)
>  	SREG	x31, 31*REGBYTES(sp)
> +#ifdef CONFIG_RISCV_SMODE
> +	csrr	a0, scause
> +	csrr	a1, sepc
> +#else
>  	csrr	a0, mcause
>  	csrr	a1, mepc
> +#endif
>  	mv	a2, sp
>  	jal	handle_trap
> +#ifdef CONFIG_RISCV_SMODE
> +	csrw	sepc, a0
> +#else
>  	csrw	mepc, a0
> +#endif
>
> +#ifdef CONFIG_RISCV_SMODE
> +/*
> + * Remain in S-mode after sret
> + */
> +	li	t0, SSTATUS_SPP
> +	csrs	sstatus, t0
> +#else
>  /*
>   * Remain in M-mode after mret
>   */
>  	li	t0, MSTATUS_MPP
>  	csrs	mstatus, t0
> +#endif
>  	LREG	x1, 1*REGBYTES(sp)
>  	LREG	x2, 2*REGBYTES(sp)
>  	LREG	x3, 3*REGBYTES(sp)
> @@ -279,4 +308,8 @@ trap_entry:
>  	LREG	x30, 30*REGBYTES(sp)
>  	LREG	x31, 31*REGBYTES(sp)
>  	addi	sp, sp, 32*REGBYTES
> +#ifdef CONFIG_RISCV_SMODE
> +	sret
> +#else
>  	mret
> +#endif
Lukas Auer Nov. 21, 2018, 10:06 p.m. UTC | #3 
Hi Palmer,

On Wed, 2018-11-21 at 08:28 -0800, Palmer Dabbelt wrote:
> On Tue, 20 Nov 2018 19:41:10 PST (-0800), anup@brainfault.org wrote:
> > This patch adds kconfig option RISCV_SMODE to run u-boot in
> > S-mode. When this opition is enabled we use s<xyz> CSRs instead
> > of m<xyz> CSRs.
> > 
> > It is important to note that there is no equivalent S-mode CSR
> > for misa and mhartid CSRs so we expect M-mode runtime firmware
> > (BBL or equivalent) to emulate misa and mhartid CSR read.
> > 
> > In-future, we will have more patches to avoid accessing misa and
> > mhartid CSRs from S-mode.
> 
> Ya, I don't like this.  Our current boot protocol puts mhartid in a1
> upon 
> entering the supervisor, and allows the detection of misa via a
> device tree 
> pointer provided in a0.  
> 
> As long as everyone agrees this isn't what we're actually looking for
> then I'm 
> fine with the patch, I just don't want to end up requiring this "M
> mode 
> emulates some CSRs for S mode" interface.  Since we don't have an
> actual 
> platform spec we've got to be careful to avoid a bunch of defacto
> interfaces 
> that we'll need to support later.
> 

I agree with you. We actually won't need the CSR emulation of misa and
mhartid for long. With Bin's recent patch series [1], the ISA string is
read from the device tree, replacing the use of misa. mhartid is only
used to pass the hart id to the kernel. I am going to replace it in a
patch series I am working on.
So the interface should just be temporary.

Thanks,
Lukas

[1]: https://patchwork.ozlabs.org/project/uboot/list/?series=75644

> > 
> > Signed-off-by: Anup Patel <anup@brainfault.org>
> > Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
> > Tested-by: Bin Meng <bmeng.cn@gmail.com>
> > ---
> >  arch/riscv/Kconfig     |  5 +++++
> >  arch/riscv/cpu/start.S | 33 +++++++++++++++++++++++++++++++++
> >  2 files changed, 38 insertions(+)
> > 
> > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> > index 3e0af55e71..8f2139ff60 100644
> > --- a/arch/riscv/Kconfig
> > +++ b/arch/riscv/Kconfig
> > @@ -55,6 +55,11 @@ config RISCV_ISA_C
> >  config RISCV_ISA_A
> >  	def_bool y
> > 
> > +config RISCV_SMODE
> > +	bool "Run in S-Mode"
> > +	help
> > +	  Enable this option to build an U-Boot for RISC-V S-Mode
> > +
> >  config 32BIT
> >  	bool
> > 
> > diff --git a/arch/riscv/cpu/start.S b/arch/riscv/cpu/start.S
> > index 5af189b338..e4276e8e19 100644
> > --- a/arch/riscv/cpu/start.S
> > +++ b/arch/riscv/cpu/start.S
> > @@ -39,10 +39,18 @@ _start:
> >  	mv	s1, a1
> > 
> >  	la	t0, trap_entry
> > +#ifdef CONFIG_RISCV_SMODE
> > +	csrw	stvec, t0
> > +#else
> >  	csrw	mtvec, t0
> > +#endif
> > 
> >  	/* mask all interrupts */
> > +#ifdef CONFIG_RISCV_SMODE
> > +	csrw	sie, zero
> > +#else
> >  	csrw	mie, zero
> > +#endif
> > 
> >  	/* Enable cache */
> >  	jal	icache_enable
> > @@ -164,7 +172,11 @@ fix_rela_dyn:
> >  */
> >  	la	t0, trap_entry
> >  	add	t0, t0, t6
> > +#ifdef CONFIG_RISCV_SMODE
> > +	csrw	stvec, t0
> > +#else
> >  	csrw	mtvec, t0
> > +#endif
> > 
> >  clear_bss:
> >  	la	t0, __bss_start		/* t0 <- rel __bss_start in FLASH
> > */
> > @@ -236,17 +248,34 @@ trap_entry:
> >  	SREG	x29, 29*REGBYTES(sp)
> >  	SREG	x30, 30*REGBYTES(sp)
> >  	SREG	x31, 31*REGBYTES(sp)
> > +#ifdef CONFIG_RISCV_SMODE
> > +	csrr	a0, scause
> > +	csrr	a1, sepc
> > +#else
> >  	csrr	a0, mcause
> >  	csrr	a1, mepc
> > +#endif
> >  	mv	a2, sp
> >  	jal	handle_trap
> > +#ifdef CONFIG_RISCV_SMODE
> > +	csrw	sepc, a0
> > +#else
> >  	csrw	mepc, a0
> > +#endif
> > 
> > +#ifdef CONFIG_RISCV_SMODE
> > +/*
> > + * Remain in S-mode after sret
> > + */
> > +	li	t0, SSTATUS_SPP
> > +	csrs	sstatus, t0
> > +#else
> >  /*
> >   * Remain in M-mode after mret
> >   */
> >  	li	t0, MSTATUS_MPP
> >  	csrs	mstatus, t0
> > +#endif
> >  	LREG	x1, 1*REGBYTES(sp)
> >  	LREG	x2, 2*REGBYTES(sp)
> >  	LREG	x3, 3*REGBYTES(sp)
> > @@ -279,4 +308,8 @@ trap_entry:
> >  	LREG	x30, 30*REGBYTES(sp)
> >  	LREG	x31, 31*REGBYTES(sp)
> >  	addi	sp, sp, 32*REGBYTES
> > +#ifdef CONFIG_RISCV_SMODE
> > +	sret
> > +#else
> >  	mret
> > +#endif
Lukas Auer Nov. 21, 2018, 10:11 p.m. UTC | #4 
Hi Anup,

On Wed, 2018-11-21 at 13:47 +0000, Auer, Lukas wrote:
> Hi Anup,
> 
> On Wed, 2018-11-21 at 09:11 +0530, Anup Patel wrote:
> > This patch adds kconfig option RISCV_SMODE to run u-boot in
> 
> nit: U-Boot (also in the first line of the commit message)
> 
> > S-mode. When this opition is enabled we use s<xyz> CSRs instead
> > of m<xyz> CSRs.
> > 

Can you also modify arch/riscv/lib/interrupts.c to test for the
supervisor instead of the machine timer and external interrupts when
CONFIG_RISCV_SMODE is set?

Thanks,
Lukas

> > It is important to note that there is no equivalent S-mode CSR
> > for misa and mhartid CSRs so we expect M-mode runtime firmware
> > (BBL or equivalent) to emulate misa and mhartid CSR read.
> > 
> > In-future, we will have more patches to avoid accessing misa and
> > mhartid CSRs from S-mode.
> > 
> > Signed-off-by: Anup Patel <anup@brainfault.org>
> > Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
> > Tested-by: Bin Meng <bmeng.cn@gmail.com>
> > ---
> >  arch/riscv/Kconfig     |  5 +++++
> >  arch/riscv/cpu/start.S | 33 +++++++++++++++++++++++++++++++++
> >  2 files changed, 38 insertions(+)
> > 
> > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> > index 3e0af55e71..8f2139ff60 100644
> > --- a/arch/riscv/Kconfig
> > +++ b/arch/riscv/Kconfig
> > @@ -55,6 +55,11 @@ config RISCV_ISA_C
> >  config RISCV_ISA_A
> >  	def_bool y
> >  
> > +config RISCV_SMODE
> > +	bool "Run in S-Mode"
> > +	help
> > +	  Enable this option to build an U-Boot for RISC-V S-Mode
> 
> nit: there should be no "an" here
> 
> > +
> >  config 32BIT
> >  	bool
> >  
> > diff --git a/arch/riscv/cpu/start.S b/arch/riscv/cpu/start.S
> > index 5af189b338..e4276e8e19 100644
> > --- a/arch/riscv/cpu/start.S
> > +++ b/arch/riscv/cpu/start.S
> > @@ -39,10 +39,18 @@ _start:
> >  	mv	s1, a1
> >  
> >  	la	t0, trap_entry
> > +#ifdef CONFIG_RISCV_SMODE
> 
> It might make the code more readable by using defines for the CSRs,
> which are set to the correct values at the start of start.S depending
> on if CONFIG_RISCV_SMODE is set (similar to what we do for the load
> and
> store instructions).
> This is just a thought, I think this way is perfectly fine as well.
> :)
> 
> Reviewed-by: Lukas Auer <lukas.auer@aisec.fraunhofer.de>
> 
> Thanks,
> Lukas
> 
> > +	csrw	stvec, t0
> > +#else
> >  	csrw	mtvec, t0
> > +#endif
> >  
> >  	/* mask all interrupts */
> > +#ifdef CONFIG_RISCV_SMODE
> > +	csrw	sie, zero
> > +#else
> >  	csrw	mie, zero
> > +#endif
> >  
> >  	/* Enable cache */
> >  	jal	icache_enable
> > @@ -164,7 +172,11 @@ fix_rela_dyn:
> >  */
> >  	la	t0, trap_entry
> >  	add	t0, t0, t6
> > +#ifdef CONFIG_RISCV_SMODE
> > +	csrw	stvec, t0
> > +#else
> >  	csrw	mtvec, t0
> > +#endif
> >  
> >  clear_bss:
> >  	la	t0, __bss_start		/* t0 <- rel __bss_start in
> > FLASH */
> > @@ -236,17 +248,34 @@ trap_entry:
> >  	SREG	x29, 29*REGBYTES(sp)
> >  	SREG	x30, 30*REGBYTES(sp)
> >  	SREG	x31, 31*REGBYTES(sp)
> > +#ifdef CONFIG_RISCV_SMODE
> > +	csrr	a0, scause
> > +	csrr	a1, sepc
> > +#else
> >  	csrr	a0, mcause
> >  	csrr	a1, mepc
> > +#endif
> >  	mv	a2, sp
> >  	jal	handle_trap
> > +#ifdef CONFIG_RISCV_SMODE
> > +	csrw	sepc, a0
> > +#else
> >  	csrw	mepc, a0
> > +#endif
> >  
> > +#ifdef CONFIG_RISCV_SMODE
> > +/*
> > + * Remain in S-mode after sret
> > + */
> > +	li	t0, SSTATUS_SPP
> > +	csrs	sstatus, t0
> > +#else
> >  /*
> >   * Remain in M-mode after mret
> >   */
> >  	li	t0, MSTATUS_MPP
> >  	csrs	mstatus, t0
> > +#endif
> >  	LREG	x1, 1*REGBYTES(sp)
> >  	LREG	x2, 2*REGBYTES(sp)
> >  	LREG	x3, 3*REGBYTES(sp)
> > @@ -279,4 +308,8 @@ trap_entry:
> >  	LREG	x30, 30*REGBYTES(sp)
> >  	LREG	x31, 31*REGBYTES(sp)
> >  	addi	sp, sp, 32*REGBYTES
> > +#ifdef CONFIG_RISCV_SMODE
> > +	sret
> > +#else
> >  	mret
> > +#endif
> 
> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> https://lists.denx.de/listinfo/u-boot
Palmer Dabbelt Nov. 21, 2018, 10:16 p.m. UTC | #5 
On Wed, 21 Nov 2018 14:06:00 PST (-0800), lukas.auer@aisec.fraunhofer.de wrote:
> Hi Palmer,
> 
> On Wed, 2018-11-21 at 08:28 -0800, Palmer Dabbelt wrote:
>> On Tue, 20 Nov 2018 19:41:10 PST (-0800), anup@brainfault.org wrote:
>> > This patch adds kconfig option RISCV_SMODE to run u-boot in
>> > S-mode. When this opition is enabled we use s<xyz> CSRs instead
>> > of m<xyz> CSRs.
>> > 
>> > It is important to note that there is no equivalent S-mode CSR
>> > for misa and mhartid CSRs so we expect M-mode runtime firmware
>> > (BBL or equivalent) to emulate misa and mhartid CSR read.
>> > 
>> > In-future, we will have more patches to avoid accessing misa and
>> > mhartid CSRs from S-mode.
>> 
>> Ya, I don't like this.  Our current boot protocol puts mhartid in a1
>> upon 
>> entering the supervisor, and allows the detection of misa via a
>> device tree 
>> pointer provided in a0.  
>> 
>> As long as everyone agrees this isn't what we're actually looking for
>> then I'm 
>> fine with the patch, I just don't want to end up requiring this "M
>> mode 
>> emulates some CSRs for S mode" interface.  Since we don't have an
>> actual 
>> platform spec we've got to be careful to avoid a bunch of defacto
>> interfaces 
>> that we'll need to support later.
>> 
> 
> I agree with you. We actually won't need the CSR emulation of misa and
> mhartid for long. With Bin's recent patch series [1], the ISA string is
> read from the device tree, replacing the use of misa. mhartid is only
> used to pass the hart id to the kernel. I am going to replace it in a
> patch series I am working on.
> So the interface should just be temporary.

Great, thanks!

> 
> Thanks,
> Lukas
> 
> [1]: https://patchwork.ozlabs.org/project/uboot/list/?series=75644
> 
>> > 
>> > Signed-off-by: Anup Patel <anup@brainfault.org>
>> > Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
>> > Tested-by: Bin Meng <bmeng.cn@gmail.com>
>> > ---
>> >  arch/riscv/Kconfig     |  5 +++++
>> >  arch/riscv/cpu/start.S | 33 +++++++++++++++++++++++++++++++++
>> >  2 files changed, 38 insertions(+)
>> > 
>> > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
>> > index 3e0af55e71..8f2139ff60 100644
>> > --- a/arch/riscv/Kconfig
>> > +++ b/arch/riscv/Kconfig
>> > @@ -55,6 +55,11 @@ config RISCV_ISA_C
>> >  config RISCV_ISA_A
>> >  	def_bool y
>> > 
>> > +config RISCV_SMODE
>> > +	bool "Run in S-Mode"
>> > +	help
>> > +	  Enable this option to build an U-Boot for RISC-V S-Mode
>> > +
>> >  config 32BIT
>> >  	bool
>> > 
>> > diff --git a/arch/riscv/cpu/start.S b/arch/riscv/cpu/start.S
>> > index 5af189b338..e4276e8e19 100644
>> > --- a/arch/riscv/cpu/start.S
>> > +++ b/arch/riscv/cpu/start.S
>> > @@ -39,10 +39,18 @@ _start:
>> >  	mv	s1, a1
>> > 
>> >  	la	t0, trap_entry
>> > +#ifdef CONFIG_RISCV_SMODE
>> > +	csrw	stvec, t0
>> > +#else
>> >  	csrw	mtvec, t0
>> > +#endif
>> > 
>> >  	/* mask all interrupts */
>> > +#ifdef CONFIG_RISCV_SMODE
>> > +	csrw	sie, zero
>> > +#else
>> >  	csrw	mie, zero
>> > +#endif
>> > 
>> >  	/* Enable cache */
>> >  	jal	icache_enable
>> > @@ -164,7 +172,11 @@ fix_rela_dyn:
>> >  */
>> >  	la	t0, trap_entry
>> >  	add	t0, t0, t6
>> > +#ifdef CONFIG_RISCV_SMODE
>> > +	csrw	stvec, t0
>> > +#else
>> >  	csrw	mtvec, t0
>> > +#endif
>> > 
>> >  clear_bss:
>> >  	la	t0, __bss_start		/* t0 <- rel __bss_start in FLASH
>> > */
>> > @@ -236,17 +248,34 @@ trap_entry:
>> >  	SREG	x29, 29*REGBYTES(sp)
>> >  	SREG	x30, 30*REGBYTES(sp)
>> >  	SREG	x31, 31*REGBYTES(sp)
>> > +#ifdef CONFIG_RISCV_SMODE
>> > +	csrr	a0, scause
>> > +	csrr	a1, sepc
>> > +#else
>> >  	csrr	a0, mcause
>> >  	csrr	a1, mepc
>> > +#endif
>> >  	mv	a2, sp
>> >  	jal	handle_trap
>> > +#ifdef CONFIG_RISCV_SMODE
>> > +	csrw	sepc, a0
>> > +#else
>> >  	csrw	mepc, a0
>> > +#endif
>> > 
>> > +#ifdef CONFIG_RISCV_SMODE
>> > +/*
>> > + * Remain in S-mode after sret
>> > + */
>> > +	li	t0, SSTATUS_SPP
>> > +	csrs	sstatus, t0
>> > +#else
>> >  /*
>> >   * Remain in M-mode after mret
>> >   */
>> >  	li	t0, MSTATUS_MPP
>> >  	csrs	mstatus, t0
>> > +#endif
>> >  	LREG	x1, 1*REGBYTES(sp)
>> >  	LREG	x2, 2*REGBYTES(sp)
>> >  	LREG	x3, 3*REGBYTES(sp)
>> > @@ -279,4 +308,8 @@ trap_entry:
>> >  	LREG	x30, 30*REGBYTES(sp)
>> >  	LREG	x31, 31*REGBYTES(sp)
>> >  	addi	sp, sp, 32*REGBYTES
>> > +#ifdef CONFIG_RISCV_SMODE
>> > +	sret
>> > +#else
>> >  	mret
>> > +#endif
Anup Patel Nov. 22, 2018, 4 a.m. UTC | #6 
On Wed, Nov 21, 2018 at 9:58 PM Palmer Dabbelt <palmer@sifive.com> wrote:
>
> On Tue, 20 Nov 2018 19:41:10 PST (-0800), anup@brainfault.org wrote:
> > This patch adds kconfig option RISCV_SMODE to run u-boot in
> > S-mode. When this opition is enabled we use s<xyz> CSRs instead
> > of m<xyz> CSRs.
> >
> > It is important to note that there is no equivalent S-mode CSR
> > for misa and mhartid CSRs so we expect M-mode runtime firmware
> > (BBL or equivalent) to emulate misa and mhartid CSR read.
> >
> > In-future, we will have more patches to avoid accessing misa and
> > mhartid CSRs from S-mode.
>
> Ya, I don't like this.  Our current boot protocol puts mhartid in a1 upon
> entering the supervisor, and allows the detection of misa via a device tree
> pointer provided in a0.
>
> As long as everyone agrees this isn't what we're actually looking for then I'm
> fine with the patch, I just don't want to end up requiring this "M mode
> emulates some CSRs for S mode" interface.  Since we don't have an actual
> platform spec we've got to be careful to avoid a bunch of defacto interfaces
> that we'll need to support later.

Yes, it's a temporary thingy to emulate misa and mhartid in BBL.

Eventually, U-Boot will be fixed to avoid direct access of misa and mhartid.

Regards,
Anup
Anup Patel Nov. 22, 2018, 4 a.m. UTC | #7 
On Thu, Nov 22, 2018 at 3:41 AM Auer, Lukas
<lukas.auer@aisec.fraunhofer.de> wrote:
>
> Hi Anup,
>
> On Wed, 2018-11-21 at 13:47 +0000, Auer, Lukas wrote:
> > Hi Anup,
> >
> > On Wed, 2018-11-21 at 09:11 +0530, Anup Patel wrote:
> > > This patch adds kconfig option RISCV_SMODE to run u-boot in
> >
> > nit: U-Boot (also in the first line of the commit message)
> >
> > > S-mode. When this opition is enabled we use s<xyz> CSRs instead
> > > of m<xyz> CSRs.
> > >
>
> Can you also modify arch/riscv/lib/interrupts.c to test for the
> supervisor instead of the machine timer and external interrupts when
> CONFIG_RISCV_SMODE is set?
>

Sure, will do.

Regards,
Anup
Bin Meng Nov. 22, 2018, 2:36 p.m. UTC | #8 
Hi Palmer,

On Thu, Nov 22, 2018 at 12:28 AM Palmer Dabbelt <palmer@sifive.com> wrote:
>
> On Tue, 20 Nov 2018 19:41:10 PST (-0800), anup@brainfault.org wrote:
> > This patch adds kconfig option RISCV_SMODE to run u-boot in
> > S-mode. When this opition is enabled we use s<xyz> CSRs instead
> > of m<xyz> CSRs.
> >
> > It is important to note that there is no equivalent S-mode CSR
> > for misa and mhartid CSRs so we expect M-mode runtime firmware
> > (BBL or equivalent) to emulate misa and mhartid CSR read.
> >
> > In-future, we will have more patches to avoid accessing misa and
> > mhartid CSRs from S-mode.
>
> Ya, I don't like this.  Our current boot protocol puts mhartid in a1 upon
> entering the supervisor, and allows the detection of misa via a device tree
> pointer provided in a0.
>

As Anup mentioned in the commit message, the read of mhartid in
*S-mode* should be fixed in future patches. We still need it for
M-mode.

> As long as everyone agrees this isn't what we're actually looking for then I'm
> fine with the patch, I just don't want to end up requiring this "M mode
> emulates some CSRs for S mode" interface.  Since we don't have an actual
> platform spec we've got to be careful to avoid a bunch of defacto interfaces
> that we'll need to support later.
>

Agreed.

Regards,
Bin
diff mbox series

Patch

diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
index 3e0af55e71..8f2139ff60 100644
--- a/arch/riscv/Kconfig
+++ b/arch/riscv/Kconfig
@@ -55,6 +55,11 @@  config RISCV_ISA_C
 config RISCV_ISA_A
 	def_bool y
 
+config RISCV_SMODE
+	bool "Run in S-Mode"
+	help
+	  Enable this option to build an U-Boot for RISC-V S-Mode
+
 config 32BIT
 	bool
 
diff --git a/arch/riscv/cpu/start.S b/arch/riscv/cpu/start.S
index 5af189b338..e4276e8e19 100644
--- a/arch/riscv/cpu/start.S
+++ b/arch/riscv/cpu/start.S
@@ -39,10 +39,18 @@  _start:
 	mv	s1, a1
 
 	la	t0, trap_entry
+#ifdef CONFIG_RISCV_SMODE
+	csrw	stvec, t0
+#else
 	csrw	mtvec, t0
+#endif
 
 	/* mask all interrupts */
+#ifdef CONFIG_RISCV_SMODE
+	csrw	sie, zero
+#else
 	csrw	mie, zero
+#endif
 
 	/* Enable cache */
 	jal	icache_enable
@@ -164,7 +172,11 @@  fix_rela_dyn:
 */
 	la	t0, trap_entry
 	add	t0, t0, t6
+#ifdef CONFIG_RISCV_SMODE
+	csrw	stvec, t0
+#else
 	csrw	mtvec, t0
+#endif
 
 clear_bss:
 	la	t0, __bss_start		/* t0 <- rel __bss_start in FLASH */
@@ -236,17 +248,34 @@  trap_entry:
 	SREG	x29, 29*REGBYTES(sp)
 	SREG	x30, 30*REGBYTES(sp)
 	SREG	x31, 31*REGBYTES(sp)
+#ifdef CONFIG_RISCV_SMODE
+	csrr	a0, scause
+	csrr	a1, sepc
+#else
 	csrr	a0, mcause
 	csrr	a1, mepc
+#endif
 	mv	a2, sp
 	jal	handle_trap
+#ifdef CONFIG_RISCV_SMODE
+	csrw	sepc, a0
+#else
 	csrw	mepc, a0
+#endif
 
+#ifdef CONFIG_RISCV_SMODE
+/*
+ * Remain in S-mode after sret
+ */
+	li	t0, SSTATUS_SPP
+	csrs	sstatus, t0
+#else
 /*
  * Remain in M-mode after mret
  */
 	li	t0, MSTATUS_MPP
 	csrs	mstatus, t0
+#endif
 	LREG	x1, 1*REGBYTES(sp)
 	LREG	x2, 2*REGBYTES(sp)
 	LREG	x3, 3*REGBYTES(sp)
@@ -279,4 +308,8 @@  trap_entry:
 	LREG	x30, 30*REGBYTES(sp)
 	LREG	x31, 31*REGBYTES(sp)
 	addi	sp, sp, 32*REGBYTES
+#ifdef CONFIG_RISCV_SMODE
+	sret
+#else
 	mret
+#endif