[PATCH 0/6] riscv: convert bottom half of exception handling to C

[PATCH 0/6] riscv: convert bottom half of exception handling to C

[Jisheng Zhang]

For readability, maintainability and future scalability, convert the
bottom half of the exception handling to C.

During the conversion, I found Anton fixed a performance issue
and my patches will touch the same exception asm code, so I include
Anton's patch for completeness. I also cooked a similar patch to avoid
corrupting the RAS in ret_from_fork() per the inspiration.

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.



Anton Blanchard (1):
  riscv: Improve exception and system call latency

Jisheng Zhang (5):
  riscv: avoid corrupting the RAS
  riscv: convert bottom half of exception handling to C
  riscv: errata: remove ALT_INSN_FAULT and ALT_PAGE_FAULT
  riscv: errata: sifive: remove NOMMU handling
  riscv: remove asmlinkage from updated functions

 arch/riscv/errata/sifive/errata.c         | 25 +++++++---
 arch/riscv/errata/sifive/errata_cip_453.S |  4 --
 arch/riscv/include/asm/asm-prototypes.h   |  7 +--
 arch/riscv/include/asm/errata_list.h      | 21 ++------
 arch/riscv/kernel/entry.S                 | 61 ++---------------------
 arch/riscv/kernel/stacktrace.c            |  4 +-
 arch/riscv/kernel/traps.c                 | 57 ++++++++++++++++++---
 7 files changed, 81 insertions(+), 98 deletions(-)

-- 
2.43.0

[PATCH 1/6] riscv: Improve exception and system call latency

[Jisheng Zhang]

From: Anton Blanchard <antonb@tenstorrent.com>

Many CPUs implement return address branch prediction as a stack. The
RISCV architecture refers to this as a return address stack (RAS). If
this gets corrupted then the CPU will mispredict at least one but
potentally many function returns.

There are two issues with the current RISCV exception code:

- We are using the alternate link stack (x5/t0) for the indirect branch
  which makes the hardware think this is a function return. This will
  corrupt the RAS.

- We modify the return address of handle_exception to point to
  ret_from_exception. This will also corrupt the RAS.

Testing the null system call latency before and after the patch:

Visionfive2 (StarFive JH7110 / U74)
baseline: 189.87 ns
patched:  176.76 ns

Lichee pi 4a (T-Head TH1520 / C910)
baseline: 666.58 ns
patched:  636.90 ns

Just over 7% on the U74 and just over 4% on the C910.

Signed-off-by: Anton Blanchard <antonb@tenstorrent.com>
Signed-off-by: Cyril Bur <cyrilbur@tenstorrent.com>
---
 arch/riscv/kernel/entry.S      | 17 ++++++++++-------
 arch/riscv/kernel/stacktrace.c |  4 ++--
 2 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/arch/riscv/kernel/entry.S b/arch/riscv/kernel/entry.S
index 68a24cf9481a..c933460ed3e9 100644
--- a/arch/riscv/kernel/entry.S
+++ b/arch/riscv/kernel/entry.S
@@ -88,7 +88,6 @@ SYM_CODE_START(handle_exception)
 	call riscv_v_context_nesting_start
 #endif
 	move a0, sp /* pt_regs */
-	la ra, ret_from_exception
 
 	/*
 	 * MSB of cause differentiates between
@@ -97,7 +96,8 @@ SYM_CODE_START(handle_exception)
 	bge s4, zero, 1f
 
 	/* Handle interrupts */
-	tail do_irq
+	call do_irq
+	j ret_from_exception
 1:
 	/* Handle other exceptions */
 	slli t0, s4, RISCV_LGPTR
@@ -105,11 +105,14 @@ SYM_CODE_START(handle_exception)
 	la t2, excp_vect_table_end
 	add t0, t1, t0
 	/* Check if exception code lies within bounds */
-	bgeu t0, t2, 1f
-	REG_L t0, 0(t0)
-	jr t0
-1:
-	tail do_trap_unknown
+	bgeu t0, t2, 3f
+	REG_L t1, 0(t0)
+2:	jalr t1
+	j ret_from_exception
+3:
+
+	la t1, do_trap_unknown
+	j 2b
 SYM_CODE_END(handle_exception)
 ASM_NOKPROBE(handle_exception)
 
diff --git a/arch/riscv/kernel/stacktrace.c b/arch/riscv/kernel/stacktrace.c
index 528ec7cc9a62..5eb3d135b717 100644
--- a/arch/riscv/kernel/stacktrace.c
+++ b/arch/riscv/kernel/stacktrace.c
@@ -16,7 +16,7 @@
 
 #ifdef CONFIG_FRAME_POINTER
 
-extern asmlinkage void ret_from_exception(void);
+extern asmlinkage void handle_exception(void);
 
 static inline int fp_is_valid(unsigned long fp, unsigned long sp)
 {
@@ -70,7 +70,7 @@ void notrace walk_stackframe(struct task_struct *task, struct pt_regs *regs,
 			fp = frame->fp;
 			pc = ftrace_graph_ret_addr(current, NULL, frame->ra,
 						   &frame->ra);
-			if (pc == (unsigned long)ret_from_exception) {
+			if (pc == (unsigned long)handle_exception) {
 				if (unlikely(!__kernel_text_address(pc) || !fn(arg, pc)))
 					break;
 
-- 
2.43.0

Re: [PATCH 1/6] riscv: Improve exception and system call latency

[Charlie Jenkins]

On Mon, Jun 17, 2024 at 01:05:48AM +0800, Jisheng Zhang wrote:
> From: Anton Blanchard <antonb@tenstorrent.com>
> 
> Many CPUs implement return address branch prediction as a stack. The
> RISCV architecture refers to this as a return address stack (RAS). If
> this gets corrupted then the CPU will mispredict at least one but
> potentally many function returns.
> 
> There are two issues with the current RISCV exception code:
> 
> - We are using the alternate link stack (x5/t0) for the indirect branch
>   which makes the hardware think this is a function return. This will
>   corrupt the RAS.
> 
> - We modify the return address of handle_exception to point to
>   ret_from_exception. This will also corrupt the RAS.
> 
> Testing the null system call latency before and after the patch:
> 
> Visionfive2 (StarFive JH7110 / U74)
> baseline: 189.87 ns
> patched:  176.76 ns
> 
> Lichee pi 4a (T-Head TH1520 / C910)
> baseline: 666.58 ns
> patched:  636.90 ns
> 
> Just over 7% on the U74 and just over 4% on the C910.
> 
> Signed-off-by: Anton Blanchard <antonb@tenstorrent.com>
> Signed-off-by: Cyril Bur <cyrilbur@tenstorrent.com>

Do you need to sign this off since you're sending this Jisheng?

> ---
>  arch/riscv/kernel/entry.S      | 17 ++++++++++-------
>  arch/riscv/kernel/stacktrace.c |  4 ++--
>  2 files changed, 12 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/riscv/kernel/entry.S b/arch/riscv/kernel/entry.S
> index 68a24cf9481a..c933460ed3e9 100644
> --- a/arch/riscv/kernel/entry.S
> +++ b/arch/riscv/kernel/entry.S
> @@ -88,7 +88,6 @@ SYM_CODE_START(handle_exception)
>  	call riscv_v_context_nesting_start
>  #endif
>  	move a0, sp /* pt_regs */
> -	la ra, ret_from_exception
>  
>  	/*
>  	 * MSB of cause differentiates between
> @@ -97,7 +96,8 @@ SYM_CODE_START(handle_exception)
>  	bge s4, zero, 1f
>  
>  	/* Handle interrupts */
> -	tail do_irq
> +	call do_irq
> +	j ret_from_exception
>  1:
>  	/* Handle other exceptions */
>  	slli t0, s4, RISCV_LGPTR
> @@ -105,11 +105,14 @@ SYM_CODE_START(handle_exception)
>  	la t2, excp_vect_table_end
>  	add t0, t1, t0
>  	/* Check if exception code lies within bounds */
> -	bgeu t0, t2, 1f
> -	REG_L t0, 0(t0)
> -	jr t0
> -1:
> -	tail do_trap_unknown
> +	bgeu t0, t2, 3f
> +	REG_L t1, 0(t0)
> +2:	jalr t1
> +	j ret_from_exception
> +3:
> +

The whitespace is odd here, but nonetheless:

Reviewed-by: Charlie Jenkins <charlie@rivosinc.com>

> +	la t1, do_trap_unknown
> +	j 2b
>  SYM_CODE_END(handle_exception)
>  ASM_NOKPROBE(handle_exception)
>  
> diff --git a/arch/riscv/kernel/stacktrace.c b/arch/riscv/kernel/stacktrace.c
> index 528ec7cc9a62..5eb3d135b717 100644
> --- a/arch/riscv/kernel/stacktrace.c
> +++ b/arch/riscv/kernel/stacktrace.c
> @@ -16,7 +16,7 @@
>  
>  #ifdef CONFIG_FRAME_POINTER
>  
> -extern asmlinkage void ret_from_exception(void);
> +extern asmlinkage void handle_exception(void);
>  
>  static inline int fp_is_valid(unsigned long fp, unsigned long sp)
>  {
> @@ -70,7 +70,7 @@ void notrace walk_stackframe(struct task_struct *task, struct pt_regs *regs,
>  			fp = frame->fp;
>  			pc = ftrace_graph_ret_addr(current, NULL, frame->ra,
>  						   &frame->ra);
> -			if (pc == (unsigned long)ret_from_exception) {
> +			if (pc == (unsigned long)handle_exception) {
>  				if (unlikely(!__kernel_text_address(pc) || !fn(arg, pc)))
>  					break;
>  
> -- 
> 2.43.0
> 
> 
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH 1/6] riscv: Improve exception and system call latency

[Jisheng Zhang]

On Fri, Jun 21, 2024 at 05:15:29PM -0700, Charlie Jenkins wrote:
> On Mon, Jun 17, 2024 at 01:05:48AM +0800, Jisheng Zhang wrote:
> > From: Anton Blanchard <antonb@tenstorrent.com>
> > 
> > Many CPUs implement return address branch prediction as a stack. The
> > RISCV architecture refers to this as a return address stack (RAS). If
> > this gets corrupted then the CPU will mispredict at least one but
> > potentally many function returns.
> > 
> > There are two issues with the current RISCV exception code:
> > 
> > - We are using the alternate link stack (x5/t0) for the indirect branch
> >   which makes the hardware think this is a function return. This will
> >   corrupt the RAS.
> > 
> > - We modify the return address of handle_exception to point to
> >   ret_from_exception. This will also corrupt the RAS.
> > 
> > Testing the null system call latency before and after the patch:
> > 
> > Visionfive2 (StarFive JH7110 / U74)
> > baseline: 189.87 ns
> > patched:  176.76 ns
> > 
> > Lichee pi 4a (T-Head TH1520 / C910)
> > baseline: 666.58 ns
> > patched:  636.90 ns
> > 
> > Just over 7% on the U74 and just over 4% on the C910.
> > 
> > Signed-off-by: Anton Blanchard <antonb@tenstorrent.com>
> > Signed-off-by: Cyril Bur <cyrilbur@tenstorrent.com>
> 
> Do you need to sign this off since you're sending this Jisheng?

will do in newer version. Thanks for reminding.

I'm sending out this for reference since we touched the same
asm source code.
> 
> > ---
> >  arch/riscv/kernel/entry.S      | 17 ++++++++++-------
> >  arch/riscv/kernel/stacktrace.c |  4 ++--
> >  2 files changed, 12 insertions(+), 9 deletions(-)
> > 
> > diff --git a/arch/riscv/kernel/entry.S b/arch/riscv/kernel/entry.S
> > index 68a24cf9481a..c933460ed3e9 100644
> > --- a/arch/riscv/kernel/entry.S
> > +++ b/arch/riscv/kernel/entry.S
> > @@ -88,7 +88,6 @@ SYM_CODE_START(handle_exception)
> >  	call riscv_v_context_nesting_start
> >  #endif
> >  	move a0, sp /* pt_regs */
> > -	la ra, ret_from_exception
> >  
> >  	/*
> >  	 * MSB of cause differentiates between
> > @@ -97,7 +96,8 @@ SYM_CODE_START(handle_exception)
> >  	bge s4, zero, 1f
> >  
> >  	/* Handle interrupts */
> > -	tail do_irq
> > +	call do_irq
> > +	j ret_from_exception
> >  1:
> >  	/* Handle other exceptions */
> >  	slli t0, s4, RISCV_LGPTR
> > @@ -105,11 +105,14 @@ SYM_CODE_START(handle_exception)
> >  	la t2, excp_vect_table_end
> >  	add t0, t1, t0
> >  	/* Check if exception code lies within bounds */
> > -	bgeu t0, t2, 1f
> > -	REG_L t0, 0(t0)
> > -	jr t0
> > -1:
> > -	tail do_trap_unknown
> > +	bgeu t0, t2, 3f
> > +	REG_L t1, 0(t0)
> > +2:	jalr t1
> > +	j ret_from_exception
> > +3:
> > +
> 
> The whitespace is odd here, but nonetheless:
> 
> Reviewed-by: Charlie Jenkins <charlie@rivosinc.com>
> 
> > +	la t1, do_trap_unknown
> > +	j 2b
> >  SYM_CODE_END(handle_exception)
> >  ASM_NOKPROBE(handle_exception)
> >  
> > diff --git a/arch/riscv/kernel/stacktrace.c b/arch/riscv/kernel/stacktrace.c
> > index 528ec7cc9a62..5eb3d135b717 100644
> > --- a/arch/riscv/kernel/stacktrace.c
> > +++ b/arch/riscv/kernel/stacktrace.c
> > @@ -16,7 +16,7 @@
> >  
> >  #ifdef CONFIG_FRAME_POINTER
> >  
> > -extern asmlinkage void ret_from_exception(void);
> > +extern asmlinkage void handle_exception(void);
> >  
> >  static inline int fp_is_valid(unsigned long fp, unsigned long sp)
> >  {
> > @@ -70,7 +70,7 @@ void notrace walk_stackframe(struct task_struct *task, struct pt_regs *regs,
> >  			fp = frame->fp;
> >  			pc = ftrace_graph_ret_addr(current, NULL, frame->ra,
> >  						   &frame->ra);
> > -			if (pc == (unsigned long)ret_from_exception) {
> > +			if (pc == (unsigned long)handle_exception) {
> >  				if (unlikely(!__kernel_text_address(pc) || !fn(arg, pc)))
> >  					break;
> >  
> > -- 
> > 2.43.0
> > 
> > 
> > _______________________________________________
> > linux-riscv mailing list
> > linux-riscv@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH 2/6] riscv: avoid corrupting the RAS

[Jisheng Zhang]

Inspired by[1], "modifying the return address of to point to
ret_from_exception will corrupt the RAS", so modify the code
to remove the code of modifying ra.

Link: https://lore.kernel.org/linux-riscv/20240607061335.2197383-1-cyrilbur@tenstorrent.com/ [1]
Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
---
 arch/riscv/kernel/entry.S | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/riscv/kernel/entry.S b/arch/riscv/kernel/entry.S
index c933460ed3e9..81dec627a8d4 100644
--- a/arch/riscv/kernel/entry.S
+++ b/arch/riscv/kernel/entry.S
@@ -235,8 +235,8 @@ SYM_CODE_START(ret_from_fork)
 	jalr s0
 1:
 	move a0, sp /* pt_regs */
-	la ra, ret_from_exception
-	tail syscall_exit_to_user_mode
+	call syscall_exit_to_user_mode
+	j ret_from_exception
 SYM_CODE_END(ret_from_fork)
 
 #ifdef CONFIG_IRQ_STACKS
-- 
2.43.0

Re: [PATCH 2/6] riscv: avoid corrupting the RAS

[Cyril Bur]

On Mon, Jun 17, 2024 at 3:21 AM Jisheng Zhang <jszhang@kernel.org> wrote:
>
> Inspired by[1], "modifying the return address of to point to
> ret_from_exception will corrupt the RAS", so modify the code
> to remove the code of modifying ra.
>
> Link: https://lore.kernel.org/linux-riscv/20240607061335.2197383-1-cyrilbur@tenstorrent.com/ [1]
> Signed-off-by: Jisheng Zhang <jszhang@kernel.org>

I've run this and it is the same idea that Anton used in the previous patch
Reviewed-by: Cyril Bur <cyrilbur@tenstorrent.com>

> ---
>  arch/riscv/kernel/entry.S | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/riscv/kernel/entry.S b/arch/riscv/kernel/entry.S
> index c933460ed3e9..81dec627a8d4 100644
> --- a/arch/riscv/kernel/entry.S
> +++ b/arch/riscv/kernel/entry.S
> @@ -235,8 +235,8 @@ SYM_CODE_START(ret_from_fork)
>         jalr s0
>  1:
>         move a0, sp /* pt_regs */
> -       la ra, ret_from_exception
> -       tail syscall_exit_to_user_mode
> +       call syscall_exit_to_user_mode
> +       j ret_from_exception
>  SYM_CODE_END(ret_from_fork)
>
>  #ifdef CONFIG_IRQ_STACKS
> --
> 2.43.0
>
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH 3/6] riscv: convert bottom half of exception handling to C

[Jisheng Zhang]

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(-)

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)
 {
-- 
2.43.0

Re: [PATCH 3/6] riscv: convert bottom half of exception handling to C

[Deepak Gupta]

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>

Re: [PATCH 3/6] riscv: convert bottom half of exception handling to C

[Cyril Bur]

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

Re: [PATCH 3/6] riscv: convert bottom half of exception handling to C

[Clément Léger]

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

Re: [PATCH 3/6] riscv: convert bottom half of exception handling to C

[Jisheng Zhang]

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

Re: [PATCH 3/6] riscv: convert bottom half of exception handling to C

[Deepak Gupta]

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

Re: riscv: convert bottom half of exception handling to C

[Cyril Bur]

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)
>   {

Re: riscv: convert bottom half of exception handling to C

[Jisheng Zhang]

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)
> >   {

Re: [PATCH 3/6] riscv: convert bottom half of exception handling to C

[Charlie Jenkins]

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

Re: [PATCH 3/6] riscv: convert bottom half of exception handling to C

[Cyril Bur]

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

[PATCH 4/6] riscv: errata: remove ALT_INSN_FAULT and ALT_PAGE_FAULT

[Jisheng Zhang]

They are used for SIFIVE_CIP_453 errata, which has been solved by
replacing the excp_vect_table[] two entries in last commit. So these
two macros are useless now, remove them.

Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
---
 arch/riscv/include/asm/errata_list.h | 16 +---------------
 1 file changed, 1 insertion(+), 15 deletions(-)

diff --git a/arch/riscv/include/asm/errata_list.h b/arch/riscv/include/asm/errata_list.h
index 95b79afc4061..46bf00c4a57a 100644
--- a/arch/riscv/include/asm/errata_list.h
+++ b/arch/riscv/include/asm/errata_list.h
@@ -27,21 +27,7 @@
 #define	ERRATA_THEAD_NUMBER 2
 #endif
 
-#ifdef __ASSEMBLY__
-
-#define ALT_INSN_FAULT(x)						\
-ALTERNATIVE(__stringify(RISCV_PTR do_trap_insn_fault),			\
-	    __stringify(RISCV_PTR sifive_cip_453_insn_fault_trp),	\
-	    SIFIVE_VENDOR_ID, ERRATA_SIFIVE_CIP_453,			\
-	    CONFIG_ERRATA_SIFIVE_CIP_453)
-
-#define ALT_PAGE_FAULT(x)						\
-ALTERNATIVE(__stringify(RISCV_PTR do_page_fault),			\
-	    __stringify(RISCV_PTR sifive_cip_453_page_fault_trp),	\
-	    SIFIVE_VENDOR_ID, ERRATA_SIFIVE_CIP_453,			\
-	    CONFIG_ERRATA_SIFIVE_CIP_453)
-#else /* !__ASSEMBLY__ */
-
+#ifndef __ASSEMBLY__
 #define ALT_SFENCE_VMA_ASID(asid)					\
 asm(ALTERNATIVE("sfence.vma x0, %0", "sfence.vma", SIFIVE_VENDOR_ID,	\
 		ERRATA_SIFIVE_CIP_1200, CONFIG_ERRATA_SIFIVE_CIP_1200)	\
-- 
2.43.0

[PATCH 5/6] riscv: errata: sifive: remove NOMMU handling

[Jisheng Zhang]

Since NOMMU is now properly handling in generic do_traps() which will
call do_trap_unknown() for instruciton page fault for NOMMU.

Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
---
 arch/riscv/errata/sifive/errata_cip_453.S | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/arch/riscv/errata/sifive/errata_cip_453.S b/arch/riscv/errata/sifive/errata_cip_453.S
index f1b9623fe1de..b8ca2ebc2652 100644
--- a/arch/riscv/errata/sifive/errata_cip_453.S
+++ b/arch/riscv/errata/sifive/errata_cip_453.S
@@ -23,11 +23,7 @@
 
 ENTRY(sifive_cip_453_page_fault_trp)
 	ADD_SIGN_EXT a0, t0, t1
-#ifdef CONFIG_MMU
 	la t0, do_page_fault
-#else
-	la t0, do_trap_unknown
-#endif
 	jr t0
 END(sifive_cip_453_page_fault_trp)
 
-- 
2.43.0

[PATCH 6/6] riscv: remove asmlinkage from updated functions

[Jisheng Zhang]

Now that the callers of these functions have moved into C, they no longer
need the asmlinkage annotation. Remove it

Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
---
 arch/riscv/include/asm/asm-prototypes.h |  6 +++---
 arch/riscv/kernel/traps.c               | 16 ++++++++--------
 2 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/arch/riscv/include/asm/asm-prototypes.h b/arch/riscv/include/asm/asm-prototypes.h
index 81a1f27fa54f..70b86a825922 100644
--- a/arch/riscv/include/asm/asm-prototypes.h
+++ b/arch/riscv/include/asm/asm-prototypes.h
@@ -37,7 +37,7 @@ asmlinkage void riscv_v_context_nesting_end(struct pt_regs *regs);
 
 #endif /* CONFIG_RISCV_ISA_V */
 
-#define DECLARE_DO_ERROR_INFO(name)	asmlinkage void name(struct pt_regs *regs)
+#define DECLARE_DO_ERROR_INFO(name)	void name(struct pt_regs *regs)
 
 DECLARE_DO_ERROR_INFO(do_trap_unknown);
 DECLARE_DO_ERROR_INFO(do_trap_insn_misaligned);
@@ -53,8 +53,8 @@ DECLARE_DO_ERROR_INFO(do_trap_ecall_m);
 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);
+void do_page_fault(struct pt_regs *regs);
+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/kernel/traps.c b/arch/riscv/kernel/traps.c
index b44d4a8d4083..ddca8e74fb72 100644
--- a/arch/riscv/kernel/traps.c
+++ b/arch/riscv/kernel/traps.c
@@ -147,7 +147,7 @@ static void do_trap_error(struct pt_regs *regs, int signo, int code,
 #define __trap_section noinstr
 #endif
 #define DO_ERROR_INFO(name, signo, code, str)					\
-asmlinkage __visible __trap_section void name(struct pt_regs *regs)		\
+__visible __trap_section void name(struct pt_regs *regs)			\
 {										\
 	if (user_mode(regs)) {							\
 		irqentry_enter_from_user_mode(regs);				\
@@ -167,7 +167,7 @@ DO_ERROR_INFO(do_trap_insn_misaligned,
 DO_ERROR_INFO(do_trap_insn_fault,
 	SIGSEGV, SEGV_ACCERR, "instruction access fault");
 
-asmlinkage __visible __trap_section void do_trap_insn_illegal(struct pt_regs *regs)
+__visible __trap_section void do_trap_insn_illegal(struct pt_regs *regs)
 {
 	bool handled;
 
@@ -198,7 +198,7 @@ asmlinkage __visible __trap_section void do_trap_insn_illegal(struct pt_regs *re
 DO_ERROR_INFO(do_trap_load_fault,
 	SIGSEGV, SEGV_ACCERR, "load access fault");
 
-asmlinkage __visible __trap_section void do_trap_load_misaligned(struct pt_regs *regs)
+__visible __trap_section void do_trap_load_misaligned(struct pt_regs *regs)
 {
 	if (user_mode(regs)) {
 		irqentry_enter_from_user_mode(regs);
@@ -219,7 +219,7 @@ asmlinkage __visible __trap_section void do_trap_load_misaligned(struct pt_regs
 	}
 }
 
-asmlinkage __visible __trap_section void do_trap_store_misaligned(struct pt_regs *regs)
+__visible __trap_section void do_trap_store_misaligned(struct pt_regs *regs)
 {
 	if (user_mode(regs)) {
 		irqentry_enter_from_user_mode(regs);
@@ -294,7 +294,7 @@ void handle_break(struct pt_regs *regs)
 		die(regs, "Kernel BUG");
 }
 
-asmlinkage __visible __trap_section void do_trap_break(struct pt_regs *regs)
+__visible __trap_section void do_trap_break(struct pt_regs *regs)
 {
 	if (user_mode(regs)) {
 		irqentry_enter_from_user_mode(regs);
@@ -311,7 +311,7 @@ asmlinkage __visible __trap_section void do_trap_break(struct pt_regs *regs)
 	}
 }
 
-asmlinkage __visible __trap_section  __no_stack_protector
+__visible __trap_section  __no_stack_protector
 void do_trap_ecall_u(struct pt_regs *regs)
 {
 	if (user_mode(regs)) {
@@ -355,7 +355,7 @@ void do_trap_ecall_u(struct pt_regs *regs)
 }
 
 #ifdef CONFIG_MMU
-asmlinkage __visible noinstr void do_page_fault(struct pt_regs *regs)
+__visible noinstr void do_page_fault(struct pt_regs *regs)
 {
 	irqentry_state_t state = irqentry_enter(regs);
 
@@ -378,7 +378,7 @@ static void noinstr handle_riscv_irq(struct pt_regs *regs)
 	irq_exit_rcu();
 }
 
-asmlinkage void noinstr do_irq(struct pt_regs *regs)
+void noinstr do_irq(struct pt_regs *regs)
 {
 	irqentry_state_t state = irqentry_enter(regs);
 
-- 
2.43.0

Re: [PATCH 6/6] riscv: remove asmlinkage from updated functions

[Charlie Jenkins]

On Mon, Jun 17, 2024 at 01:05:53AM +0800, Jisheng Zhang wrote:
> Now that the callers of these functions have moved into C, they no longer
> need the asmlinkage annotation. Remove it

A couple of the functions here can be inlined as well now that they are
not called by assembly, this will allow the compiler to get rid of the
stack setup/tear down code and eliminate a handful of instructions. The
functions that can be inlined are riscv_v_context_nesting_start() and
do_irq().

- Charlie

> 
> Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
> ---
>  arch/riscv/include/asm/asm-prototypes.h |  6 +++---
>  arch/riscv/kernel/traps.c               | 16 ++++++++--------
>  2 files changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/riscv/include/asm/asm-prototypes.h b/arch/riscv/include/asm/asm-prototypes.h
> index 81a1f27fa54f..70b86a825922 100644
> --- a/arch/riscv/include/asm/asm-prototypes.h
> +++ b/arch/riscv/include/asm/asm-prototypes.h
> @@ -37,7 +37,7 @@ asmlinkage void riscv_v_context_nesting_end(struct pt_regs *regs);
>  
>  #endif /* CONFIG_RISCV_ISA_V */
>  
> -#define DECLARE_DO_ERROR_INFO(name)	asmlinkage void name(struct pt_regs *regs)
> +#define DECLARE_DO_ERROR_INFO(name)	void name(struct pt_regs *regs)
>  
>  DECLARE_DO_ERROR_INFO(do_trap_unknown);
>  DECLARE_DO_ERROR_INFO(do_trap_insn_misaligned);
> @@ -53,8 +53,8 @@ DECLARE_DO_ERROR_INFO(do_trap_ecall_m);
>  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);
> +void do_page_fault(struct pt_regs *regs);
> +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/kernel/traps.c b/arch/riscv/kernel/traps.c
> index b44d4a8d4083..ddca8e74fb72 100644
> --- a/arch/riscv/kernel/traps.c
> +++ b/arch/riscv/kernel/traps.c
> @@ -147,7 +147,7 @@ static void do_trap_error(struct pt_regs *regs, int signo, int code,
>  #define __trap_section noinstr
>  #endif
>  #define DO_ERROR_INFO(name, signo, code, str)					\
> -asmlinkage __visible __trap_section void name(struct pt_regs *regs)		\
> +__visible __trap_section void name(struct pt_regs *regs)			\
>  {										\
>  	if (user_mode(regs)) {							\
>  		irqentry_enter_from_user_mode(regs);				\
> @@ -167,7 +167,7 @@ DO_ERROR_INFO(do_trap_insn_misaligned,
>  DO_ERROR_INFO(do_trap_insn_fault,
>  	SIGSEGV, SEGV_ACCERR, "instruction access fault");
>  
> -asmlinkage __visible __trap_section void do_trap_insn_illegal(struct pt_regs *regs)
> +__visible __trap_section void do_trap_insn_illegal(struct pt_regs *regs)
>  {
>  	bool handled;
>  
> @@ -198,7 +198,7 @@ asmlinkage __visible __trap_section void do_trap_insn_illegal(struct pt_regs *re
>  DO_ERROR_INFO(do_trap_load_fault,
>  	SIGSEGV, SEGV_ACCERR, "load access fault");
>  
> -asmlinkage __visible __trap_section void do_trap_load_misaligned(struct pt_regs *regs)
> +__visible __trap_section void do_trap_load_misaligned(struct pt_regs *regs)
>  {
>  	if (user_mode(regs)) {
>  		irqentry_enter_from_user_mode(regs);
> @@ -219,7 +219,7 @@ asmlinkage __visible __trap_section void do_trap_load_misaligned(struct pt_regs
>  	}
>  }
>  
> -asmlinkage __visible __trap_section void do_trap_store_misaligned(struct pt_regs *regs)
> +__visible __trap_section void do_trap_store_misaligned(struct pt_regs *regs)
>  {
>  	if (user_mode(regs)) {
>  		irqentry_enter_from_user_mode(regs);
> @@ -294,7 +294,7 @@ void handle_break(struct pt_regs *regs)
>  		die(regs, "Kernel BUG");
>  }
>  
> -asmlinkage __visible __trap_section void do_trap_break(struct pt_regs *regs)
> +__visible __trap_section void do_trap_break(struct pt_regs *regs)
>  {
>  	if (user_mode(regs)) {
>  		irqentry_enter_from_user_mode(regs);
> @@ -311,7 +311,7 @@ asmlinkage __visible __trap_section void do_trap_break(struct pt_regs *regs)
>  	}
>  }
>  
> -asmlinkage __visible __trap_section  __no_stack_protector
> +__visible __trap_section  __no_stack_protector
>  void do_trap_ecall_u(struct pt_regs *regs)
>  {
>  	if (user_mode(regs)) {
> @@ -355,7 +355,7 @@ void do_trap_ecall_u(struct pt_regs *regs)
>  }
>  
>  #ifdef CONFIG_MMU
> -asmlinkage __visible noinstr void do_page_fault(struct pt_regs *regs)
> +__visible noinstr void do_page_fault(struct pt_regs *regs)
>  {
>  	irqentry_state_t state = irqentry_enter(regs);
>  
> @@ -378,7 +378,7 @@ static void noinstr handle_riscv_irq(struct pt_regs *regs)
>  	irq_exit_rcu();
>  }
>  
> -asmlinkage void noinstr do_irq(struct pt_regs *regs)
> +void noinstr do_irq(struct pt_regs *regs)
>  {
>  	irqentry_state_t state = irqentry_enter(regs);
>  
> -- 
> 2.43.0
> 
> 
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH 0/6] riscv: convert bottom half of exception handling to C

[Cyril Bur]

On Mon, Jun 17, 2024 at 3:21 AM Jisheng Zhang <jszhang@kernel.org> wrote:
>
> For readability, maintainability and future scalability, convert the
> bottom half of the exception handling to C.
>
> During the conversion, I found Anton fixed a performance issue
> and my patches will touch the same exception asm code, so I include
> Anton's patch for completeness. I also cooked a similar patch to avoid
> corrupting the RAS in ret_from_fork() per the inspiration.
>
> 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.
>
>
>
> Anton Blanchard (1):
>   riscv: Improve exception and system call latency
>
I've retested this patch with the rest of the series applied. I can confirm
that the performance improvement is still there. Definitely thumbs up on my end.

> Jisheng Zhang (5):
>   riscv: avoid corrupting the RAS
>   riscv: convert bottom half of exception handling to C
>   riscv: errata: remove ALT_INSN_FAULT and ALT_PAGE_FAULT
>   riscv: errata: sifive: remove NOMMU handling
>   riscv: remove asmlinkage from updated functions
>
>  arch/riscv/errata/sifive/errata.c         | 25 +++++++---
>  arch/riscv/errata/sifive/errata_cip_453.S |  4 --
>  arch/riscv/include/asm/asm-prototypes.h   |  7 +--
>  arch/riscv/include/asm/errata_list.h      | 21 ++------
>  arch/riscv/kernel/entry.S                 | 61 ++---------------------
>  arch/riscv/kernel/stacktrace.c            |  4 +-
>  arch/riscv/kernel/traps.c                 | 57 ++++++++++++++++++---
>  7 files changed, 81 insertions(+), 98 deletions(-)
>
> --
> 2.43.0
>
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH 0/6] riscv: convert bottom half of exception handling to C

[Jisheng Zhang]

On Wed, Jun 19, 2024 at 08:16:58AM +1000, Cyril Bur wrote:
> On Mon, Jun 17, 2024 at 3:21 AM Jisheng Zhang <jszhang@kernel.org> wrote:
> >
> > For readability, maintainability and future scalability, convert the
> > bottom half of the exception handling to C.
> >
> > During the conversion, I found Anton fixed a performance issue
> > and my patches will touch the same exception asm code, so I include
> > Anton's patch for completeness. I also cooked a similar patch to avoid
> > corrupting the RAS in ret_from_fork() per the inspiration.
> >
> > 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.
> >
> >
> >
> > Anton Blanchard (1):
> >   riscv: Improve exception and system call latency
> >
> I've retested this patch with the rest of the series applied. I can confirm
> that the performance improvement is still there. Definitely thumbs up on my end.

Thanks! The 2nd patch is inspired by Anton's patch. The remainings
are just to convert the asm to c in straight forward style.

If possible, could you please add your Tested-by tag? Or even better
review the series ;)

Thanks
> 
> > Jisheng Zhang (5):
> >   riscv: avoid corrupting the RAS
> >   riscv: convert bottom half of exception handling to C
> >   riscv: errata: remove ALT_INSN_FAULT and ALT_PAGE_FAULT
> >   riscv: errata: sifive: remove NOMMU handling
> >   riscv: remove asmlinkage from updated functions
> >
> >  arch/riscv/errata/sifive/errata.c         | 25 +++++++---
> >  arch/riscv/errata/sifive/errata_cip_453.S |  4 --
> >  arch/riscv/include/asm/asm-prototypes.h   |  7 +--
> >  arch/riscv/include/asm/errata_list.h      | 21 ++------
> >  arch/riscv/kernel/entry.S                 | 61 ++---------------------
> >  arch/riscv/kernel/stacktrace.c            |  4 +-
> >  arch/riscv/kernel/traps.c                 | 57 ++++++++++++++++++---
> >  7 files changed, 81 insertions(+), 98 deletions(-)
> >
> > --
> > 2.43.0
> >
> >
> > _______________________________________________
> > linux-riscv mailing list
> > linux-riscv@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [CAUTION - External Sender] Re: [PATCH 0/6] riscv: convert bottom half of exception handling to C

[Cyril Bur]

On Thu, Jun 20, 2024 at 1:25 AM Jisheng Zhang <jszhang@kernel.org> wrote:
>
> On Wed, Jun 19, 2024 at 08:16:58AM +1000, Cyril Bur wrote:
> > On Mon, Jun 17, 2024 at 3:21 AM Jisheng Zhang <jszhang@kernel.org> wrote:
> > >
> > > For readability, maintainability and future scalability, convert the
> > > bottom half of the exception handling to C.
> > >
> > > During the conversion, I found Anton fixed a performance issue
> > > and my patches will touch the same exception asm code, so I include
> > > Anton's patch for completeness. I also cooked a similar patch to avoid
> > > corrupting the RAS in ret_from_fork() per the inspiration.
> > >
> > > 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.
> > >
> > >
> > >
> > > Anton Blanchard (1):
> > >   riscv: Improve exception and system call latency
> > >
> > I've retested this patch with the rest of the series applied. I can confirm
> > that the performance improvement is still there. Definitely thumbs up on my end.
>
> Thanks! The 2nd patch is inspired by Anton's patch. The remainings
> are just to convert the asm to c in straight forward style.
>
> If possible, could you please add your Tested-by tag? Or even better
> review the series ;)
>
I only saw this response after having a look at 3/6. I'm a little
nervous about a full review
as I don't know all the riscv catches yet.

I'll say that the testing of the series wasn't massive. I just booted
it enough to exercise
that codepath and get some numbers. Having said that I'm fine putting
a Tested-by
for the series, it did boot to userspace after all :).

> Thanks
> >
> > > Jisheng Zhang (5):
> > >   riscv: avoid corrupting the RAS
> > >   riscv: convert bottom half of exception handling to C
> > >   riscv: errata: remove ALT_INSN_FAULT and ALT_PAGE_FAULT
> > >   riscv: errata: sifive: remove NOMMU handling
> > >   riscv: remove asmlinkage from updated functions
> > >
> > >  arch/riscv/errata/sifive/errata.c         | 25 +++++++---
> > >  arch/riscv/errata/sifive/errata_cip_453.S |  4 --
> > >  arch/riscv/include/asm/asm-prototypes.h   |  7 +--
> > >  arch/riscv/include/asm/errata_list.h      | 21 ++------
> > >  arch/riscv/kernel/entry.S                 | 61 ++---------------------
> > >  arch/riscv/kernel/stacktrace.c            |  4 +-
> > >  arch/riscv/kernel/traps.c                 | 57 ++++++++++++++++++---
> > >  7 files changed, 81 insertions(+), 98 deletions(-)
> > >
> > > --
> > > 2.43.0
> > >
> > >
> > > _______________________________________________
> > > linux-riscv mailing list
> > > linux-riscv@lists.infradead.org
> > > http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH 0/6] riscv: convert bottom half of exception handling to C

[Deepak Gupta]

On Mon, Jun 17, 2024 at 01:05:47AM +0800, Jisheng Zhang wrote:
>For readability, maintainability and future scalability, convert the
>bottom half of the exception handling to C.
>
>During the conversion, I found Anton fixed a performance issue
>and my patches will touch the same exception asm code, so I include
>Anton's patch for completeness. I also cooked a similar patch to avoid
>corrupting the RAS in ret_from_fork() per the inspiration.

nit: Probably corruption is wrong word here giving the notion that software
got some capability to corrupt uarch structures. It's simply mismatched # of
call and # of rets. Imbalance (instead of calling it corruption) in return
address stack (RAS) leading to incorret predictions on return.

>
>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.
>
>
>
>Anton Blanchard (1):
>  riscv: Improve exception and system call latency
>
>Jisheng Zhang (5):
>  riscv: avoid corrupting the RAS
>  riscv: convert bottom half of exception handling to C
>  riscv: errata: remove ALT_INSN_FAULT and ALT_PAGE_FAULT
>  riscv: errata: sifive: remove NOMMU handling
>  riscv: remove asmlinkage from updated functions
>
> arch/riscv/errata/sifive/errata.c         | 25 +++++++---
> arch/riscv/errata/sifive/errata_cip_453.S |  4 --
> arch/riscv/include/asm/asm-prototypes.h   |  7 +--
> arch/riscv/include/asm/errata_list.h      | 21 ++------
> arch/riscv/kernel/entry.S                 | 61 ++---------------------
> arch/riscv/kernel/stacktrace.c            |  4 +-
> arch/riscv/kernel/traps.c                 | 57 ++++++++++++++++++---
> 7 files changed, 81 insertions(+), 98 deletions(-)
>
>-- 
>2.43.0
>
>

Re: [PATCH 3/6] riscv: convert bottom half of exception handling to C

[Cyril Bur]

On Mon, Jun 17, 2024 at 3:21 AM Jisheng Zhang <jszhang@kernel.org> 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,

Patch looks great. I've been going through to understand, this is more
a learning exercise for me than anything else.
I have a question though.

> 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)
>  {
> --
> 2.43.0
>
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv