diff mbox series

[v2,06/10] lib: sbi: Simplify parameters of sbi_illegal_insn_handler()

Message ID 20240312102804.1436376-7-apatel@ventanamicro.com
State Superseded
Headers show
Series Improve trap handling for nested traps | expand

Commit Message

Anup Patel March 12, 2024, 10:28 a.m. UTC 
The struct sbi_trap_context already has the information needed by
sbi_illegal_insn_handler() so directly pass struct sbi_trap_context
pointer to this function.

Signed-off-by: Anup Patel <apatel@ventanamicro.com>
---
 include/sbi/sbi_illegal_insn.h |  4 ++--
 lib/sbi/sbi_illegal_insn.c     | 14 +++++++-------
 lib/sbi/sbi_trap.c             |  2 +-
 3 files changed, 10 insertions(+), 10 deletions(-)

Comments

Samuel Holland March 12, 2024, 4:56 p.m. UTC | #1 
On 2024-03-12 5:28 AM, Anup Patel wrote:
> The struct sbi_trap_context already has the information needed by
> sbi_illegal_insn_handler() so directly pass struct sbi_trap_context
> pointer to this function.
> 
> Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> ---
>  include/sbi/sbi_illegal_insn.h |  4 ++--
>  lib/sbi/sbi_illegal_insn.c     | 14 +++++++-------
>  lib/sbi/sbi_trap.c             |  2 +-
>  3 files changed, 10 insertions(+), 10 deletions(-)

Reviewed-by: Samuel Holland <samuel.holland@sifive.com>
Clément Léger March 13, 2024, 1:42 p.m. UTC | #2 
On 12/03/2024 11:28, Anup Patel wrote:
> The struct sbi_trap_context already has the information needed by
> sbi_illegal_insn_handler() so directly pass struct sbi_trap_context
> pointer to this function.
> 
> Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> ---
>  include/sbi/sbi_illegal_insn.h |  4 ++--
>  lib/sbi/sbi_illegal_insn.c     | 14 +++++++-------
>  lib/sbi/sbi_trap.c             |  2 +-
>  3 files changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/include/sbi/sbi_illegal_insn.h b/include/sbi/sbi_illegal_insn.h
> index 0397935..7be72ac 100644
> --- a/include/sbi/sbi_illegal_insn.h
> +++ b/include/sbi/sbi_illegal_insn.h
> @@ -12,8 +12,8 @@
>  
>  #include <sbi/sbi_types.h>
>  
> -struct sbi_trap_regs;
> +struct sbi_trap_context;
>  
> -int sbi_illegal_insn_handler(ulong insn, struct sbi_trap_regs *regs);
> +int sbi_illegal_insn_handler(struct sbi_trap_context *tcntx);
>  
>  #endif
> diff --git a/lib/sbi/sbi_illegal_insn.c b/lib/sbi/sbi_illegal_insn.c
> index dd0b3c1..29c5d74 100644
> --- a/lib/sbi/sbi_illegal_insn.c
> +++ b/lib/sbi/sbi_illegal_insn.c
> @@ -136,8 +136,9 @@ static const illegal_insn_func illegal_insn_table[32] = {
>  	truly_illegal_insn  /* 31 */
>  };
>  
> -int sbi_illegal_insn_handler(ulong insn, struct sbi_trap_regs *regs)
> +int sbi_illegal_insn_handler(struct sbi_trap_context *tcntx)
>  {
> +	ulong insn = tcntx->trap.tval;

Maybe add some intermediate variable:

	struct sbi_trap_regs *regs = &tcntx->regs;

So you won't have to modify anything below.

Regards,

Clément

>  	struct sbi_trap_info uptrap;
>  
>  	/*
> @@ -153,13 +154,12 @@ int sbi_illegal_insn_handler(ulong insn, struct sbi_trap_regs *regs)
>  
>  	sbi_pmu_ctr_incr_fw(SBI_PMU_FW_ILLEGAL_INSN);
>  	if (unlikely((insn & 3) != 3)) {
> -		insn = sbi_get_insn(regs->mepc, &uptrap);
> -		if (uptrap.cause) {
> -			return sbi_trap_redirect(regs, &uptrap);
> -		}
> +		insn = sbi_get_insn(tcntx->regs.mepc, &uptrap);
> +		if (uptrap.cause)
> +			return sbi_trap_redirect(&tcntx->regs, &uptrap);
>  		if ((insn & 3) != 3)
> -			return truly_illegal_insn(insn, regs);
> +			return truly_illegal_insn(insn, &tcntx->regs);
>  	}
>  
> -	return illegal_insn_table[(insn & 0x7c) >> 2](insn, regs);
> +	return illegal_insn_table[(insn & 0x7c) >> 2](insn, &tcntx->regs);
>  }
> diff --git a/lib/sbi/sbi_trap.c b/lib/sbi/sbi_trap.c
> index a2afb0a..4e691df 100644
> --- a/lib/sbi/sbi_trap.c
> +++ b/lib/sbi/sbi_trap.c
> @@ -285,7 +285,7 @@ struct sbi_trap_context *sbi_trap_handler(struct sbi_trap_context *tcntx)
>  
>  	switch (mcause) {
>  	case CAUSE_ILLEGAL_INSTRUCTION:
> -		rc  = sbi_illegal_insn_handler(tcntx->trap.tval, regs);
> +		rc  = sbi_illegal_insn_handler(tcntx);
>  		msg = "illegal instruction handler failed";
>  		break;
>  	case CAUSE_MISALIGNED_LOAD:
Anup Patel March 17, 2024, 11:28 a.m. UTC | #3 
On Wed, Mar 13, 2024 at 7:12 PM Clément Léger <cleger@rivosinc.com> wrote:
>
>
>
> On 12/03/2024 11:28, Anup Patel wrote:
> > The struct sbi_trap_context already has the information needed by
> > sbi_illegal_insn_handler() so directly pass struct sbi_trap_context
> > pointer to this function.
> >
> > Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> > ---
> >  include/sbi/sbi_illegal_insn.h |  4 ++--
> >  lib/sbi/sbi_illegal_insn.c     | 14 +++++++-------
> >  lib/sbi/sbi_trap.c             |  2 +-
> >  3 files changed, 10 insertions(+), 10 deletions(-)
> >
> > diff --git a/include/sbi/sbi_illegal_insn.h b/include/sbi/sbi_illegal_insn.h
> > index 0397935..7be72ac 100644
> > --- a/include/sbi/sbi_illegal_insn.h
> > +++ b/include/sbi/sbi_illegal_insn.h
> > @@ -12,8 +12,8 @@
> >
> >  #include <sbi/sbi_types.h>
> >
> > -struct sbi_trap_regs;
> > +struct sbi_trap_context;
> >
> > -int sbi_illegal_insn_handler(ulong insn, struct sbi_trap_regs *regs);
> > +int sbi_illegal_insn_handler(struct sbi_trap_context *tcntx);
> >
> >  #endif
> > diff --git a/lib/sbi/sbi_illegal_insn.c b/lib/sbi/sbi_illegal_insn.c
> > index dd0b3c1..29c5d74 100644
> > --- a/lib/sbi/sbi_illegal_insn.c
> > +++ b/lib/sbi/sbi_illegal_insn.c
> > @@ -136,8 +136,9 @@ static const illegal_insn_func illegal_insn_table[32] = {
> >       truly_illegal_insn  /* 31 */
> >  };
> >
> > -int sbi_illegal_insn_handler(ulong insn, struct sbi_trap_regs *regs)
> > +int sbi_illegal_insn_handler(struct sbi_trap_context *tcntx)
> >  {
> > +     ulong insn = tcntx->trap.tval;
>
> Maybe add some intermediate variable:
>
>         struct sbi_trap_regs *regs = &tcntx->regs;
>
> So you won't have to modify anything below.

OKay, I will update.

Regards,
Anup

>
> Regards,
>
> Clément
>
> >       struct sbi_trap_info uptrap;
> >
> >       /*
> > @@ -153,13 +154,12 @@ int sbi_illegal_insn_handler(ulong insn, struct sbi_trap_regs *regs)
> >
> >       sbi_pmu_ctr_incr_fw(SBI_PMU_FW_ILLEGAL_INSN);
> >       if (unlikely((insn & 3) != 3)) {
> > -             insn = sbi_get_insn(regs->mepc, &uptrap);
> > -             if (uptrap.cause) {
> > -                     return sbi_trap_redirect(regs, &uptrap);
> > -             }
> > +             insn = sbi_get_insn(tcntx->regs.mepc, &uptrap);
> > +             if (uptrap.cause)
> > +                     return sbi_trap_redirect(&tcntx->regs, &uptrap);
> >               if ((insn & 3) != 3)
> > -                     return truly_illegal_insn(insn, regs);
> > +                     return truly_illegal_insn(insn, &tcntx->regs);
> >       }
> >
> > -     return illegal_insn_table[(insn & 0x7c) >> 2](insn, regs);
> > +     return illegal_insn_table[(insn & 0x7c) >> 2](insn, &tcntx->regs);
> >  }
> > diff --git a/lib/sbi/sbi_trap.c b/lib/sbi/sbi_trap.c
> > index a2afb0a..4e691df 100644
> > --- a/lib/sbi/sbi_trap.c
> > +++ b/lib/sbi/sbi_trap.c
> > @@ -285,7 +285,7 @@ struct sbi_trap_context *sbi_trap_handler(struct sbi_trap_context *tcntx)
> >
> >       switch (mcause) {
> >       case CAUSE_ILLEGAL_INSTRUCTION:
> > -             rc  = sbi_illegal_insn_handler(tcntx->trap.tval, regs);
> > +             rc  = sbi_illegal_insn_handler(tcntx);
> >               msg = "illegal instruction handler failed";
> >               break;
> >       case CAUSE_MISALIGNED_LOAD:
diff mbox series

Patch

diff --git a/include/sbi/sbi_illegal_insn.h b/include/sbi/sbi_illegal_insn.h
index 0397935..7be72ac 100644
--- a/include/sbi/sbi_illegal_insn.h
+++ b/include/sbi/sbi_illegal_insn.h
@@ -12,8 +12,8 @@ 
 
 #include <sbi/sbi_types.h>
 
-struct sbi_trap_regs;
+struct sbi_trap_context;
 
-int sbi_illegal_insn_handler(ulong insn, struct sbi_trap_regs *regs);
+int sbi_illegal_insn_handler(struct sbi_trap_context *tcntx);
 
 #endif
diff --git a/lib/sbi/sbi_illegal_insn.c b/lib/sbi/sbi_illegal_insn.c
index dd0b3c1..29c5d74 100644
--- a/lib/sbi/sbi_illegal_insn.c
+++ b/lib/sbi/sbi_illegal_insn.c
@@ -136,8 +136,9 @@  static const illegal_insn_func illegal_insn_table[32] = {
 	truly_illegal_insn  /* 31 */
 };
 
