| Message ID | 20240616170553.2832-4-jszhang@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | riscv: convert bottom half of exception handling to C | expand |
On Mon, Jun 17, 2024 at 01:05:50AM +0800, Jisheng Zhang wrote: >For readability, maintainability and future scalability, convert the >bottom half of the exception handling to C. > >Mostly the assembly code is converted to C in a relatively >straightforward manner. > >However, there are two modifications I need to mention: > >1. the CSR_CAUSE reg reading and saving is moved to the C code >because we need the cause to dispatch the exception handling, >if we keep the cause reading and saving, we either pass it to >do_traps() via. 2nd param or get it from pt_regs which an extra >memory load is needed, I don't like any of the two solutions becase >the exception handling sits in hot code path, every instruction >matters. CC: Clement. I think its better to save away cause in pt_regs prior to calling `do_traps`. Once control is transferred to C code in `do_traps`, another trap can happen. It's a problem anyways today without CPU support. Although with Ssdbltrp [1] extension and it kernel support [2] for it, I expect asm code would clear up `SDT` bit in mstatus. Whenever `Ssdbltrp` lands, I think `do_traps` should expect nesting of traps and thus cause should be saved away before it gets control so that safely traps can be nested. [1] - https://github.com/riscv/riscv-double-trap/releases/download/v1.0-rc1/riscv-double-trap.pdf [2] - https://lore.kernel.org/all/20240418133916.1442471-1-cleger@rivosinc.com/ > >2.To cope with SIFIVE_CIP_453 errata, it looks like we don't need >alternative mechanism any more after the asm->c convertion. Just >replace the excp_vect_table two entries. > >Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
On Thu, Jun 20, 2024 at 3:04 AM Deepak Gupta <debug@rivosinc.com> wrote: > > On Mon, Jun 17, 2024 at 01:05:50AM +0800, Jisheng Zhang wrote: > >For readability, maintainability and future scalability, convert the > >bottom half of the exception handling to C. > > > >Mostly the assembly code is converted to C in a relatively > >straightforward manner. > > > >However, there are two modifications I need to mention: > > > >1. the CSR_CAUSE reg reading and saving is moved to the C code > >because we need the cause to dispatch the exception handling, > >if we keep the cause reading and saving, we either pass it to > >do_traps() via. 2nd param or get it from pt_regs which an extra > >memory load is needed, I don't like any of the two solutions becase > >the exception handling sits in hot code path, every instruction > >matters. > > CC: Clement. > > I think its better to save away cause in pt_regs prior to calling > `do_traps`. Once control is transferred to C code in `do_traps`, > another trap can happen. It's a problem anyways today without CPU support. > > Although with Ssdbltrp [1] extension and it kernel support [2] for it, > I expect asm code would clear up `SDT` bit in mstatus. Whenever `Ssdbltrp` lands, > I think `do_traps` should expect nesting of traps and thus cause should be saved > away before it gets control so that safely traps can be nested. > Is a possible solution to do both options Jisheng suggested? Save the cause before calling do_traps but also pass it via second param? > [1] - https://github.com/riscv/riscv-double-trap/releases/download/v1.0-rc1/riscv-double-trap.pdf > [2] - https://lore.kernel.org/all/20240418133916.1442471-1-cleger@rivosinc.com/ > > > > >2.To cope with SIFIVE_CIP_453 errata, it looks like we don't need > >alternative mechanism any more after the asm->c convertion. Just > >replace the excp_vect_table two entries. > > > >Signed-off-by: Jisheng Zhang <jszhang@kernel.org> > > _______________________________________________ > linux-riscv mailing list > linux-riscv@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-riscv
On 20/06/2024 02:02, Cyril Bur wrote: > On Thu, Jun 20, 2024 at 3:04 AM Deepak Gupta <debug@rivosinc.com> wrote: >> >> On Mon, Jun 17, 2024 at 01:05:50AM +0800, Jisheng Zhang wrote: >>> For readability, maintainability and future scalability, convert the >>> bottom half of the exception handling to C. >>> >>> Mostly the assembly code is converted to C in a relatively >>> straightforward manner. >>> >>> However, there are two modifications I need to mention: >>> >>> 1. the CSR_CAUSE reg reading and saving is moved to the C code >>> because we need the cause to dispatch the exception handling, >>> if we keep the cause reading and saving, we either pass it to >>> do_traps() via. 2nd param or get it from pt_regs which an extra >>> memory load is needed, I don't like any of the two solutions becase >>> the exception handling sits in hot code path, every instruction >>> matters. >> >> CC: Clement. >> >> I think its better to save away cause in pt_regs prior to calling >> `do_traps`. Once control is transferred to C code in `do_traps`, >> another trap can happen. It's a problem anyways today without CPU support. >> >> Although with Ssdbltrp [1] extension and it kernel support [2] for it, >> I expect asm code would clear up `SDT` bit in mstatus. Whenever `Ssdbltrp` lands, >> I think `do_traps` should expect nesting of traps and thus cause should be saved >> away before it gets control so that safely traps can be nested. Hi, Indeed, every register that is "unique" to a trap and than can be overwritten by a second trap should be saved before reenabling them when using Ssdbltrp. So that would be nice to preserve that. >> > > Is a possible solution to do both options Jisheng suggested? Save the > cause before > calling do_traps but also pass it via second param? I guess so if it fits your performance requirements. Thanks, Clément > >> [1] - https://github.com/riscv/riscv-double-trap/releases/download/v1.0-rc1/riscv-double-trap.pdf >> [2] - https://lore.kernel.org/all/20240418133916.1442471-1-cleger@rivosinc.com/ >> >>> >>> 2.To cope with SIFIVE_CIP_453 errata, it looks like we don't need >>> alternative mechanism any more after the asm->c convertion. Just >>> replace the excp_vect_table two entries. >>> >>> Signed-off-by: Jisheng Zhang <jszhang@kernel.org> >> >> _______________________________________________ >> linux-riscv mailing list >> linux-riscv@lists.infradead.org >> http://lists.infradead.org/mailman/listinfo/linux-riscv
On 17/6/2024 3:05 am, Jisheng Zhang wrote: > For readability, maintainability and future scalability, convert the > bottom half of the exception handling to C. > > Mostly the assembly code is converted to C in a relatively > straightforward manner. > > However, there are two modifications I need to mention: > > 1. the CSR_CAUSE reg reading and saving is moved to the C code > because we need the cause to dispatch the exception handling, > if we keep the cause reading and saving, we either pass it to > do_traps() via. 2nd param or get it from pt_regs which an extra > memory load is needed, I don't like any of the two solutions becase > the exception handling sits in hot code path, every instruction > matters. > > 2.To cope with SIFIVE_CIP_453 errata, it looks like we don't need > alternative mechanism any more after the asm->c convertion. Just > replace the excp_vect_table two entries. > Hi Jisheng, Sorry I've been having email client problems. This email if from a few days ago. I really like the look of the patch. I just have one question. > Signed-off-by: Jisheng Zhang <jszhang@kernel.org> > --- > arch/riscv/errata/sifive/errata.c | 25 ++++++++--- > arch/riscv/include/asm/asm-prototypes.h | 1 + > arch/riscv/include/asm/errata_list.h | 5 +-- > arch/riscv/kernel/entry.S | 58 +------------------------ > arch/riscv/kernel/traps.c | 41 +++++++++++++++++ > 5 files changed, 64 insertions(+), 66 deletions(-) > > diff --git a/arch/riscv/errata/sifive/errata.c b/arch/riscv/errata/sifive/errata.c > index 716cfedad3a2..bbba99f207ca 100644 > --- a/arch/riscv/errata/sifive/errata.c > +++ b/arch/riscv/errata/sifive/errata.c > @@ -10,9 +10,14 @@ > #include <linux/bug.h> > #include <asm/patch.h> > #include <asm/alternative.h> > +#include <asm/csr.h> > #include <asm/vendorid_list.h> > #include <asm/errata_list.h> > > +extern void (*excp_vect_table[])(struct pt_regs *regs); > +extern void sifive_cip_453_insn_fault_trp(struct pt_regs *regs); > +extern void sifive_cip_453_page_fault_trp(struct pt_regs *regs); > + > struct errata_info_t { > char name[32]; > bool (*check_func)(unsigned long arch_id, unsigned long impid); > @@ -20,6 +25,9 @@ struct errata_info_t { > > static bool errata_cip_453_check_func(unsigned long arch_id, unsigned long impid) > { > + if (!IS_ENABLED(CONFIG_ERRATA_SIFIVE_CIP_453)) > + return false; > + > /* > * Affected cores: > * Architecture ID: 0x8000000000000007 > @@ -51,10 +59,6 @@ static bool errata_cip_1200_check_func(unsigned long arch_id, unsigned long imp > } > > static struct errata_info_t errata_list[ERRATA_SIFIVE_NUMBER] = { > - { > - .name = "cip-453", > - .check_func = errata_cip_453_check_func > - }, > { > .name = "cip-1200", > .check_func = errata_cip_1200_check_func > @@ -62,11 +66,20 @@ static struct errata_info_t errata_list[ERRATA_SIFIVE_NUMBER] = { > }; > > static u32 __init_or_module sifive_errata_probe(unsigned long archid, > - unsigned long impid) > + unsigned long impid, > + unsigned int stage) > { > int idx; > u32 cpu_req_errata = 0; > > + if (stage == RISCV_ALTERNATIVES_BOOT) { > + if (IS_ENABLED(CONFIG_MMU) && > + errata_cip_453_check_func(archid, impid)) { > + excp_vect_table[EXC_INST_ACCESS] = sifive_cip_453_insn_fault_trp; > + excp_vect_table[EXC_INST_PAGE_FAULT] = sifive_cip_453_page_fault_trp; > + } > + } > + > for (idx = 0; idx < ERRATA_SIFIVE_NUMBER; idx++) > if (errata_list[idx].check_func(archid, impid)) > cpu_req_errata |= (1U << idx); > @@ -99,7 +112,7 @@ void sifive_errata_patch_func(struct alt_entry *begin, struct alt_entry *end, > if (stage == RISCV_ALTERNATIVES_EARLY_BOOT) > return; > > - cpu_req_errata = sifive_errata_probe(archid, impid); > + cpu_req_errata = sifive_errata_probe(archid, impid, stage); > > for (alt = begin; alt < end; alt++) { > if (alt->vendor_id != SIFIVE_VENDOR_ID) > diff --git a/arch/riscv/include/asm/asm-prototypes.h b/arch/riscv/include/asm/asm-prototypes.h > index cd627ec289f1..81a1f27fa54f 100644 > --- a/arch/riscv/include/asm/asm-prototypes.h > +++ b/arch/riscv/include/asm/asm-prototypes.h > @@ -55,5 +55,6 @@ DECLARE_DO_ERROR_INFO(do_trap_break); > asmlinkage void handle_bad_stack(struct pt_regs *regs); > asmlinkage void do_page_fault(struct pt_regs *regs); > asmlinkage void do_irq(struct pt_regs *regs); > +asmlinkage void do_traps(struct pt_regs *regs); > > #endif /* _ASM_RISCV_PROTOTYPES_H */ > diff --git a/arch/riscv/include/asm/errata_list.h b/arch/riscv/include/asm/errata_list.h > index 7c8a71a526a3..95b79afc4061 100644 > --- a/arch/riscv/include/asm/errata_list.h > +++ b/arch/riscv/include/asm/errata_list.h > @@ -17,9 +17,8 @@ > #endif > > #ifdef CONFIG_ERRATA_SIFIVE > -#define ERRATA_SIFIVE_CIP_453 0 > -#define ERRATA_SIFIVE_CIP_1200 1 > -#define ERRATA_SIFIVE_NUMBER 2 > +#define ERRATA_SIFIVE_CIP_1200 0 > +#define ERRATA_SIFIVE_NUMBER 1 > #endif > > #ifdef CONFIG_ERRATA_THEAD > diff --git a/arch/riscv/kernel/entry.S b/arch/riscv/kernel/entry.S > index 81dec627a8d4..401bfe85a098 100644 > --- a/arch/riscv/kernel/entry.S > +++ b/arch/riscv/kernel/entry.S > @@ -62,13 +62,11 @@ SYM_CODE_START(handle_exception) > csrrc s1, CSR_STATUS, t0 > csrr s2, CSR_EPC > csrr s3, CSR_TVAL > - csrr s4, CSR_CAUSE > csrr s5, CSR_SCRATCH > REG_S s0, PT_SP(sp) > REG_S s1, PT_STATUS(sp) > REG_S s2, PT_EPC(sp) > REG_S s3, PT_BADADDR(sp) > - REG_S s4, PT_CAUSE(sp) > REG_S s5, PT_TP(sp) > > /* > @@ -83,36 +81,9 @@ SYM_CODE_START(handle_exception) > /* Load the kernel shadow call stack pointer if coming from userspace */ > scs_load_current_if_task_changed s5 > > -#ifdef CONFIG_RISCV_ISA_V_PREEMPTIVE > - move a0, sp > - call riscv_v_context_nesting_start > -#endif Along with removing this, can the asmlinkage in asm-prototypes.h be removed? Or are you keeping the _start() in because the _end() needs to stay? That leads me to think about leaving the call to riscv_context_nesting_start() in asm here for the symmetry of _start() and _end() being called from entry.S. > move a0, sp /* pt_regs */ > - > - /* > - * MSB of cause differentiates between > - * interrupts and exceptions > - */ > - bge s4, zero, 1f > - > - /* Handle interrupts */ > - call do_irq > - j ret_from_exception > -1: > - /* Handle other exceptions */ > - slli t0, s4, RISCV_LGPTR > - la t1, excp_vect_table > - la t2, excp_vect_table_end > - add t0, t1, t0 > - /* Check if exception code lies within bounds */ > - bgeu t0, t2, 3f > - REG_L t1, 0(t0) > -2: jalr t1 > + call do_traps > j ret_from_exception > -3: > - > - la t1, do_trap_unknown > - j 2b > SYM_CODE_END(handle_exception) > ASM_NOKPROBE(handle_exception) > > @@ -329,33 +300,6 @@ SYM_FUNC_START(__switch_to) > ret > SYM_FUNC_END(__switch_to) > > -#ifndef CONFIG_MMU > -#define do_page_fault do_trap_unknown > -#endif > - > - .section ".rodata" > - .align LGREG > - /* Exception vector table */ > -SYM_DATA_START_LOCAL(excp_vect_table) > - RISCV_PTR do_trap_insn_misaligned > - ALT_INSN_FAULT(RISCV_PTR do_trap_insn_fault) > - RISCV_PTR do_trap_insn_illegal > - RISCV_PTR do_trap_break > - RISCV_PTR do_trap_load_misaligned > - RISCV_PTR do_trap_load_fault > - RISCV_PTR do_trap_store_misaligned > - RISCV_PTR do_trap_store_fault > - RISCV_PTR do_trap_ecall_u /* system call */ > - RISCV_PTR do_trap_ecall_s > - RISCV_PTR do_trap_unknown > - RISCV_PTR do_trap_ecall_m > - /* instruciton page fault */ > - ALT_PAGE_FAULT(RISCV_PTR do_page_fault) > - RISCV_PTR do_page_fault /* load page fault */ > - RISCV_PTR do_trap_unknown > - RISCV_PTR do_page_fault /* store page fault */ > -SYM_DATA_END_LABEL(excp_vect_table, SYM_L_LOCAL, excp_vect_table_end) > - > #ifndef CONFIG_MMU > SYM_DATA_START(__user_rt_sigreturn) > li a7, __NR_rt_sigreturn > diff --git a/arch/riscv/kernel/traps.c b/arch/riscv/kernel/traps.c > index 05a16b1f0aee..b44d4a8d4083 100644 > --- a/arch/riscv/kernel/traps.c > +++ b/arch/riscv/kernel/traps.c > @@ -390,6 +390,47 @@ asmlinkage void noinstr do_irq(struct pt_regs *regs) > irqentry_exit(regs, state); > } > > +void (*excp_vect_table[])(struct pt_regs *regs) __ro_after_init = { > + do_trap_insn_misaligned, /* 0 Instruction address misaligned */ > + do_trap_insn_fault, /* 1 Instruction access fault */ > + do_trap_insn_illegal, /* 2 Illegal instruction */ > + do_trap_break, /* 3 Breakpoint */ > + do_trap_load_misaligned, /* 4 Load address misaligned */ > + do_trap_load_fault, /* 5 Load access fault */ > + do_trap_store_misaligned, /* 6 Store/AMO address misaligned */ > + do_trap_store_fault, /* 7 Store/AMO access fault */ > + do_trap_ecall_u, /* 8 Environment call from U-mode */ > + do_trap_ecall_s, /* 9 Environment call from S-mode */ > + do_trap_unknown, /* 10 Reserved */ > + do_trap_ecall_m, /* 11 Environment call from M-mode */ > +#ifdef CONFIG_MMU > + do_page_fault, /* 12 Instruciton page fault */ > + do_page_fault, /* 13 Load page fault */ > + do_trap_unknown, /* 14 Reserved */ > + do_page_fault, /* 15 Store/AMO page fault */ > +#endif > +}; > + > +asmlinkage void noinstr do_traps(struct pt_regs *regs) > +{ > + unsigned long cause = csr_read(CSR_CAUSE); > + > + regs->cause = cause; > + > +#ifdef CONFIG_RISCV_ISA_V_PREEMPTIVE > + riscv_v_context_nesting_start(regs); > +#endif > + if (cause & CAUSE_IRQ_FLAG) { > + do_irq(regs); > + } else { > + if (cause >= ARRAY_SIZE(excp_vect_table)) { > + do_trap_unknown(regs); > + return; > + } > + excp_vect_table[cause](regs); > + } > +} > + > #ifdef CONFIG_GENERIC_BUG > int is_valid_bugaddr(unsigned long pc) > {
On Fri, Jun 21, 2024 at 09:10:11AM +1000, Cyril Bur wrote: > > > On 17/6/2024 3:05 am, Jisheng Zhang wrote: > > For readability, maintainability and future scalability, convert the > > bottom half of the exception handling to C. > > > > Mostly the assembly code is converted to C in a relatively > > straightforward manner. > > > > However, there are two modifications I need to mention: > > > > 1. the CSR_CAUSE reg reading and saving is moved to the C code > > because we need the cause to dispatch the exception handling, > > if we keep the cause reading and saving, we either pass it to > > do_traps() via. 2nd param or get it from pt_regs which an extra > > memory load is needed, I don't like any of the two solutions becase > > the exception handling sits in hot code path, every instruction > > matters. > > > > 2.To cope with SIFIVE_CIP_453 errata, it looks like we don't need > > alternative mechanism any more after the asm->c convertion. Just > > replace the excp_vect_table two entries. > > > > Hi Jisheng, Hi Cyril, > > Sorry I've been having email client problems. This email if from a few days > ago. > > I really like the look of the patch. I just have one question. thanks! > > > Signed-off-by: Jisheng Zhang <jszhang@kernel.org> > > --- > > arch/riscv/errata/sifive/errata.c | 25 ++++++++--- > > arch/riscv/include/asm/asm-prototypes.h | 1 + > > arch/riscv/include/asm/errata_list.h | 5 +-- > > arch/riscv/kernel/entry.S | 58 +------------------------ > > arch/riscv/kernel/traps.c | 41 +++++++++++++++++ > > 5 files changed, 64 insertions(+), 66 deletions(-) > > > > diff --git a/arch/riscv/errata/sifive/errata.c b/arch/riscv/errata/sifive/errata.c > > index 716cfedad3a2..bbba99f207ca 100644 > > --- a/arch/riscv/errata/sifive/errata.c > > +++ b/arch/riscv/errata/sifive/errata.c > > @@ -10,9 +10,14 @@ > > #include <linux/bug.h> > > #include <asm/patch.h> > > #include <asm/alternative.h> > > +#include <asm/csr.h> > > #include <asm/vendorid_list.h> > > #include <asm/errata_list.h> > > +extern void (*excp_vect_table[])(struct pt_regs *regs); > > +extern void sifive_cip_453_insn_fault_trp(struct pt_regs *regs); > > +extern void sifive_cip_453_page_fault_trp(struct pt_regs *regs); > > + > > struct errata_info_t { > > char name[32]; > > bool (*check_func)(unsigned long arch_id, unsigned long impid); > > @@ -20,6 +25,9 @@ struct errata_info_t { > > static bool errata_cip_453_check_func(unsigned long arch_id, unsigned long impid) > > { > > + if (!IS_ENABLED(CONFIG_ERRATA_SIFIVE_CIP_453)) > > + return false; > > + > > /* > > * Affected cores: > > * Architecture ID: 0x8000000000000007 > > @@ -51,10 +59,6 @@ static bool errata_cip_1200_check_func(unsigned long arch_id, unsigned long imp > > } > > static struct errata_info_t errata_list[ERRATA_SIFIVE_NUMBER] = { > > - { > > - .name = "cip-453", > > - .check_func = errata_cip_453_check_func > > - }, > > { > > .name = "cip-1200", > > .check_func = errata_cip_1200_check_func > > @@ -62,11 +66,20 @@ static struct errata_info_t errata_list[ERRATA_SIFIVE_NUMBER] = { > > }; > > static u32 __init_or_module sifive_errata_probe(unsigned long archid, > > - unsigned long impid) > > + unsigned long impid, > > + unsigned int stage) > > { > > int idx; > > u32 cpu_req_errata = 0; > > + if (stage == RISCV_ALTERNATIVES_BOOT) { > > + if (IS_ENABLED(CONFIG_MMU) && > > + errata_cip_453_check_func(archid, impid)) { > > + excp_vect_table[EXC_INST_ACCESS] = sifive_cip_453_insn_fault_trp; > > + excp_vect_table[EXC_INST_PAGE_FAULT] = sifive_cip_453_page_fault_trp; > > + } > > + } > > + > > for (idx = 0; idx < ERRATA_SIFIVE_NUMBER; idx++) > > if (errata_list[idx].check_func(archid, impid)) > > cpu_req_errata |= (1U << idx); > > @@ -99,7 +112,7 @@ void sifive_errata_patch_func(struct alt_entry *begin, struct alt_entry *end, > > if (stage == RISCV_ALTERNATIVES_EARLY_BOOT) > > return; > > - cpu_req_errata = sifive_errata_probe(archid, impid); > > + cpu_req_errata = sifive_errata_probe(archid, impid, stage); > > for (alt = begin; alt < end; alt++) { > > if (alt->vendor_id != SIFIVE_VENDOR_ID) > > diff --git a/arch/riscv/include/asm/asm-prototypes.h b/arch/riscv/include/asm/asm-prototypes.h > > index cd627ec289f1..81a1f27fa54f 100644 > > --- a/arch/riscv/include/asm/asm-prototypes.h > > +++ b/arch/riscv/include/asm/asm-prototypes.h > > @@ -55,5 +55,6 @@ DECLARE_DO_ERROR_INFO(do_trap_break); > > asmlinkage void handle_bad_stack(struct pt_regs *regs); > > asmlinkage void do_page_fault(struct pt_regs *regs); > > asmlinkage void do_irq(struct pt_regs *regs); > > +asmlinkage void do_traps(struct pt_regs *regs); > > #endif /* _ASM_RISCV_PROTOTYPES_H */ > > diff --git a/arch/riscv/include/asm/errata_list.h b/arch/riscv/include/asm/errata_list.h > > index 7c8a71a526a3..95b79afc4061 100644 > > --- a/arch/riscv/include/asm/errata_list.h > > +++ b/arch/riscv/include/asm/errata_list.h > > @@ -17,9 +17,8 @@ > > #endif > > #ifdef CONFIG_ERRATA_SIFIVE > > -#define ERRATA_SIFIVE_CIP_453 0 > > -#define ERRATA_SIFIVE_CIP_1200 1 > > -#define ERRATA_SIFIVE_NUMBER 2 > > +#define ERRATA_SIFIVE_CIP_1200 0 > > +#define ERRATA_SIFIVE_NUMBER 1 > > #endif > > #ifdef CONFIG_ERRATA_THEAD > > diff --git a/arch/riscv/kernel/entry.S b/arch/riscv/kernel/entry.S > > index 81dec627a8d4..401bfe85a098 100644 > > --- a/arch/riscv/kernel/entry.S > > +++ b/arch/riscv/kernel/entry.S > > @@ -62,13 +62,11 @@ SYM_CODE_START(handle_exception) > > csrrc s1, CSR_STATUS, t0 > > csrr s2, CSR_EPC > > csrr s3, CSR_TVAL > > - csrr s4, CSR_CAUSE > > csrr s5, CSR_SCRATCH > > REG_S s0, PT_SP(sp) > > REG_S s1, PT_STATUS(sp) > > REG_S s2, PT_EPC(sp) > > REG_S s3, PT_BADADDR(sp) > > - REG_S s4, PT_CAUSE(sp) > > REG_S s5, PT_TP(sp) > > /* > > @@ -83,36 +81,9 @@ SYM_CODE_START(handle_exception) > > /* Load the kernel shadow call stack pointer if coming from userspace */ > > scs_load_current_if_task_changed s5 > > -#ifdef CONFIG_RISCV_ISA_V_PREEMPTIVE > > - move a0, sp > > - call riscv_v_context_nesting_start > > -#endif > Along with removing this, can the asmlinkage in asm-prototypes.h be removed? the asmlinkage is removed in patch6 of the series ;) > Or are you keeping the _start() in because the _end() needs to stay? > > That leads me to think about leaving the call to > riscv_context_nesting_start() in asm here for the symmetry of _start() and > _end() being called from entry.S. seems a good idea, symmetry matters. will do in new version > > > move a0, sp /* pt_regs */ > > - > > - /* > > - * MSB of cause differentiates between > > - * interrupts and exceptions > > - */ > > - bge s4, zero, 1f > > - > > - /* Handle interrupts */ > > - call do_irq > > - j ret_from_exception > > -1: > > - /* Handle other exceptions */ > > - slli t0, s4, RISCV_LGPTR > > - la t1, excp_vect_table > > - la t2, excp_vect_table_end > > - add t0, t1, t0 > > - /* Check if exception code lies within bounds */ > > - bgeu t0, t2, 3f > > - REG_L t1, 0(t0) > > -2: jalr t1 > > + call do_traps > > j ret_from_exception > > -3: > > - > > - la t1, do_trap_unknown > > - j 2b > > SYM_CODE_END(handle_exception) > > ASM_NOKPROBE(handle_exception) > > @@ -329,33 +300,6 @@ SYM_FUNC_START(__switch_to) > > ret > > SYM_FUNC_END(__switch_to) > > -#ifndef CONFIG_MMU > > -#define do_page_fault do_trap_unknown > > -#endif > > - > > - .section ".rodata" > > - .align LGREG > > - /* Exception vector table */ > > -SYM_DATA_START_LOCAL(excp_vect_table) > > - RISCV_PTR do_trap_insn_misaligned > > - ALT_INSN_FAULT(RISCV_PTR do_trap_insn_fault) > > - RISCV_PTR do_trap_insn_illegal > > - RISCV_PTR do_trap_break > > - RISCV_PTR do_trap_load_misaligned > > - RISCV_PTR do_trap_load_fault > > - RISCV_PTR do_trap_store_misaligned > > - RISCV_PTR do_trap_store_fault > > - RISCV_PTR do_trap_ecall_u /* system call */ > > - RISCV_PTR do_trap_ecall_s > > - RISCV_PTR do_trap_unknown > > - RISCV_PTR do_trap_ecall_m > > - /* instruciton page fault */ > > - ALT_PAGE_FAULT(RISCV_PTR do_page_fault) > > - RISCV_PTR do_page_fault /* load page fault */ > > - RISCV_PTR do_trap_unknown > > - RISCV_PTR do_page_fault /* store page fault */ > > -SYM_DATA_END_LABEL(excp_vect_table, SYM_L_LOCAL, excp_vect_table_end) > > - > > #ifndef CONFIG_MMU > > SYM_DATA_START(__user_rt_sigreturn) > > li a7, __NR_rt_sigreturn > > diff --git a/arch/riscv/kernel/traps.c b/arch/riscv/kernel/traps.c > > index 05a16b1f0aee..b44d4a8d4083 100644 > > --- a/arch/riscv/kernel/traps.c > > +++ b/arch/riscv/kernel/traps.c > > @@ -390,6 +390,47 @@ asmlinkage void noinstr do_irq(struct pt_regs *regs) > > irqentry_exit(regs, state); > > } > > +void (*excp_vect_table[])(struct pt_regs *regs) __ro_after_init = { > > + do_trap_insn_misaligned, /* 0 Instruction address misaligned */ > > + do_trap_insn_fault, /* 1 Instruction access fault */ > > + do_trap_insn_illegal, /* 2 Illegal instruction */ > > + do_trap_break, /* 3 Breakpoint */ > > + do_trap_load_misaligned, /* 4 Load address misaligned */ > > + do_trap_load_fault, /* 5 Load access fault */ > > + do_trap_store_misaligned, /* 6 Store/AMO address misaligned */ > > + do_trap_store_fault, /* 7 Store/AMO access fault */ > > + do_trap_ecall_u, /* 8 Environment call from U-mode */ > > + do_trap_ecall_s, /* 9 Environment call from S-mode */ > > + do_trap_unknown, /* 10 Reserved */ > > + do_trap_ecall_m, /* 11 Environment call from M-mode */ > > +#ifdef CONFIG_MMU > > + do_page_fault, /* 12 Instruciton page fault */ > > + do_page_fault, /* 13 Load page fault */ > > + do_trap_unknown, /* 14 Reserved */ > > + do_page_fault, /* 15 Store/AMO page fault */ > > +#endif > > +}; > > + > > +asmlinkage void noinstr do_traps(struct pt_regs *regs) > > +{ > > + unsigned long cause = csr_read(CSR_CAUSE); > > + > > + regs->cause = cause; > > + > > +#ifdef CONFIG_RISCV_ISA_V_PREEMPTIVE > > + riscv_v_context_nesting_start(regs); > > +#endif > > + if (cause & CAUSE_IRQ_FLAG) { > > + do_irq(regs); > > + } else { > > + if (cause >= ARRAY_SIZE(excp_vect_table)) { > > + do_trap_unknown(regs); > > + return; > > + } > > + excp_vect_table[cause](regs); > > + } > > +} > > + > > #ifdef CONFIG_GENERIC_BUG > > int is_valid_bugaddr(unsigned long pc) > > {
On Thu, Jun 20, 2024 at 10:06:15AM +0200, Clément Léger wrote: > > > On 20/06/2024 02:02, Cyril Bur wrote: > > On Thu, Jun 20, 2024 at 3:04 AM Deepak Gupta <debug@rivosinc.com> wrote: > >> > >> On Mon, Jun 17, 2024 at 01:05:50AM +0800, Jisheng Zhang wrote: > >>> For readability, maintainability and future scalability, convert the > >>> bottom half of the exception handling to C. > >>> > >>> Mostly the assembly code is converted to C in a relatively > >>> straightforward manner. > >>> > >>> However, there are two modifications I need to mention: > >>> > >>> 1. the CSR_CAUSE reg reading and saving is moved to the C code > >>> because we need the cause to dispatch the exception handling, > >>> if we keep the cause reading and saving, we either pass it to > >>> do_traps() via. 2nd param or get it from pt_regs which an extra > >>> memory load is needed, I don't like any of the two solutions becase > >>> the exception handling sits in hot code path, every instruction > >>> matters. > >> > >> CC: Clement. > >> > >> I think its better to save away cause in pt_regs prior to calling > >> `do_traps`. Once control is transferred to C code in `do_traps`, > >> another trap can happen. It's a problem anyways today without CPU support. > >> > >> Although with Ssdbltrp [1] extension and it kernel support [2] for it, > >> I expect asm code would clear up `SDT` bit in mstatus. Whenever `Ssdbltrp` lands, Hi Deepak, Clément, Currently, SR_IE bit is is set(setting means enable irq) in c, could the 'SDT' bit be cleared in c as well when Ssdbltrp lands? Thanks > >> I think `do_traps` should expect nesting of traps and thus cause should be saved > >> away before it gets control so that safely traps can be nested. > > Hi, > > Indeed, every register that is "unique" to a trap and than can be > overwritten by a second trap should be saved before reenabling them when > using Ssdbltrp. So that would be nice to preserve that. > > >> > > > > Is a possible solution to do both options Jisheng suggested? Save the > > cause before > > calling do_traps but also pass it via second param? > > I guess so if it fits your performance requirements. > > Thanks, > > Clément > > > > >> [1] - https://github.com/riscv/riscv-double-trap/releases/download/v1.0-rc1/riscv-double-trap.pdf > >> [2] - https://lore.kernel.org/all/20240418133916.1442471-1-cleger@rivosinc.com/ > >> > >>> > >>> 2.To cope with SIFIVE_CIP_453 errata, it looks like we don't need > >>> alternative mechanism any more after the asm->c convertion. Just > >>> replace the excp_vect_table two entries. > >>> > >>> Signed-off-by: Jisheng Zhang <jszhang@kernel.org> > >> > >> _______________________________________________ > >> linux-riscv mailing list > >> linux-riscv@lists.infradead.org > >> http://lists.infradead.org/mailman/listinfo/linux-riscv
On Fri, Jun 21, 2024 at 07:56:56AM +0800, Jisheng Zhang wrote: >On Thu, Jun 20, 2024 at 10:06:15AM +0200, Clément Léger wrote: >> >> >> On 20/06/2024 02:02, Cyril Bur wrote: >> > On Thu, Jun 20, 2024 at 3:04 AM Deepak Gupta <debug@rivosinc.com> wrote: >> >> >> >> On Mon, Jun 17, 2024 at 01:05:50AM +0800, Jisheng Zhang wrote: >> >>> For readability, maintainability and future scalability, convert the >> >>> bottom half of the exception handling to C. >> >>> >> >>> Mostly the assembly code is converted to C in a relatively >> >>> straightforward manner. >> >>> >> >>> However, there are two modifications I need to mention: >> >>> >> >>> 1. the CSR_CAUSE reg reading and saving is moved to the C code >> >>> because we need the cause to dispatch the exception handling, >> >>> if we keep the cause reading and saving, we either pass it to >> >>> do_traps() via. 2nd param or get it from pt_regs which an extra >> >>> memory load is needed, I don't like any of the two solutions becase >> >>> the exception handling sits in hot code path, every instruction >> >>> matters. >> >> >> >> CC: Clement. >> >> >> >> I think its better to save away cause in pt_regs prior to calling >> >> `do_traps`. Once control is transferred to C code in `do_traps`, >> >> another trap can happen. It's a problem anyways today without CPU support. >> >> >> >> Although with Ssdbltrp [1] extension and it kernel support [2] for it, >> >> I expect asm code would clear up `SDT` bit in mstatus. Whenever `Ssdbltrp` lands, > >Hi Deepak, Clément, > >Currently, SR_IE bit is is set(setting means enable irq) in c, could the >'SDT' bit be cleared in c as well when Ssdbltrp lands? SDT is placed in sstatus CSR. So yes its possible to clear it in C in `do_traps`. Although then you (and any future developer) will have to pay extra attention to this function because this function can be nested depending on when SDT is cleared or not. Maintainence (and introductions of error) wise it doesn't look ideal. If we keep read of `cause` in asm code and pass it as parameter to `do_traps`, it cleanly defines the boundary of which functions can be nested and which can't. It helps features like SSE [1, 2] (which expect nesting of events and had to be creative) to implement cleaner logic. [1] https://lists.riscv.org/g/tech-prs/message/515 [2] https://lpc.events/event/17/contributions/1479/attachments/1243/2526/SSE_Plumbers.pdf > >Thanks >> >> I think `do_traps` should expect nesting of traps and thus cause should be saved >> >> away before it gets control so that safely traps can be nested. >> >> Hi, >> >> Indeed, every register that is "unique" to a trap and than can be >> overwritten by a second trap should be saved before reenabling them when >> using Ssdbltrp. So that would be nice to preserve that. >> >> >> >> > >> > Is a possible solution to do both options Jisheng suggested? Save the >> > cause before >> > calling do_traps but also pass it via second param? >> >> I guess so if it fits your performance requirements. >> >> Thanks, >> >> Clément >> >> > >> >> [1] - https://github.com/riscv/riscv-double-trap/releases/download/v1.0-rc1/riscv-double-trap.pdf >> >> [2] - https://lore.kernel.org/all/20240418133916.1442471-1-cleger@rivosinc.com/ >> >> >> >>> >> >>> 2.To cope with SIFIVE_CIP_453 errata, it looks like we don't need >> >>> alternative mechanism any more after the asm->c convertion. Just >> >>> replace the excp_vect_table two entries. >> >>> >> >>> Signed-off-by: Jisheng Zhang <jszhang@kernel.org> >> >> >> >> _______________________________________________ >> >> linux-riscv mailing list >> >> linux-riscv@lists.infradead.org >> >> http://lists.infradead.org/mailman/listinfo/linux-riscv
On Mon, Jun 17, 2024 at 01:05:50AM +0800, Jisheng Zhang wrote: > For readability, maintainability and future scalability, convert the > bottom half of the exception handling to C. I would be interested to see the performance impact of this, since this mostly eliminates the code from patch 1 and replaces it in C. Anton/Cyril can you share the test case you used so that these changes can also be benchmarked? > > Mostly the assembly code is converted to C in a relatively > straightforward manner. > > However, there are two modifications I need to mention: > > 1. the CSR_CAUSE reg reading and saving is moved to the C code > because we need the cause to dispatch the exception handling, > if we keep the cause reading and saving, we either pass it to > do_traps() via. 2nd param or get it from pt_regs which an extra > memory load is needed, I don't like any of the two solutions becase > the exception handling sits in hot code path, every instruction > matters. > > 2.To cope with SIFIVE_CIP_453 errata, it looks like we don't need > alternative mechanism any more after the asm->c convertion. Just > replace the excp_vect_table two entries. > > Signed-off-by: Jisheng Zhang <jszhang@kernel.org> > --- > arch/riscv/errata/sifive/errata.c | 25 ++++++++--- > arch/riscv/include/asm/asm-prototypes.h | 1 + > arch/riscv/include/asm/errata_list.h | 5 +-- > arch/riscv/kernel/entry.S | 58 +------------------------ > arch/riscv/kernel/traps.c | 41 +++++++++++++++++ > 5 files changed, 64 insertions(+), 66 deletions(-) > > diff --git a/arch/riscv/errata/sifive/errata.c b/arch/riscv/errata/sifive/errata.c > index 716cfedad3a2..bbba99f207ca 100644 > --- a/arch/riscv/errata/sifive/errata.c > +++ b/arch/riscv/errata/sifive/errata.c > @@ -10,9 +10,14 @@ > #include <linux/bug.h> > #include <asm/patch.h> > #include <asm/alternative.h> > +#include <asm/csr.h> > #include <asm/vendorid_list.h> > #include <asm/errata_list.h> > > +extern void (*excp_vect_table[])(struct pt_regs *regs); > +extern void sifive_cip_453_insn_fault_trp(struct pt_regs *regs); > +extern void sifive_cip_453_page_fault_trp(struct pt_regs *regs); > + > struct errata_info_t { > char name[32]; > bool (*check_func)(unsigned long arch_id, unsigned long impid); > @@ -20,6 +25,9 @@ struct errata_info_t { > > static bool errata_cip_453_check_func(unsigned long arch_id, unsigned long impid) > { > + if (!IS_ENABLED(CONFIG_ERRATA_SIFIVE_CIP_453)) > + return false; > + > /* > * Affected cores: > * Architecture ID: 0x8000000000000007 > @@ -51,10 +59,6 @@ static bool errata_cip_1200_check_func(unsigned long arch_id, unsigned long imp > } > > static struct errata_info_t errata_list[ERRATA_SIFIVE_NUMBER] = { > - { > - .name = "cip-453", > - .check_func = errata_cip_453_check_func > - }, > { > .name = "cip-1200", > .check_func = errata_cip_1200_check_func > @@ -62,11 +66,20 @@ static struct errata_info_t errata_list[ERRATA_SIFIVE_NUMBER] = { > }; > > static u32 __init_or_module sifive_errata_probe(unsigned long archid, > - unsigned long impid) > + unsigned long impid, > + unsigned int stage) > { > int idx; > u32 cpu_req_errata = 0; > > + if (stage == RISCV_ALTERNATIVES_BOOT) { > + if (IS_ENABLED(CONFIG_MMU) && > + errata_cip_453_check_func(archid, impid)) { > + excp_vect_table[EXC_INST_ACCESS] = sifive_cip_453_insn_fault_trp; > + excp_vect_table[EXC_INST_PAGE_FAULT] = sifive_cip_453_page_fault_trp; > + } > + } > + > for (idx = 0; idx < ERRATA_SIFIVE_NUMBER; idx++) > if (errata_list[idx].check_func(archid, impid)) > cpu_req_errata |= (1U << idx); > @@ -99,7 +112,7 @@ void sifive_errata_patch_func(struct alt_entry *begin, struct alt_entry *end, > if (stage == RISCV_ALTERNATIVES_EARLY_BOOT) > return; > > - cpu_req_errata = sifive_errata_probe(archid, impid); > + cpu_req_errata = sifive_errata_probe(archid, impid, stage); > > for (alt = begin; alt < end; alt++) { > if (alt->vendor_id != SIFIVE_VENDOR_ID) > diff --git a/arch/riscv/include/asm/asm-prototypes.h b/arch/riscv/include/asm/asm-prototypes.h > index cd627ec289f1..81a1f27fa54f 100644 > --- a/arch/riscv/include/asm/asm-prototypes.h > +++ b/arch/riscv/include/asm/asm-prototypes.h > @@ -55,5 +55,6 @@ DECLARE_DO_ERROR_INFO(do_trap_break); > asmlinkage void handle_bad_stack(struct pt_regs *regs); > asmlinkage void do_page_fault(struct pt_regs *regs); > asmlinkage void do_irq(struct pt_regs *regs); > +asmlinkage void do_traps(struct pt_regs *regs); > > #endif /* _ASM_RISCV_PROTOTYPES_H */ > diff --git a/arch/riscv/include/asm/errata_list.h b/arch/riscv/include/asm/errata_list.h > index 7c8a71a526a3..95b79afc4061 100644 > --- a/arch/riscv/include/asm/errata_list.h > +++ b/arch/riscv/include/asm/errata_list.h > @@ -17,9 +17,8 @@ > #endif > > #ifdef CONFIG_ERRATA_SIFIVE > -#define ERRATA_SIFIVE_CIP_453 0 > -#define ERRATA_SIFIVE_CIP_1200 1 > -#define ERRATA_SIFIVE_NUMBER 2 > +#define ERRATA_SIFIVE_CIP_1200 0 > +#define ERRATA_SIFIVE_NUMBER 1 > #endif > > #ifdef CONFIG_ERRATA_THEAD > diff --git a/arch/riscv/kernel/entry.S b/arch/riscv/kernel/entry.S > index 81dec627a8d4..401bfe85a098 100644 > --- a/arch/riscv/kernel/entry.S > +++ b/arch/riscv/kernel/entry.S > @@ -62,13 +62,11 @@ SYM_CODE_START(handle_exception) > csrrc s1, CSR_STATUS, t0 > csrr s2, CSR_EPC > csrr s3, CSR_TVAL > - csrr s4, CSR_CAUSE > csrr s5, CSR_SCRATCH > REG_S s0, PT_SP(sp) > REG_S s1, PT_STATUS(sp) > REG_S s2, PT_EPC(sp) > REG_S s3, PT_BADADDR(sp) > - REG_S s4, PT_CAUSE(sp) > REG_S s5, PT_TP(sp) > > /* > @@ -83,36 +81,9 @@ SYM_CODE_START(handle_exception) > /* Load the kernel shadow call stack pointer if coming from userspace */ > scs_load_current_if_task_changed s5 > > -#ifdef CONFIG_RISCV_ISA_V_PREEMPTIVE > - move a0, sp > - call riscv_v_context_nesting_start > -#endif > move a0, sp /* pt_regs */ > - > - /* > - * MSB of cause differentiates between > - * interrupts and exceptions > - */ > - bge s4, zero, 1f > - > - /* Handle interrupts */ > - call do_irq > - j ret_from_exception > -1: > - /* Handle other exceptions */ > - slli t0, s4, RISCV_LGPTR > - la t1, excp_vect_table > - la t2, excp_vect_table_end > - add t0, t1, t0 > - /* Check if exception code lies within bounds */ > - bgeu t0, t2, 3f > - REG_L t1, 0(t0) > -2: jalr t1 > + call do_traps > j ret_from_exception > -3: > - > - la t1, do_trap_unknown > - j 2b > SYM_CODE_END(handle_exception) > ASM_NOKPROBE(handle_exception) > > @@ -329,33 +300,6 @@ SYM_FUNC_START(__switch_to) > ret > SYM_FUNC_END(__switch_to) > > -#ifndef CONFIG_MMU > -#define do_page_fault do_trap_unknown > -#endif > - > - .section ".rodata" > - .align LGREG > - /* Exception vector table */ > -SYM_DATA_START_LOCAL(excp_vect_table) > - RISCV_PTR do_trap_insn_misaligned > - ALT_INSN_FAULT(RISCV_PTR do_trap_insn_fault) > - RISCV_PTR do_trap_insn_illegal > - RISCV_PTR do_trap_break > - RISCV_PTR do_trap_load_misaligned > - RISCV_PTR do_trap_load_fault > - RISCV_PTR do_trap_store_misaligned > - RISCV_PTR do_trap_store_fault > - RISCV_PTR do_trap_ecall_u /* system call */ > - RISCV_PTR do_trap_ecall_s > - RISCV_PTR do_trap_unknown > - RISCV_PTR do_trap_ecall_m > - /* instruciton page fault */ > - ALT_PAGE_FAULT(RISCV_PTR do_page_fault) > - RISCV_PTR do_page_fault /* load page fault */ > - RISCV_PTR do_trap_unknown > - RISCV_PTR do_page_fault /* store page fault */ > -SYM_DATA_END_LABEL(excp_vect_table, SYM_L_LOCAL, excp_vect_table_end) > - > #ifndef CONFIG_MMU > SYM_DATA_START(__user_rt_sigreturn) > li a7, __NR_rt_sigreturn > diff --git a/arch/riscv/kernel/traps.c b/arch/riscv/kernel/traps.c > index 05a16b1f0aee..b44d4a8d4083 100644 > --- a/arch/riscv/kernel/traps.c > +++ b/arch/riscv/kernel/traps.c > @@ -390,6 +390,47 @@ asmlinkage void noinstr do_irq(struct pt_regs *regs) > irqentry_exit(regs, state); > } > > +void (*excp_vect_table[])(struct pt_regs *regs) __ro_after_init = { > + do_trap_insn_misaligned, /* 0 Instruction address misaligned */ > + do_trap_insn_fault, /* 1 Instruction access fault */ > + do_trap_insn_illegal, /* 2 Illegal instruction */ > + do_trap_break, /* 3 Breakpoint */ > + do_trap_load_misaligned, /* 4 Load address misaligned */ > + do_trap_load_fault, /* 5 Load access fault */ > + do_trap_store_misaligned, /* 6 Store/AMO address misaligned */ > + do_trap_store_fault, /* 7 Store/AMO access fault */ > + do_trap_ecall_u, /* 8 Environment call from U-mode */ > + do_trap_ecall_s, /* 9 Environment call from S-mode */ > + do_trap_unknown, /* 10 Reserved */ > + do_trap_ecall_m, /* 11 Environment call from M-mode */ > +#ifdef CONFIG_MMU > + do_page_fault, /* 12 Instruciton page fault */ > + do_page_fault, /* 13 Load page fault */ > + do_trap_unknown, /* 14 Reserved */ > + do_page_fault, /* 15 Store/AMO page fault */ > +#endif > +}; > + > +asmlinkage void noinstr do_traps(struct pt_regs *regs) > +{ > + unsigned long cause = csr_read(CSR_CAUSE); > + > + regs->cause = cause; > + > +#ifdef CONFIG_RISCV_ISA_V_PREEMPTIVE > + riscv_v_context_nesting_start(regs); > +#endif > + if (cause & CAUSE_IRQ_FLAG) { > + do_irq(regs); > + } else { > + if (cause >= ARRAY_SIZE(excp_vect_table)) { Using unlikely here may optimize the hotpath here slightly. - Charlie > + do_trap_unknown(regs); > + return; > + } > + excp_vect_table[cause](regs); > + } > +} > + > #ifdef CONFIG_GENERIC_BUG > int is_valid_bugaddr(unsigned long pc) > { > -- > 2.43.0 > > > _______________________________________________ > linux-riscv mailing list > linux-riscv@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-riscv
On 25/6/2024 4:49 am, Charlie Jenkins wrote: > On Mon, Jun 17, 2024 at 01:05:50AM +0800, Jisheng Zhang wrote: >> For readability, maintainability and future scalability, convert the >> bottom half of the exception handling to C. > > I would be interested to see the performance impact of this, since this > mostly eliminates the code from patch 1 and replaces it in C. > Anton/Cyril can you share the test case you used so that these changes > can also be benchmarked? I ran the series as a whole and it keeps the performance improvement. I think that makes sense because C shouldn't be modifying return addresses. The benchmark is a tweaked (for obvious reasons) of tools/testing/selftests/powerpc/benchmarks/null_syscall.c. > >> >> Mostly the assembly code is converted to C in a relatively >> straightforward manner. >> >> However, there are two modifications I need to mention: >> >> 1. the CSR_CAUSE reg reading and saving is moved to the C code >> because we need the cause to dispatch the exception handling, >> if we keep the cause reading and saving, we either pass it to >> do_traps() via. 2nd param or get it from pt_regs which an extra >> memory load is needed, I don't like any of the two solutions becase >> the exception handling sits in hot code path, every instruction >> matters. >> >> 2.To cope with SIFIVE_CIP_453 errata, it looks like we don't need >> alternative mechanism any more after the asm->c convertion. Just >> replace the excp_vect_table two entries. >> >> Signed-off-by: Jisheng Zhang <jszhang@kernel.org> >> --- >> arch/riscv/errata/sifive/errata.c | 25 ++++++++--- >> arch/riscv/include/asm/asm-prototypes.h | 1 + >> arch/riscv/include/asm/errata_list.h | 5 +-- >> arch/riscv/kernel/entry.S | 58 +------------------------ >> arch/riscv/kernel/traps.c | 41 +++++++++++++++++ >> 5 files changed, 64 insertions(+), 66 deletions(-) >> >> diff --git a/arch/riscv/errata/sifive/errata.c b/arch/riscv/errata/sifive/errata.c >> index 716cfedad3a2..bbba99f207ca 100644 >> --- a/arch/riscv/errata/sifive/errata.c >> +++ b/arch/riscv/errata/sifive/errata.c >> @@ -10,9 +10,14 @@ >> #include <linux/bug.h> >> #include <asm/patch.h> >> #include <asm/alternative.h> >> +#include <asm/csr.h> >> #include <asm/vendorid_list.h> >> #include <asm/errata_list.h> >> >> +extern void (*excp_vect_table[])(struct pt_regs *regs); >> +extern void sifive_cip_453_insn_fault_trp(struct pt_regs *regs); >> +extern void sifive_cip_453_page_fault_trp(struct pt_regs *regs); >> + >> struct errata_info_t { >> char name[32]; >> bool (*check_func)(unsigned long arch_id, unsigned long impid); >> @@ -20,6 +25,9 @@ struct errata_info_t { >> >> static bool errata_cip_453_check_func(unsigned long arch_id, unsigned long impid) >> { >> + if (!IS_ENABLED(CONFIG_ERRATA_SIFIVE_CIP_453)) >> + return false; >> + >> /* >> * Affected cores: >> * Architecture ID: 0x8000000000000007 >> @@ -51,10 +59,6 @@ static bool errata_cip_1200_check_func(unsigned long arch_id, unsigned long imp >> } >> >> static struct errata_info_t errata_list[ERRATA_SIFIVE_NUMBER] = { >> - { >> - .name = "cip-453", >> - .check_func = errata_cip_453_check_func >> - }, >> { >> .name = "cip-1200", >> .check_func = errata_cip_1200_check_func >> @@ -62,11 +66,20 @@ static struct errata_info_t errata_list[ERRATA_SIFIVE_NUMBER] = { >> }; >> >> static u32 __init_or_module sifive_errata_probe(unsigned long archid, >> - unsigned long impid) >> + unsigned long impid, >> + unsigned int stage) >> { >> int idx; >> u32 cpu_req_errata = 0; >> >> + if (stage == RISCV_ALTERNATIVES_BOOT) { >> + if (IS_ENABLED(CONFIG_MMU) && >> + errata_cip_453_check_func(archid, impid)) { >> + excp_vect_table[EXC_INST_ACCESS] = sifive_cip_453_insn_fault_trp; >> + excp_vect_table[EXC_INST_PAGE_FAULT] = sifive_cip_453_page_fault_trp; >> + } >> + } >> + >> for (idx = 0; idx < ERRATA_SIFIVE_NUMBER; idx++) >> if (errata_list[idx].check_func(archid, impid)) >> cpu_req_errata |= (1U << idx); >> @@ -99,7 +112,7 @@ void sifive_errata_patch_func(struct alt_entry *begin, struct alt_entry *end, >> if (stage == RISCV_ALTERNATIVES_EARLY_BOOT) >> return; >> >> - cpu_req_errata = sifive_errata_probe(archid, impid); >> + cpu_req_errata = sifive_errata_probe(archid, impid, stage); >> >> for (alt = begin; alt < end; alt++) { >> if (alt->vendor_id != SIFIVE_VENDOR_ID) >> diff --git a/arch/riscv/include/asm/asm-prototypes.h b/arch/riscv/include/asm/asm-prototypes.h >> index cd627ec289f1..81a1f27fa54f 100644 >> --- a/arch/riscv/include/asm/asm-prototypes.h >> +++ b/arch/riscv/include/asm/asm-prototypes.h >> @@ -55,5 +55,6 @@ DECLARE_DO_ERROR_INFO(do_trap_break); >> asmlinkage void handle_bad_stack(struct pt_regs *regs); >> asmlinkage void do_page_fault(struct pt_regs *regs); >> asmlinkage void do_irq(struct pt_regs *regs); >> +asmlinkage void do_traps(struct pt_regs *regs); >> >> #endif /* _ASM_RISCV_PROTOTYPES_H */ >> diff --git a/arch/riscv/include/asm/errata_list.h b/arch/riscv/include/asm/errata_list.h >> index 7c8a71a526a3..95b79afc4061 100644 >> --- a/arch/riscv/include/asm/errata_list.h >> +++ b/arch/riscv/include/asm/errata_list.h >> @@ -17,9 +17,8 @@ >> #endif >> >> #ifdef CONFIG_ERRATA_SIFIVE >> -#define ERRATA_SIFIVE_CIP_453 0 >> -#define ERRATA_SIFIVE_CIP_1200 1 >> -#define ERRATA_SIFIVE_NUMBER 2 >> +#define ERRATA_SIFIVE_CIP_1200 0 >> +#define ERRATA_SIFIVE_NUMBER 1 >> #endif >> >> #ifdef CONFIG_ERRATA_THEAD >> diff --git a/arch/riscv/kernel/entry.S b/arch/riscv/kernel/entry.S >> index 81dec627a8d4..401bfe85a098 100644 >> --- a/arch/riscv/kernel/entry.S >> +++ b/arch/riscv/kernel/entry.S >> @@ -62,13 +62,11 @@ SYM_CODE_START(handle_exception) >> csrrc s1, CSR_STATUS, t0 >> csrr s2, CSR_EPC >> csrr s3, CSR_TVAL >> - csrr s4, CSR_CAUSE >> csrr s5, CSR_SCRATCH >> REG_S s0, PT_SP(sp) >> REG_S s1, PT_STATUS(sp) >> REG_S s2, PT_EPC(sp) >> REG_S s3, PT_BADADDR(sp) >> - REG_S s4, PT_CAUSE(sp) >> REG_S s5, PT_TP(sp) >> >> /* >> @@ -83,36 +81,9 @@ SYM_CODE_START(handle_exception) >> /* Load the kernel shadow call stack pointer if coming from userspace */ >> scs_load_current_if_task_changed s5 >> >> -#ifdef CONFIG_RISCV_ISA_V_PREEMPTIVE >> - move a0, sp >> - call riscv_v_context_nesting_start >> -#endif >> move a0, sp /* pt_regs */ >> - >> - /* >> - * MSB of cause differentiates between >> - * interrupts and exceptions >> - */ >> - bge s4, zero, 1f >> - >> - /* Handle interrupts */ >> - call do_irq >> - j ret_from_exception >> -1: >> - /* Handle other exceptions */ >> - slli t0, s4, RISCV_LGPTR >> - la t1, excp_vect_table >> - la t2, excp_vect_table_end >> - add t0, t1, t0 >> - /* Check if exception code lies within bounds */ >> - bgeu t0, t2, 3f >> - REG_L t1, 0(t0) >> -2: jalr t1 >> + call do_traps >> j ret_from_exception >> -3: >> - >> - la t1, do_trap_unknown >> - j 2b >> SYM_CODE_END(handle_exception) >> ASM_NOKPROBE(handle_exception) >> >> @@ -329,33 +300,6 @@ SYM_FUNC_START(__switch_to) >> ret >> SYM_FUNC_END(__switch_to) >> >> -#ifndef CONFIG_MMU >> -#define do_page_fault do_trap_unknown >> -#endif >> - >> - .section ".rodata" >> - .align LGREG >> - /* Exception vector table */ >> -SYM_DATA_START_LOCAL(excp_vect_table) >> - RISCV_PTR do_trap_insn_misaligned >> - ALT_INSN_FAULT(RISCV_PTR do_trap_insn_fault) >> - RISCV_PTR do_trap_insn_illegal >> - RISCV_PTR do_trap_break >> - RISCV_PTR do_trap_load_misaligned >> - RISCV_PTR do_trap_load_fault >> - RISCV_PTR do_trap_store_misaligned >> - RISCV_PTR do_trap_store_fault >> - RISCV_PTR do_trap_ecall_u /* system call */ >> - RISCV_PTR do_trap_ecall_s >> - RISCV_PTR do_trap_unknown >> - RISCV_PTR do_trap_ecall_m >> - /* instruciton page fault */ >> - ALT_PAGE_FAULT(RISCV_PTR do_page_fault) >> - RISCV_PTR do_page_fault /* load page fault */ >> - RISCV_PTR do_trap_unknown >> - RISCV_PTR do_page_fault /* store page fault */ >> -SYM_DATA_END_LABEL(excp_vect_table, SYM_L_LOCAL, excp_vect_table_end) >> - >> #ifndef CONFIG_MMU >> SYM_DATA_START(__user_rt_sigreturn) >> li a7, __NR_rt_sigreturn >> diff --git a/arch/riscv/kernel/traps.c b/arch/riscv/kernel/traps.c >> index 05a16b1f0aee..b44d4a8d4083 100644 >> --- a/arch/riscv/kernel/traps.c >> +++ b/arch/riscv/kernel/traps.c >> @@ -390,6 +390,47 @@ asmlinkage void noinstr do_irq(struct pt_regs *regs) >> irqentry_exit(regs, state); >> } >> >> +void (*excp_vect_table[])(struct pt_regs *regs) __ro_after_init = { >> + do_trap_insn_misaligned, /* 0 Instruction address misaligned */ >> + do_trap_insn_fault, /* 1 Instruction access fault */ >> + do_trap_insn_illegal, /* 2 Illegal instruction */ >> + do_trap_break, /* 3 Breakpoint */ >> + do_trap_load_misaligned, /* 4 Load address misaligned */ >> + do_trap_load_fault, /* 5 Load access fault */ >> + do_trap_store_misaligned, /* 6 Store/AMO address misaligned */ >> + do_trap_store_fault, /* 7 Store/AMO access fault */ >> + do_trap_ecall_u, /* 8 Environment call from U-mode */ >> + do_trap_ecall_s, /* 9 Environment call from S-mode */ >> + do_trap_unknown, /* 10 Reserved */ >> + do_trap_ecall_m, /* 11 Environment call from M-mode */ >> +#ifdef CONFIG_MMU >> + do_page_fault, /* 12 Instruciton page fault */ >> + do_page_fault, /* 13 Load page fault */ >> + do_trap_unknown, /* 14 Reserved */ >> + do_page_fault, /* 15 Store/AMO page fault */ >> +#endif >> +}; >> + >> +asmlinkage void noinstr do_traps(struct pt_regs *regs) >> +{ >> + unsigned long cause = csr_read(CSR_CAUSE); >> + >> + regs->cause = cause; >> + >> +#ifdef CONFIG_RISCV_ISA_V_PREEMPTIVE >> + riscv_v_context_nesting_start(regs); >> +#endif >> + if (cause & CAUSE_IRQ_FLAG) { >> + do_irq(regs); >> + } else { >> + if (cause >= ARRAY_SIZE(excp_vect_table)) { > > Using unlikely here may optimize the hotpath here slightly. > > - Charlie > >> + do_trap_unknown(regs); >> + return; >> + } >> + excp_vect_table[cause](regs); >> + } >> +} >> + >> #ifdef CONFIG_GENERIC_BUG >> int is_valid_bugaddr(unsigned long pc) >> { >> -- >> 2.43.0 >> >> >> _______________________________________________ >> linux-riscv mailing list >> linux-riscv@lists.infradead.org >> http://lists.infradead.org/mailman/listinfo/linux-riscv
diff --git a/arch/riscv/errata/sifive/errata.c b/arch/riscv/errata/sifive/errata.c index 716cfedad3a2..bbba99f207ca 100644 --- a/arch/riscv/errata/sifive/errata.c +++ b/arch/riscv/errata/sifive/errata.c @@ -10,9 +10,14 @@ #include <linux/bug.h> #include <asm/patch.h> #include <asm/alternative.h> +#include <asm/csr.h> #include <asm/vendorid_list.h> #include <asm/errata_list.h> +extern void (*excp_vect_table[])(struct pt_regs *regs); +extern void sifive_cip_453_insn_fault_trp(struct pt_regs *regs); +extern void sifive_cip_453_page_fault_trp(struct pt_regs *regs); + struct errata_info_t { char name[32]; bool (*check_func)(unsigned long arch_id, unsigned long impid); @@ -20,6 +25,9 @@ struct errata_info_t { static bool errata_cip_453_check_func(unsigned long arch_id, unsigned long impid) { + if (!IS_ENABLED(CONFIG_ERRATA_SIFIVE_CIP_453)) + return false; + /* * Affected cores: * Architecture ID: 0x8000000000000007 @@ -51,10 +59,6 @@ static bool errata_cip_1200_check_func(unsigned long arch_id, unsigned long imp } static struct errata_info_t errata_list[ERRATA_SIFIVE_NUMBER] = { - { - .name = "cip-453", - .check_func = errata_cip_453_check_func - }, { .name = "cip-1200", .check_func = errata_cip_1200_check_func @@ -62,11 +66,20 @@ static struct errata_info_t errata_list[ERRATA_SIFIVE_NUMBER] = { }; static u32 __init_or_module sifive_errata_probe(unsigned long archid, - unsigned long impid) + unsigned long impid, + unsigned int stage) { int idx; u32 cpu_req_errata = 0; + if (stage == RISCV_ALTERNATIVES_BOOT) { + if (IS_ENABLED(CONFIG_MMU) && + errata_cip_453_check_func(archid, impid)) { + excp_vect_table[EXC_INST_ACCESS] = sifive_cip_453_insn_fault_trp; + excp_vect_table[EXC_INST_PAGE_FAULT] = sifive_cip_453_page_fault_trp; + } + } + for (idx = 0; idx < ERRATA_SIFIVE_NUMBER; idx++) if (errata_list[idx].check_func(archid, impid)) cpu_req_errata |= (1U << idx); @@ -99,7 +112,7 @@ void sifive_errata_patch_func(struct alt_entry *begin, struct alt_entry *end, if (stage == RISCV_ALTERNATIVES_EARLY_BOOT) return; - cpu_req_errata = sifive_errata_probe(archid, impid); + cpu_req_errata = sifive_errata_probe(archid, impid, stage); for (alt = begin; alt < end; alt++) { if (alt->vendor_id != SIFIVE_VENDOR_ID) diff --git a/arch/riscv/include/asm/asm-prototypes.h b/arch/riscv/include/asm/asm-prototypes.h index cd627ec289f1..81a1f27fa54f 100644 --- a/arch/riscv/include/asm/asm-prototypes.h +++ b/arch/riscv/include/asm/asm-prototypes.h @@ -55,5 +55,6 @@ DECLARE_DO_ERROR_INFO(do_trap_break); asmlinkage void handle_bad_stack(struct pt_regs *regs); asmlinkage void do_page_fault(struct pt_regs *regs); asmlinkage void do_irq(struct pt_regs *regs); +asmlinkage void do_traps(struct pt_regs *regs); #endif /* _ASM_RISCV_PROTOTYPES_H */ diff --git a/arch/riscv/include/asm/errata_list.h b/arch/riscv/include/asm/errata_list.h index 7c8a71a526a3..95b79afc4061 100644 --- a/arch/riscv/include/asm/errata_list.h +++ b/arch/riscv/include/asm/errata_list.h @@ -17,9 +17,8 @@ #endif #ifdef CONFIG_ERRATA_SIFIVE -#define ERRATA_SIFIVE_CIP_453 0 -#define ERRATA_SIFIVE_CIP_1200 1 -#define ERRATA_SIFIVE_NUMBER 2 +#define ERRATA_SIFIVE_CIP_1200 0 +#define ERRATA_SIFIVE_NUMBER 1 #endif #ifdef CONFIG_ERRATA_THEAD diff --git a/arch/riscv/kernel/entry.S b/arch/riscv/kernel/entry.S index 81dec627a8d4..401bfe85a098 100644 --- a/arch/riscv/kernel/entry.S +++ b/arch/riscv/kernel/entry.S @@ -62,13 +62,11 @@ SYM_CODE_START(handle_exception) csrrc s1, CSR_STATUS, t0 csrr s2, CSR_EPC csrr s3, CSR_TVAL - csrr s4, CSR_CAUSE csrr s5, CSR_SCRATCH REG_S s0, PT_SP(sp) REG_S s1, PT_STATUS(sp) REG_S s2, PT_EPC(sp) REG_S s3, PT_BADADDR(sp) - REG_S s4, PT_CAUSE(sp) REG_S s5, PT_TP(sp) /* @@ -83,36 +81,9 @@ SYM_CODE_START(handle_exception) /* Load the kernel shadow call stack pointer if coming from userspace */ scs_load_current_if_task_changed s5 -#ifdef CONFIG_RISCV_ISA_V_PREEMPTIVE - move a0, sp - call riscv_v_context_nesting_start -#endif move a0, sp /* pt_regs */ - - /* - * MSB of cause differentiates between - * interrupts and exceptions - */ - bge s4, zero, 1f - - /* Handle interrupts */ - call do_irq - j ret_from_exception -1: - /* Handle other exceptions */ - slli t0, s4, RISCV_LGPTR - la t1, excp_vect_table - la t2, excp_vect_table_end - add t0, t1, t0 - /* Check if exception code lies within bounds */ - bgeu t0, t2, 3f - REG_L t1, 0(t0) -2: jalr t1 + call do_traps j ret_from_exception -3: - - la t1, do_trap_unknown - j 2b SYM_CODE_END(handle_exception) ASM_NOKPROBE(handle_exception) @@ -329,33 +300,6 @@ SYM_FUNC_START(__switch_to) ret SYM_FUNC_END(__switch_to) -#ifndef CONFIG_MMU -#define do_page_fault do_trap_unknown -#endif - - .section ".rodata" - .align LGREG - /* Exception vector table */ -SYM_DATA_START_LOCAL(excp_vect_table) - RISCV_PTR do_trap_insn_misaligned - ALT_INSN_FAULT(RISCV_PTR do_trap_insn_fault) - RISCV_PTR do_trap_insn_illegal - RISCV_PTR do_trap_break - RISCV_PTR do_trap_load_misaligned - RISCV_PTR do_trap_load_fault - RISCV_PTR do_trap_store_misaligned - RISCV_PTR do_trap_store_fault - RISCV_PTR do_trap_ecall_u /* system call */ - RISCV_PTR do_trap_ecall_s - RISCV_PTR do_trap_unknown - RISCV_PTR do_trap_ecall_m - /* instruciton page fault */ - ALT_PAGE_FAULT(RISCV_PTR do_page_fault) - RISCV_PTR do_page_fault /* load page fault */ - RISCV_PTR do_trap_unknown - RISCV_PTR do_page_fault /* store page fault */ -SYM_DATA_END_LABEL(excp_vect_table, SYM_L_LOCAL, excp_vect_table_end) - #ifndef CONFIG_MMU SYM_DATA_START(__user_rt_sigreturn) li a7, __NR_rt_sigreturn diff --git a/arch/riscv/kernel/traps.c b/arch/riscv/kernel/traps.c index 05a16b1f0aee..b44d4a8d4083 100644 --- a/arch/riscv/kernel/traps.c +++ b/arch/riscv/kernel/traps.c @@ -390,6 +390,47 @@ asmlinkage void noinstr do_irq(struct pt_regs *regs) irqentry_exit(regs, state); } +void (*excp_vect_table[])(struct pt_regs *regs) __ro_after_init = { + do_trap_insn_misaligned, /* 0 Instruction address misaligned */ + do_trap_insn_fault, /* 1 Instruction access fault */ + do_trap_insn_illegal, /* 2 Illegal instruction */ + do_trap_break, /* 3 Breakpoint */ + do_trap_load_misaligned, /* 4 Load address misaligned */ + do_trap_load_fault, /* 5 Load access fault */ + do_trap_store_misaligned, /* 6 Store/AMO address misaligned */ + do_trap_store_fault, /* 7 Store/AMO access fault */ + do_trap_ecall_u, /* 8 Environment call from U-mode */ + do_trap_ecall_s, /* 9 Environment call from S-mode */ + do_trap_unknown, /* 10 Reserved */ + do_trap_ecall_m, /* 11 Environment call from M-mode */ +#ifdef CONFIG_MMU + do_page_fault, /* 12 Instruciton page fault */ + do_page_fault, /* 13 Load page fault */ + do_trap_unknown, /* 14 Reserved */ + do_page_fault, /* 15 Store/AMO page fault */ +#endif +}; + +asmlinkage void noinstr do_traps(struct pt_regs *regs) +{ + unsigned long cause = csr_read(CSR_CAUSE); + + regs->cause = cause; + +#ifdef CONFIG_RISCV_ISA_V_PREEMPTIVE + riscv_v_context_nesting_start(regs); +#endif + if (cause & CAUSE_IRQ_FLAG) { + do_irq(regs); + } else { + if (cause >= ARRAY_SIZE(excp_vect_table)) { + do_trap_unknown(regs); + return; + } + excp_vect_table[cause](regs); + } +} + #ifdef CONFIG_GENERIC_BUG int is_valid_bugaddr(unsigned long pc) {
For readability, maintainability and future scalability, convert the bottom half of the exception handling to C. Mostly the assembly code is converted to C in a relatively straightforward manner. However, there are two modifications I need to mention: 1. the CSR_CAUSE reg reading and saving is moved to the C code because we need the cause to dispatch the exception handling, if we keep the cause reading and saving, we either pass it to do_traps() via. 2nd param or get it from pt_regs which an extra memory load is needed, I don't like any of the two solutions becase the exception handling sits in hot code path, every instruction matters. 2.To cope with SIFIVE_CIP_453 errata, it looks like we don't need alternative mechanism any more after the asm->c convertion. Just replace the excp_vect_table two entries. Signed-off-by: Jisheng Zhang <jszhang@kernel.org> --- arch/riscv/errata/sifive/errata.c | 25 ++++++++--- arch/riscv/include/asm/asm-prototypes.h | 1 + arch/riscv/include/asm/errata_list.h | 5 +-- arch/riscv/kernel/entry.S | 58 +------------------------ arch/riscv/kernel/traps.c | 41 +++++++++++++++++ 5 files changed, 64 insertions(+), 66 deletions(-)