All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] Add a dummy register to sbi_trap_regs
@ 2025-01-09 23:52 Chao-ying Fu
  2025-01-10  0:30 ` Jessica Clarke
                   ` (3 more replies)
  0 siblings, 4 replies; 11+ messages in thread
From: Chao-ying Fu @ 2025-01-09 23:52 UTC (permalink / raw)
  To: opensbi

SBI_TRAP_CONTEXT_SIZE becomes a multiple of 8 bytes for RV32
or 16 bytes for RV64, so we have sp aligned to 8 or 16 bytes
to have better performance on some platforms,
---
 include/sbi/sbi_trap.h | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/include/sbi/sbi_trap.h b/include/sbi/sbi_trap.h
index d5182bf..df99d8e 100644
--- a/include/sbi/sbi_trap.h
+++ b/include/sbi/sbi_trap.h
@@ -84,8 +84,10 @@
 #define SBI_TRAP_REGS_mstatus			33
 /** Index of mstatusH member in sbi_trap_regs */
 #define SBI_TRAP_REGS_mstatusH			34
+/** Index of a dummy register */
+#define SBI_TRAP_REGS_dummy			35
 /** Last member index in sbi_trap_regs */
-#define SBI_TRAP_REGS_last			35
+#define SBI_TRAP_REGS_last			36
 
 /** Index of cause member in sbi_trap_info */
 #define SBI_TRAP_INFO_cause			0
@@ -194,6 +196,8 @@ struct sbi_trap_regs {
 	unsigned long mstatus;
 	/** mstatusH register state (only for 32-bit) */
 	unsigned long mstatusH;
+	/** a dummy register */
+	unsigned long dummy;
 };
 
 /** Representation of trap details */
-- 
2.47.1



^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH] Add a dummy register to sbi_trap_regs
  2025-01-09 23:52 [PATCH] Add a dummy register to sbi_trap_regs Chao-ying Fu
@ 2025-01-10  0:30 ` Jessica Clarke
  2025-01-10  1:44   ` Chao-ying Fu
  2025-02-06 19:22 ` [PATCH v2] Align SBI trap registers to a nice boundary for performance reasons Raj Vishwanathan
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 11+ messages in thread
From: Jessica Clarke @ 2025-01-10  0:30 UTC (permalink / raw)
  To: opensbi

On 9 Jan 2025, at 23:52, Chao-ying Fu <icebergfu@gmail.com> wrote:
> 
> SBI_TRAP_CONTEXT_SIZE becomes a multiple of 8 bytes for RV32
> or 16 bytes for RV64, so we have sp aligned to 8 or 16 bytes
> to have better performance on some platforms,
> ---
> include/sbi/sbi_trap.h | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/include/sbi/sbi_trap.h b/include/sbi/sbi_trap.h
> index d5182bf..df99d8e 100644
> --- a/include/sbi/sbi_trap.h
> +++ b/include/sbi/sbi_trap.h
> @@ -84,8 +84,10 @@
> #define SBI_TRAP_REGS_mstatus 33
> /** Index of mstatusH member in sbi_trap_regs */
> #define SBI_TRAP_REGS_mstatusH 34
> +/** Index of a dummy register */
> +#define SBI_TRAP_REGS_dummy 35
> /** Last member index in sbi_trap_regs */
> -#define SBI_TRAP_REGS_last 35
> +#define SBI_TRAP_REGS_last 36
> 
> /** Index of cause member in sbi_trap_info */
> #define SBI_TRAP_INFO_cause 0
> @@ -194,6 +196,8 @@ struct sbi_trap_regs {
> unsigned long mstatus;
> /** mstatusH register state (only for 32-bit) */
> unsigned long mstatusH;
> + /** a dummy register */
> + unsigned long dummy;

This is ugly and fragile for when changes are made to the structure.
Either make SBI_TRAP_CONTEXT_SIZE round up the size or, if for some
reason that doesn?t work, you can always make the struct type
overaligned.

Also, sp is aligned to 16 bytes anyway, that?s mandated by the ABI. So
I don?t understand your justification.

Strong NAK.

Jess



^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH] Add a dummy register to sbi_trap_regs
  2025-01-10  0:30 ` Jessica Clarke
@ 2025-01-10  1:44   ` Chao-ying Fu
  0 siblings, 0 replies; 11+ messages in thread
From: Chao-ying Fu @ 2025-01-10  1:44 UTC (permalink / raw)
  To: opensbi

On Thu, Jan 9, 2025 at 4:30?PM Jessica Clarke <jrtc27@jrtc27.com> wrote:
>
> On 9 Jan 2025, at 23:52, Chao-ying Fu <icebergfu@gmail.com> wrote:
> >
> > SBI_TRAP_CONTEXT_SIZE becomes a multiple of 8 bytes for RV32
> > or 16 bytes for RV64, so we have sp aligned to 8 or 16 bytes
> > to have better performance on some platforms,
> > ---
> > include/sbi/sbi_trap.h | 6 +++++-
> > 1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/sbi/sbi_trap.h b/include/sbi/sbi_trap.h
> > index d5182bf..df99d8e 100644
> > --- a/include/sbi/sbi_trap.h
> > +++ b/include/sbi/sbi_trap.h
> > @@ -84,8 +84,10 @@
> > #define SBI_TRAP_REGS_mstatus 33
> > /** Index of mstatusH member in sbi_trap_regs */
> > #define SBI_TRAP_REGS_mstatusH 34
> > +/** Index of a dummy register */
> > +#define SBI_TRAP_REGS_dummy 35
> > /** Last member index in sbi_trap_regs */
> > -#define SBI_TRAP_REGS_last 35
> > +#define SBI_TRAP_REGS_last 36
> >
> > /** Index of cause member in sbi_trap_info */
> > #define SBI_TRAP_INFO_cause 0
> > @@ -194,6 +196,8 @@ struct sbi_trap_regs {
> > unsigned long mstatus;
> > /** mstatusH register state (only for 32-bit) */
> > unsigned long mstatusH;
> > + /** a dummy register */
> > + unsigned long dummy;
>
> This is ugly and fragile for when changes are made to the structure.
> Either make SBI_TRAP_CONTEXT_SIZE round up the size or, if for some
> reason that doesn?t work, you can always make the struct type
> overaligned.
>
> Also, sp is aligned to 16 bytes anyway, that?s mandated by the ABI. So
> I don?t understand your justification.
>
> Strong NAK.
>
> Jess
>
Yes. The patch can be revised to be more solid. We can just change the
define for SBI_TRAP_CONTEXT_SIZE to 16*x bytes for RV64, for example.
Note that the original sp is aligned to 16 bytes. However, there is
code in fw_base.S as follows.
        /*
         * Set T0 to appropriate exception stack
         *
         * Came_From_M_Mode = ((MSTATUS.MPP < PRV_M) ? 1 : 0) - 1;
         * Exception_Stack = TP ^ (Came_From_M_Mode & (SP ^ TP))
         *
         * Came_From_M_Mode = 0    ==>    Exception_Stack = TP
         * Came_From_M_Mode = -1   ==>    Exception_Stack = SP
         */
        csrr    t0, CSR_MSTATUS
        srl     t0, t0, MSTATUS_MPP_SHIFT
        and     t0, t0, PRV_M
        slti    t0, t0, PRV_M
        add     t0, t0, -1
        xor     sp, sp, tp
        and     t0, t0, sp
        xor     sp, sp, tp
        xor     t0, tp, t0

        /* Save original SP on exception stack */
        REG_S   sp, (SBI_TRAP_REGS_OFFSET(sp) - SBI_TRAP_CONTEXT_SIZE)(t0)

        /* Set SP to exception stack and make room for trap context */
        add     sp, t0, -(SBI_TRAP_CONTEXT_SIZE)

After this add instruction, sp may not be aligned to 16 bytes any more.
Thanks!


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v2] Align SBI trap registers to a nice boundary for performance reasons.
  2025-01-09 23:52 [PATCH] Add a dummy register to sbi_trap_regs Chao-ying Fu
  2025-01-10  0:30 ` Jessica Clarke
@ 2025-02-06 19:22 ` Raj Vishwanathan
  2025-02-08  5:34   ` Samuel Holland
  2025-02-10 22:47 ` Raj Vishwanathan
  2025-02-11 21:46 ` [PATCH v2] include: sbi: Align SBI trap registers to a nice boundary Raj Vishwanathan
  3 siblings, 1 reply; 11+ messages in thread
From: Raj Vishwanathan @ 2025-02-06 19:22 UTC (permalink / raw)
  To: opensbi

Align SBI_TRAP_CONTEXT_SIZE to a multiple of 16 bytes. If it is not
aligned to 16 bytes for RV64, it can create performance problems.
Aligning it correctly can fix the performance issues.

Signed-off-by: Raj Vishwanathan <Raj.Vishwanathan@gmail.com>
--
Test: Performance issues seen and fix Verified on FPGA.
Other methods.
   Run qemu with monitor to check the SP during cpu_in
   and cpu_out.
   Add sbi_printf to the function sbi_trap_handler to check
   the alignment of sbi_trap_context
---
 include/sbi/sbi_trap.h | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/include/sbi/sbi_trap.h b/include/sbi/sbi_trap.h
index d5182bf..82e9c08 100644
--- a/include/sbi/sbi_trap.h
+++ b/include/sbi/sbi_trap.h
@@ -112,13 +112,16 @@
 /** Size (in bytes) of sbi_trap_info */
 #define SBI_TRAP_INFO_SIZE SBI_TRAP_INFO_OFFSET(last)
 
+/** Stack pointer is aligned to 16 bytes */
+#define STACK_BOUNDARY            16
+#define ALIGN_TO_BOUNDARY(x,a) (((x) + (a) - 1) & ~((a) - 1))
+
 /** Size (in bytes) of sbi_trap_context */
-#define SBI_TRAP_CONTEXT_SIZE (SBI_TRAP_REGS_SIZE + \
-			       SBI_TRAP_INFO_SIZE + \
-			       __SIZEOF_POINTER__)
+#define SBI_TRAP_CONTEXT_SIZE ALIGN_TO_BOUNDARY((SBI_TRAP_REGS_SIZE + \
+                               SBI_TRAP_INFO_SIZE + \
+                               __SIZEOF_POINTER__),STACK_BOUNDARY)
 
 #ifndef __ASSEMBLER__
-
 #include <sbi/sbi_types.h>
 #include <sbi/sbi_scratch.h>
 
-- 
2.43.0



^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH v2] Align SBI trap registers to a nice boundary for performance reasons.
  2025-02-06 19:22 ` [PATCH v2] Align SBI trap registers to a nice boundary for performance reasons Raj Vishwanathan
@ 2025-02-08  5:34   ` Samuel Holland
  2025-02-10 22:54     ` [EXTERNAL]Re: " Raj Vishwanathan
  0 siblings, 1 reply; 11+ messages in thread
From: Samuel Holland @ 2025-02-08  5:34 UTC (permalink / raw)
  To: opensbi

Hi Raj,

This looks like the right approach. A few comments below on the mechanics of the
patch.

On 2025-02-06 1:22 PM, Raj Vishwanathan wrote:
> Align SBI_TRAP_CONTEXT_SIZE to a multiple of 16 bytes. If it is not
> aligned to 16 bytes for RV64, it can create performance problems.
> Aligning it correctly can fix the performance issues.
> 
> Signed-off-by: Raj Vishwanathan <Raj.Vishwanathan@gmail.com>
> --
> Test: Performance issues seen and fix Verified on FPGA.
> Other methods.
>    Run qemu with monitor to check the SP during cpu_in
>    and cpu_out.
>    Add sbi_printf to the function sbi_trap_handler to check
>    the alignment of sbi_trap_context
> ---
>  include/sbi/sbi_trap.h | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/include/sbi/sbi_trap.h b/include/sbi/sbi_trap.h
> index d5182bf..82e9c08 100644
> --- a/include/sbi/sbi_trap.h
> +++ b/include/sbi/sbi_trap.h
> @@ -112,13 +112,16 @@
>  /** Size (in bytes) of sbi_trap_info */
>  #define SBI_TRAP_INFO_SIZE SBI_TRAP_INFO_OFFSET(last)
>  
> +/** Stack pointer is aligned to 16 bytes */
> +#define STACK_BOUNDARY            16
> +#define ALIGN_TO_BOUNDARY(x,a) (((x) + (a) - 1) & ~((a) - 1))

This is the same as the BIT_ALIGN macro that already exists in sbi_bitops.h.

> +
>  /** Size (in bytes) of sbi_trap_context */
> -#define SBI_TRAP_CONTEXT_SIZE (SBI_TRAP_REGS_SIZE + \
> -			       SBI_TRAP_INFO_SIZE + \
> -			       __SIZEOF_POINTER__)
> +#define SBI_TRAP_CONTEXT_SIZE ALIGN_TO_BOUNDARY((SBI_TRAP_REGS_SIZE + \
> +                               SBI_TRAP_INFO_SIZE + \
> +                               __SIZEOF_POINTER__),STACK_BOUNDARY)

Please follow the existing style (spaces between arguments).

>  
>  #ifndef __ASSEMBLER__
> -

Please don't include unrelated changes in the patch.

Regards,
Samuel

>  #include <sbi/sbi_types.h>
>  #include <sbi/sbi_scratch.h>
>  



^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v2] Align SBI trap registers to a nice boundary for performance reasons.
  2025-01-09 23:52 [PATCH] Add a dummy register to sbi_trap_regs Chao-ying Fu
  2025-01-10  0:30 ` Jessica Clarke
  2025-02-06 19:22 ` [PATCH v2] Align SBI trap registers to a nice boundary for performance reasons Raj Vishwanathan
@ 2025-02-10 22:47 ` Raj Vishwanathan
  2025-02-11  2:49   ` Xiang W
  2025-02-11  4:48   ` Anup Patel
  2025-02-11 21:46 ` [PATCH v2] include: sbi: Align SBI trap registers to a nice boundary Raj Vishwanathan
  3 siblings, 2 replies; 11+ messages in thread
From: Raj Vishwanathan @ 2025-02-10 22:47 UTC (permalink / raw)
  To: opensbi

Align SBI_TRAP_CONTEXT_SIZE to a multiple of 16 bytes. If it is not
aligned to 16 bytes for RV64, it can create performance problems.
Aligning it correctly can fix the performance issues.

Signed-off-by: Raj Vishwanathan <Raj.Vishwanathan@gmail.com>
---
Test: Performance issues seen and fix Verified on FPGA.
Other methods.
    Run qemu with monitor to check the SP during cpu_in
    and cpu_out.
    Add sbi_printf to the function sbi_trap_handler to check
    the alignment of sbi_trap_context
---
 include/sbi/sbi_trap.h | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/include/sbi/sbi_trap.h b/include/sbi/sbi_trap.h
index d5182bf..5eec4da 100644
--- a/include/sbi/sbi_trap.h
+++ b/include/sbi/sbi_trap.h
@@ -112,10 +112,13 @@
 /** Size (in bytes) of sbi_trap_info */
 #define SBI_TRAP_INFO_SIZE SBI_TRAP_INFO_OFFSET(last)
 
+#define STACK_BOUNDARY 16
+#define ALIGN_TO_BOUNDARY(x, a) (((x) + (a) - 1) & ~((a) - 1))
+
 /** Size (in bytes) of sbi_trap_context */
-#define SBI_TRAP_CONTEXT_SIZE (SBI_TRAP_REGS_SIZE + \
+#define SBI_TRAP_CONTEXT_SIZE ALIGN_TO_BOUNDARY((SBI_TRAP_REGS_SIZE + \
 			       SBI_TRAP_INFO_SIZE + \
-			       __SIZEOF_POINTER__)
+			       __SIZEOF_POINTER__), STACK_BOUNDARY)
 
 #ifndef __ASSEMBLER__
 
-- 
2.43.0



^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [EXTERNAL]Re: [PATCH v2] Align SBI trap registers to a nice boundary for performance reasons.
  2025-02-08  5:34   ` Samuel Holland
@ 2025-02-10 22:54     ` Raj Vishwanathan
  0 siblings, 0 replies; 11+ messages in thread
From: Raj Vishwanathan @ 2025-02-10 22:54 UTC (permalink / raw)
  To: opensbi

Hi Samuel

Thanks for your review.
1. did look at the BIT_ALIGN macro. The sbi_bitops.h seems to be available for 'C' files  and not for assembly files. The alignment I needed is used fw_base.S 
2 &3 . I have added the space and deleted the unnecessary line

I have sent an updated patch. 

Raj

-----Original Message-----
From: Samuel Holland <samuel.holland@sifive.com> 
Sent: Friday, February 7, 2025 9:34 PM
To: Raj Vishwanathan <raj.vishwanathan@gmail.com>; opensbi at lists.infradead.org
Cc: Raj Vishwanathan <rvishwanathan@mips.com>
Subject: [EXTERNAL]Re: [PATCH v2] Align SBI trap registers to a nice boundary for performance reasons.

[You don't often get email from samuel.holland at sifive.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]

Hi Raj,

This looks like the right approach. A few comments below on the mechanics of the patch.

On 2025-02-06 1:22 PM, Raj Vishwanathan wrote:
> Align SBI_TRAP_CONTEXT_SIZE to a multiple of 16 bytes. If it is not 
> aligned to 16 bytes for RV64, it can create performance problems.
> Aligning it correctly can fix the performance issues.
>
> Signed-off-by: Raj Vishwanathan <Raj.Vishwanathan@gmail.com>
> --
> Test: Performance issues seen and fix Verified on FPGA.
> Other methods.
>    Run qemu with monitor to check the SP during cpu_in
>    and cpu_out.
>    Add sbi_printf to the function sbi_trap_handler to check
>    the alignment of sbi_trap_context
> ---
>  include/sbi/sbi_trap.h | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/include/sbi/sbi_trap.h b/include/sbi/sbi_trap.h index 
> d5182bf..82e9c08 100644
> --- a/include/sbi/sbi_trap.h
> +++ b/include/sbi/sbi_trap.h
> @@ -112,13 +112,16 @@
>  /** Size (in bytes) of sbi_trap_info */  #define SBI_TRAP_INFO_SIZE 
> SBI_TRAP_INFO_OFFSET(last)
>
> +/** Stack pointer is aligned to 16 bytes */
> +#define STACK_BOUNDARY            16
> +#define ALIGN_TO_BOUNDARY(x,a) (((x) + (a) - 1) & ~((a) - 1))

This is the same as the BIT_ALIGN macro that already exists in sbi_bitops.h.

> +
>  /** Size (in bytes) of sbi_trap_context */ -#define 
> SBI_TRAP_CONTEXT_SIZE (SBI_TRAP_REGS_SIZE + \
> -                            SBI_TRAP_INFO_SIZE + \
> -                            __SIZEOF_POINTER__)
> +#define SBI_TRAP_CONTEXT_SIZE ALIGN_TO_BOUNDARY((SBI_TRAP_REGS_SIZE + \
> +                               SBI_TRAP_INFO_SIZE + \
> +                               __SIZEOF_POINTER__),STACK_BOUNDARY)

Please follow the existing style (spaces between arguments).

>
>  #ifndef __ASSEMBLER__
> -

Please don't include unrelated changes in the patch.

Regards,
Samuel

>  #include <sbi/sbi_types.h>
>  #include <sbi/sbi_scratch.h>
>


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v2] Align SBI trap registers to a nice boundary for performance reasons.
  2025-02-10 22:47 ` Raj Vishwanathan
@ 2025-02-11  2:49   ` Xiang W
  2025-02-11  4:48   ` Anup Patel
  1 sibling, 0 replies; 11+ messages in thread
From: Xiang W @ 2025-02-11  2:49 UTC (permalink / raw)
  To: opensbi

? 2025-02-10?? 14:47 -0800?Raj Vishwanathan???
> Align SBI_TRAP_CONTEXT_SIZE to a multiple of 16 bytes. If it is not
> aligned to 16 bytes for RV64, it can create performance problems.
> Aligning it correctly can fix the performance issues.
> 
> Signed-off-by: Raj Vishwanathan <Raj.Vishwanathan@gmail.com>
> ---
> Test: Performance issues seen and fix Verified on FPGA.
> Other methods.
> ??? Run qemu with monitor to check the SP during cpu_in
> ??? and cpu_out.
> ??? Add sbi_printf to the function sbi_trap_handler to check
> ??? the alignment of sbi_trap_context
> ---
> ?include/sbi/sbi_trap.h | 7 +++++--
> ?1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/include/sbi/sbi_trap.h b/include/sbi/sbi_trap.h
> index d5182bf..5eec4da 100644
> --- a/include/sbi/sbi_trap.h
> +++ b/include/sbi/sbi_trap.h
> @@ -112,10 +112,13 @@
> ?/** Size (in bytes) of sbi_trap_info */
> ?#define SBI_TRAP_INFO_SIZE SBI_TRAP_INFO_OFFSET(last)
> ?
> +#define STACK_BOUNDARY 16
> +#define ALIGN_TO_BOUNDARY(x, a) (((x) + (a) - 1) & ~((a) - 1))
This can be replace with BOUNDUP in sbi_types.h