-int sbi_illegal_insn_handler(ulong insn, struct sbi_trap_regs *regs)
+int sbi_illegal_insn_handler(struct sbi_trap_context *tcntx)
 {
+	ulong insn = tcntx->trap.tval;
 	struct sbi_trap_info uptrap;
 
 	/*
@@ -153,13 +154,12 @@  int sbi_illegal_insn_handler(ulong insn, struct sbi_trap_regs *regs)
 
 	sbi_pmu_ctr_incr_fw(SBI_PMU_FW_ILLEGAL_INSN);
 	if (unlikely((insn & 3) != 3)) {
-		insn = sbi_get_insn(regs->mepc, &uptrap);
-		if (uptrap.cause) {
-			return sbi_trap_redirect(regs, &uptrap);
-		}
+		insn = sbi_get_insn(tcntx->regs.mepc, &uptrap);
+		if (uptrap.cause)
+			return sbi_trap_redirect(&tcntx->regs, &uptrap);
 		if ((insn & 3) != 3)
-			return truly_illegal_insn(insn, regs);
+			return truly_illegal_insn(insn, &tcntx->regs);
 	}
 
-	return illegal_insn_table[(insn & 0x7c) >> 2](insn, regs);
+	return illegal_insn_table[(insn & 0x7c) >> 2](insn, &tcntx->regs);
 }
diff --git a/lib/sbi/sbi_trap.c b/lib/sbi/sbi_trap.c
index a2afb0a..4e691df 100644
--- a/lib/sbi/sbi_trap.c
+++ b/lib/sbi/sbi_trap.c
@@ -285,7 +285,7 @@  struct sbi_trap_context *sbi_trap_handler(struct sbi_trap_context *tcntx)
 
 	switch (mcause) {
 	case CAUSE_ILLEGAL_INSTRUCTION:
-		rc  = sbi_illegal_insn_handler(tcntx->trap.tval, regs);
+		rc  = sbi_illegal_insn_handler(tcntx);
 		msg = "illegal instruction handler failed";
 		break;
 	case CAUSE_MISALIGNED_LOAD: