[U-Boot] [PATCH v5 1/4] riscv: Add kconfig option to run U-Boot in S-mode

[Alexander Graf]

On 27.11.18 07:52, Anup Patel wrote:
> On Tue, Nov 27, 2018 at 12:09 PM Rick Chen <rickchen36 at 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 at brainfault.org>
>>>> Reviewed-by: Bin Meng <bmeng.cn at gmail.com>
>>>> Tested-by: Bin Meng <bmeng.cn at gmail.com>
>>>> Reviewed-by: Lukas Auer <lukas.auer at 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:
> 1. 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.

On AArch64, we determine the current EL during runtime and then branch
to respective labels for different ELs (see the switch_el macro for asm
as well as current_el() for C).

Wouldn't it make sense to attempt something similar with S-mode, so that
the same binary can run either in M or in S depending on how it was started?


Alex