Regards,
Xiang W
> +
> ?/** Size (in bytes) of sbi_trap_context */
> -#define SBI_TRAP_CONTEXT_SIZE (SBI_TRAP_REGS_SIZE + \
> +#define SBI_TRAP_CONTEXT_SIZE ALIGN_TO_BOUNDARY((SBI_TRAP_REGS_SIZE + \
> ?			?????? SBI_TRAP_INFO_SIZE + \
> -			?????? __SIZEOF_POINTER__)
> +			?????? __SIZEOF_POINTER__), STACK_BOUNDARY)
> ?
> ?#ifndef __ASSEMBLER__
> ?
> -- 
> 2.43.0
> 
> 



^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v2] Align SBI trap registers to a nice boundary for performance reasons.
  2025-02-10 22:47 ` Raj Vishwanathan
  2025-02-11  2:49   ` Xiang W
@ 2025-02-11  4:48   ` Anup Patel
  1 sibling, 0 replies; 11+ messages in thread
From: Anup Patel @ 2025-02-11  4:48 UTC (permalink / raw)
  To: opensbi

On Tue, Feb 11, 2025 at 4:41?AM Raj Vishwanathan
<raj.vishwanathan@gmail.com> wrote:
>
> Align SBI_TRAP_CONTEXT_SIZE to a multiple of 16 bytes. If it is not
> aligned to 16 bytes for RV64, it can create performance problems.
> Aligning it correctly can fix the performance issues.
>
> Signed-off-by: Raj Vishwanathan <Raj.Vishwanathan@gmail.com>

Simplify the patch subject and use "include: sbi:" as subject prefix.

Regards,
Anup

> ---
> Test: Performance issues seen and fix Verified on FPGA.
> Other methods.
>     Run qemu with monitor to check the SP during cpu_in
>     and cpu_out.
>     Add sbi_printf to the function sbi_trap_handler to check
>     the alignment of sbi_trap_context
> ---
>  include/sbi/sbi_trap.h | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/include/sbi/sbi_trap.h b/include/sbi/sbi_trap.h
> index d5182bf..5eec4da 100644
> --- a/include/sbi/sbi_trap.h
> +++ b/include/sbi/sbi_trap.h
> @@ -112,10 +112,13 @@
>  /** Size (in bytes) of sbi_trap_info */
>  #define SBI_TRAP_INFO_SIZE SBI_TRAP_INFO_OFFSET(last)
>
> +#define STACK_BOUNDARY 16
> +#define ALIGN_TO_BOUNDARY(x, a) (((x) + (a) - 1) & ~((a) - 1))
> +
>  /** Size (in bytes) of sbi_trap_context */
> -#define SBI_TRAP_CONTEXT_SIZE (SBI_TRAP_REGS_SIZE + \
> +#define SBI_TRAP_CONTEXT_SIZE ALIGN_TO_BOUNDARY((SBI_TRAP_REGS_SIZE + \
>                                SBI_TRAP_INFO_SIZE + \
> -                              __SIZEOF_POINTER__)
> +                              __SIZEOF_POINTER__), STACK_BOUNDARY)
>
>  #ifndef __ASSEMBLER__
>
> --
> 2.43.0
>
>
> --
> opensbi mailing list
> opensbi at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/opensbi


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v2] include: sbi: Align SBI trap registers to a nice boundary
  2025-01-09 23:52 [PATCH] Add a dummy register to sbi_trap_regs Chao-ying Fu
                   ` (2 preceding siblings ...)
  2025-02-10 22:47 ` Raj Vishwanathan
@ 2025-02-11 21:46 ` Raj Vishwanathan
  2025-02-12  4:00   ` Anup Patel
  3 siblings, 1 reply; 11+ messages in thread
From: Raj Vishwanathan @ 2025-02-11 21:46 UTC (permalink / raw)
  To: opensbi

Align SBI_TRAP_CONTEXT_SIZE to a multiple of 16 bytes. If it is not
aligned to 16 bytes for RV64, it can create performance problems.
Aligning it correctly can fix the performance issues.

Signed-off-by: Raj Vishwanathan <Raj.Vishwanathan@gmail.com>
---
Test: Performance issues seen and fix Verified on FPGA.
Other methods.
    Run qemu with monitor to check the SP during cpu_in
    and cpu_out.
    Add sbi_printf to the function sbi_trap_handler to check
    the alignment of sbi_trap_context
---
 include/sbi/sbi_trap.h | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/include/sbi/sbi_trap.h b/include/sbi/sbi_trap.h
index d5182bf..5eec4da 100644
--- a/include/sbi/sbi_trap.h
+++ b/include/sbi/sbi_trap.h
@@ -112,10 +112,13 @@
 /** Size (in bytes) of sbi_trap_info */
 #define SBI_TRAP_INFO_SIZE SBI_TRAP_INFO_OFFSET(last)
 
+#define STACK_BOUNDARY 16
+#define ALIGN_TO_BOUNDARY(x, a) (((x) + (a) - 1) & ~((a) - 1))
+
 /** Size (in bytes) of sbi_trap_context */
-#define SBI_TRAP_CONTEXT_SIZE (SBI_TRAP_REGS_SIZE + \
+#define SBI_TRAP_CONTEXT_SIZE ALIGN_TO_BOUNDARY((SBI_TRAP_REGS_SIZE + \
 			       SBI_TRAP_INFO_SIZE + \
-			       __SIZEOF_POINTER__)
+			       __SIZEOF_POINTER__), STACK_BOUNDARY)
 
 #ifndef __ASSEMBLER__
 
-- 
2.43.0



^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH v2] include: sbi: Align SBI trap registers to a nice boundary
  2025-02-11 21:46 ` [PATCH v2] include: sbi: Align SBI trap registers to a nice boundary Raj Vishwanathan
@ 2025-02-12  4:00   ` Anup Patel
  0 siblings, 0 replies; 11+ messages in thread
From: Anup Patel @ 2025-02-12  4:00 UTC (permalink / raw)
  To: opensbi

On Wed, Feb 12, 2025 at 3:16?AM Raj Vishwanathan
<raj.vishwanathan@gmail.com> wrote:
>
> Align SBI_TRAP_CONTEXT_SIZE to a multiple of 16 bytes. If it is not
> aligned to 16 bytes for RV64, it can create performance problems.
> Aligning it correctly can fix the performance issues.
>
> Signed-off-by: Raj Vishwanathan <Raj.Vishwanathan@gmail.com>

LGTM.

Reviewed-by: Anup Patel <anup@brainfault.org>

Applied this patch to the riscv/opensbi repo.

Thanks,
Anup

> ---
> Test: Performance issues seen and fix Verified on FPGA.
> Other methods.
>     Run qemu with monitor to check the SP during cpu_in
>     and cpu_out.
>     Add sbi_printf to the function sbi_trap_handler to check
>     the alignment of sbi_trap_context
> ---
>  include/sbi/sbi_trap.h | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/include/sbi/sbi_trap.h b/include/sbi/sbi_trap.h
> index d5182bf..5eec4da 100644
> --- a/include/sbi/sbi_trap.h
> +++ b/include/sbi/sbi_trap.h
> @@ -112,10 +112,13 @@
>  /** Size (in bytes) of sbi_trap_info */
>  #define SBI_TRAP_INFO_SIZE SBI_TRAP_INFO_OFFSET(last)
>
> +#define STACK_BOUNDARY 16
> +#define ALIGN_TO_BOUNDARY(x, a) (((x) + (a) - 1) & ~((a) - 1))
> +
>  /** Size (in bytes) of sbi_trap_context */
> -#define SBI_TRAP_CONTEXT_SIZE (SBI_TRAP_REGS_SIZE + \
> +#define SBI_TRAP_CONTEXT_SIZE ALIGN_TO_BOUNDARY((SBI_TRAP_REGS_SIZE + \
>                                SBI_TRAP_INFO_SIZE + \
> -                              __SIZEOF_POINTER__)
> +                              __SIZEOF_POINTER__), STACK_BOUNDARY)
>
>  #ifndef __ASSEMBLER__
>
> --
> 2.43.0
>
>
> --
> opensbi mailing list
> opensbi at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/opensbi


^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2025-02-12  4:00 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-01-09 23:52 [PATCH] Add a dummy register to sbi_trap_regs Chao-ying Fu
2025-01-10  0:30 ` Jessica Clarke
2025-01-10  1:44   ` Chao-ying Fu
2025-02-06 19:22 ` [PATCH v2] Align SBI trap registers to a nice boundary for performance reasons Raj Vishwanathan
2025-02-08  5:34   ` Samuel Holland
2025-02-10 22:54     ` [EXTERNAL]Re: " Raj Vishwanathan
2025-02-10 22:47 ` Raj Vishwanathan
2025-02-11  2:49   ` Xiang W
2025-02-11  4:48   ` Anup Patel
2025-02-11 21:46 ` [PATCH v2] include: sbi: Align SBI trap registers to a nice boundary Raj Vishwanathan
2025-02-12  4:00   ` Anup Patel

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.