On Fri, Nov 30, 2018 at 12:34 PM Rick Chen rickchen36@gmail.com wrote:
Anup Patel anup@brainfault.org 於 2018年11月27日 週二 下午8:38寫道：
On Tue, Nov 27, 2018 at 4:17 PM Alexander Graf agraf@suse.de wrote:
On 27.11.18 07:52, Anup Patel wrote:
On Tue, Nov 27, 2018 at 12:09 PM Rick Chen rickchen36@gmail.com wrote:
> Subject: [PATCH v5 1/4] riscv: Add kconfig option to run U-Boot in S-mode > > 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 > Reviewed-by: Lukas Auer lukas.auer@aisec.fraunhofer.de > --- > arch/riscv/Kconfig | 5 +++++ > arch/riscv/cpu/start.S | 33 ++++++++++++++++++++++++++++ > arch/riscv/include/asm/encoding.h | 2 ++ > arch/riscv/lib/interrupts.c | 36 +++++++++++++++++++++++-------- > 4 files changed, 67 insertions(+), 9 deletions(-) > > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig index 3e0af55e71..732a357a99 > 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 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 > 15e1b8199a..704190f946 100644 > --- a/arch/riscv/cpu/start.S > +++ b/arch/riscv/cpu/start.S > @@ -41,10 +41,18 @@ _start: > li t0, CONFIG_SYS_SDRAM_BASE > SREG a2, 0(t0) > 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 > @@ -166,7 +174,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 */ > @@ -238,17 +250,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
It only the difference between s and m in csr. The code seem to be duplicate too much. Can you implement it in macro ? or consider to separate it in different .S file
It may too complex to maintain in the future.
Here are few reasons why not to do this:
- We don't have same set of CSRs for M-mode and S-mode. For
certain, M-mode CSRs we don't have any corresponding S-mode CSRs. 2. Number of CSRs for each mode are really few so there won't be much #ifdef in code. Having separate .S will be total overkill and too much code duplication. 3. Using macro would mean we use incomplete CSR names examples: "status" for "mstatus"/"sstatus", "epc" for "mepc"/"sepc", etc. This means we cannot grep code for use of a CSR. I think this is less readable compared to using #ifdef
Despite above reasons, above can always be attempted as separate patch.
Hi Anup
I mean it is not a good way to switch s and m mode like that #ifdef CONFIG_RISCV_SMODE csrw sepc, a0 #else csrw mepc, a0 #endif
You can use ## to get stvec ot mtvec in different CONFIG It can be refered to some kernel code as below: glue.h #define ____glue(name,fn) name##fn #define __glue(name,fn) ____glue(name,fn)
glue-proc.h #ifdef CONFIG_CPU_ARM720T #define CPU_NAME cpu_arm720 #else #define CPU_NAME cpu_arm7tdmi #endif
#define cpu_proc_init __glue(CPU_NAME,_proc_init)
Then It will compile as cpu_arm720_proc_init or cpu_arm7tdmi_proc_init in different configuration. stvec and mtvce can be implement by the same way.
I had understood your suggestion previously.
Though I am not fan of this approach, I will change it anyway.
Thanks, Anup