* [PATCH] Align the scratch allocation to 64 bytes
@ 2025-01-10 0:34 Chao-ying Fu
2025-01-10 4:15 ` Xiang W
2025-01-19 20:04 ` [PATCH v2] We add a new configuration CONFIG_SBI_SCRATCH_ALLOC_ALIGNMENT as an int Raj Vishwanathan
0 siblings, 2 replies; 15+ messages in thread
From: Chao-ying Fu @ 2025-01-10 0:34 UTC (permalink / raw)
To: opensbi
We increase the scratch allocation alignment to 64 bytes
as the cache line size, to avoid two atomic variables from
the same cache line that may cause livelock on some platforms.
---
lib/sbi/sbi_scratch.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/lib/sbi/sbi_scratch.c b/lib/sbi/sbi_scratch.c
index ccbbc68..6b161bc 100644
--- a/lib/sbi/sbi_scratch.c
+++ b/lib/sbi/sbi_scratch.c
@@ -14,6 +14,9 @@
#include <sbi/sbi_scratch.h>
#include <sbi/sbi_string.h>
+/* Minimum size and alignment of scratch allocations */
+#define SCRATCH_ALLOC_ALIGN 64
+
u32 last_hartindex_having_scratch = 0;
u32 hartindex_to_hartid_table[SBI_HARTMASK_MAX_BITS + 1] = { -1U };
struct sbi_scratch *hartindex_to_scratch_table[SBI_HARTMASK_MAX_BITS + 1] = { 0 };
@@ -70,8 +73,12 @@ unsigned long sbi_scratch_alloc_offset(unsigned long size)
if (!size)
return 0;
- size += __SIZEOF_POINTER__ - 1;
- size &= ~((unsigned long)__SIZEOF_POINTER__ - 1);
+ /*
+ * We let the allocation align to 64 bytes to avoid livelock on
+ * certain platforms due to atomic variables from the same cache line.
+ */
+ size += SCRATCH_ALLOC_ALIGN - 1;
+ size &= ~((unsigned long)SCRATCH_ALLOC_ALIGN - 1);
spin_lock(&extra_lock);
--
2.47.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH] Align the scratch allocation to 64 bytes
2025-01-10 4:15 ` Xiang W
@ 2025-01-10 2:36 ` Chao-ying Fu
0 siblings, 0 replies; 15+ messages in thread
From: Chao-ying Fu @ 2025-01-10 2:36 UTC (permalink / raw)
To: opensbi
On Thu, Jan 9, 2025 at 8:15?PM Xiang W <wxjstz@126.com> wrote:
>
> ? 2025-01-09?? 16:34 -0800?Chao-ying Fu???
> > We increase the scratch allocation alignment to 64 bytes
> > as the cache line size, to avoid two atomic variables from
> > the same cache line that may cause livelock on some platforms.
> > ---
> > lib/sbi/sbi_scratch.c | 11 +++++++++--
> > 1 file changed, 9 insertions(+), 2 deletions(-)
> >
> > diff --git a/lib/sbi/sbi_scratch.c b/lib/sbi/sbi_scratch.c
> > index ccbbc68..6b161bc 100644
> > --- a/lib/sbi/sbi_scratch.c
> > +++ b/lib/sbi/sbi_scratch.c
> > @@ -14,6 +14,9 @@
> > #include <sbi/sbi_scratch.h>
> > #include <sbi/sbi_string.h>
> >
> > +/* Minimum size and alignment of scratch allocations */
> > +#define SCRATCH_ALLOC_ALIGN 64
> This appears to be a platform-specific need and should not affect all platforms.
> Can be like this:
>
> #ifndef SCRATCH_ALLOC_ALIGN
> #define SCRATCH_ALLOC_ALIGN __SIZEOF_POINTER__
> #endif
>
> Regards,
> Xiang W
Yes, I will revise the patch to use the original alignment, if
SCRATCH_ALLOC_ALIGN is not defined.
Thanks a lot!
Regards,
Chao-ying
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH] Align the scratch allocation to 64 bytes
2025-01-10 0:34 [PATCH] Align the scratch allocation to 64 bytes Chao-ying Fu
@ 2025-01-10 4:15 ` Xiang W
2025-01-10 2:36 ` Chao-ying Fu
2025-01-19 20:04 ` [PATCH v2] We add a new configuration CONFIG_SBI_SCRATCH_ALLOC_ALIGNMENT as an int Raj Vishwanathan
1 sibling, 1 reply; 15+ messages in thread
From: Xiang W @ 2025-01-10 4:15 UTC (permalink / raw)
To: opensbi
? 2025-01-09?? 16:34 -0800?Chao-ying Fu???
> We increase the scratch allocation alignment to 64 bytes
> as the cache line size, to avoid two atomic variables from
> the same cache line that may cause livelock on some platforms.
> ---
> ?lib/sbi/sbi_scratch.c | 11 +++++++++--
> ?1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/lib/sbi/sbi_scratch.c b/lib/sbi/sbi_scratch.c
> index ccbbc68..6b161bc 100644
> --- a/lib/sbi/sbi_scratch.c
> +++ b/lib/sbi/sbi_scratch.c
> @@ -14,6 +14,9 @@
> ?#include <sbi/sbi_scratch.h>
> ?#include <sbi/sbi_string.h>
> ?
> +/* Minimum size and alignment of scratch allocations */
> +#define SCRATCH_ALLOC_ALIGN 64
This appears to be a platform-specific need and should not affect all platforms.
Can be like this:
#ifndef SCRATCH_ALLOC_ALIGN
#define SCRATCH_ALLOC_ALIGN __SIZEOF_POINTER__
#endif
Regards,
Xiang W
> +
> ?u32 last_hartindex_having_scratch = 0;
> ?u32 hartindex_to_hartid_table[SBI_HARTMASK_MAX_BITS + 1] = { -1U };
> ?struct sbi_scratch *hartindex_to_scratch_table[SBI_HARTMASK_MAX_BITS + 1] = { 0 };
> @@ -70,8 +73,12 @@ unsigned long sbi_scratch_alloc_offset(unsigned long size)
> ? if (!size)
> ? return 0;
> ?
> - size += __SIZEOF_POINTER__ - 1;
> - size &= ~((unsigned long)__SIZEOF_POINTER__ - 1);
> + /*
> + * We let the allocation align to 64 bytes to avoid livelock on
> + * certain platforms due to atomic variables from the same cache line.
> + */
> + size += SCRATCH_ALLOC_ALIGN - 1;
> + size &= ~((unsigned long)SCRATCH_ALLOC_ALIGN - 1);
> ?
> ? spin_lock(&extra_lock);
> ?
> --
> 2.47.1
>
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v2] We add a new configuration CONFIG_SBI_SCRATCH_ALLOC_ALIGNMENT as an int
2025-01-10 0:34 [PATCH] Align the scratch allocation to 64 bytes Chao-ying Fu
2025-01-10 4:15 ` Xiang W
@ 2025-01-19 20:04 ` Raj Vishwanathan
2025-01-20 3:58 ` Xiang W
` (3 more replies)
1 sibling, 4 replies; 15+ messages in thread
From: Raj Vishwanathan @ 2025-01-19 20:04 UTC (permalink / raw)
To: opensbi
If it is set to 0 or not defined, we will continue with the previous
defintion of allocating pointer size chunks. Otherwise we will use the
chacheline size of 64.
We have the option of increasing the scratch allocation alignment to 64 bytes
as the cache line size, to avoid two atomic variables from
the same cache line that may cause livelock on some platforms.
---
lib/sbi/Kconfig | 8 ++++++++
lib/sbi/sbi_scratch.c | 18 ++++++++++++++++--
2 files changed, 24 insertions(+), 2 deletions(-)
diff --git a/lib/sbi/Kconfig b/lib/sbi/Kconfig
index c6cc04b..1ffa170 100644
--- a/lib/sbi/Kconfig
+++ b/lib/sbi/Kconfig
@@ -69,4 +69,12 @@ config SBI_ECALL_SSE
config SBI_ECALL_MPXY
bool "MPXY extension"
default y
+config SBI_SCRATCH_ALLOC_ALIGNMENT
+ int "Scratch allocation alignment"
+ default 0
+ help
+ We provide the option for allocation alignment to 64 bytes to avoid
+ livelock on certain platforms due to atomic variables from the same
+ cache line. Leave it 0 for default behaviour.
+
endmenu
diff --git a/lib/sbi/sbi_scratch.c b/lib/sbi/sbi_scratch.c
index ccbbc68..88ea3c7 100644
--- a/lib/sbi/sbi_scratch.c
+++ b/lib/sbi/sbi_scratch.c
@@ -14,6 +14,13 @@
#include <sbi/sbi_scratch.h>
#include <sbi/sbi_string.h>
+#if !defined(CONFIG_SBI_SCRATCH_ALLOC_ALIGNMENT) || (CONFIG_SBI_SCRATCH_ALLOC_ALIGNMENT==0)
+#define SCRATCH_ALLOC_ALIGNMENT __SIZEOF_POINTER__
+#else
+#define SCRATCH_ALLOC_ALIGNMENT CONFIG_SBI_SCRATCH_ALLOC_ALIGNMENT
+#endif
+
+
u32 last_hartindex_having_scratch = 0;
u32 hartindex_to_hartid_table[SBI_HARTMASK_MAX_BITS + 1] = { -1U };
struct sbi_scratch *hartindex_to_scratch_table[SBI_HARTMASK_MAX_BITS + 1] = { 0 };
@@ -70,8 +77,15 @@ unsigned long sbi_scratch_alloc_offset(unsigned long size)
if (!size)
return 0;
- size += __SIZEOF_POINTER__ - 1;
- size &= ~((unsigned long)__SIZEOF_POINTER__ - 1);
+ /*
+ * We let the allocation align to the cache line size, so
+ * certain platforms due to atomic variables from the same cache line.
+ * This ensures that the LR/SC variables are in a separate cache line
+ * to avoid live lock.
+ */
+
+ size += SCRATCH_ALLOC_ALIGNMENT- 1;
+ size &= ~((unsigned long)SCRATCH_ALLOC_ALIGNMENT - 1);
spin_lock(&extra_lock);
--
2.43.0
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v2] We add a new configuration CONFIG_SBI_SCRATCH_ALLOC_ALIGNMENT as an int
2025-01-19 20:04 ` [PATCH v2] We add a new configuration CONFIG_SBI_SCRATCH_ALLOC_ALIGNMENT as an int Raj Vishwanathan
@ 2025-01-20 3:58 ` Xiang W
2025-01-20 18:56 ` [EXTERNAL]Re: " Raj Vishwanathan
2025-01-21 1:46 ` [PATCH v3] New configuration CONFIG_SBI_SCRATCH_ALLOC_ALIGNMENT Raj Vishwanathan
` (2 subsequent siblings)
3 siblings, 1 reply; 15+ messages in thread
From: Xiang W @ 2025-01-20 3:58 UTC (permalink / raw)
To: opensbi
? 2025-01-19?? 12:04 -0800?Raj Vishwanathan???
> If it is set to 0 or not defined, we will continue with the previous
> defintion of allocating pointer size chunks. Otherwise we will use the
> chacheline size of 64.
> We have the option of increasing the scratch allocation alignment to 64 bytes
> as the cache line size, to avoid two atomic variables from
> the same cache line that may cause livelock on some platforms.
> ---
> ?lib/sbi/Kconfig?????? |? 8 ++++++++
> ?lib/sbi/sbi_scratch.c | 18 ++++++++++++++++--
> ?2 files changed, 24 insertions(+), 2 deletions(-)
>
> diff --git a/lib/sbi/Kconfig b/lib/sbi/Kconfig
> index c6cc04b..1ffa170 100644
> --- a/lib/sbi/Kconfig
> +++ b/lib/sbi/Kconfig
> @@ -69,4 +69,12 @@ config SBI_ECALL_SSE
> ?config SBI_ECALL_MPXY
> ? bool "MPXY extension"
> ? default y
> +config SBI_SCRATCH_ALLOC_ALIGNMENT
> + int? "Scratch allocation alignment"
> + default 0
> + help
> + ? We provide the option for allocation alignment to 64 bytes to avoid
> + ? livelock on certain platforms due to atomic variables from the same
> + ? cache line.? Leave it 0 for default behaviour.
Don't stress 64 bytes specifically
Customise the size of the alignment to allocate from the extra space in
sbi_scratch. Leave it 0 for default behaviour
Regards,
Xiang W
> +
> ?endmenu
> diff --git a/lib/sbi/sbi_scratch.c b/lib/sbi/sbi_scratch.c
> index ccbbc68..88ea3c7 100644
> --- a/lib/sbi/sbi_scratch.c
> +++ b/lib/sbi/sbi_scratch.c
> @@ -14,6 +14,13 @@
> ?#include <sbi/sbi_scratch.h>
> ?#include <sbi/sbi_string.h>
> ?
> +#if !defined(CONFIG_SBI_SCRATCH_ALLOC_ALIGNMENT) || (CONFIG_SBI_SCRATCH_ALLOC_ALIGNMENT==0)
> +#define SCRATCH_ALLOC_ALIGNMENT __SIZEOF_POINTER__
> +#else
> +#define SCRATCH_ALLOC_ALIGNMENT CONFIG_SBI_SCRATCH_ALLOC_ALIGNMENT
> +#endif
> +
> +
> ?u32 last_hartindex_having_scratch = 0;
> ?u32 hartindex_to_hartid_table[SBI_HARTMASK_MAX_BITS + 1] = { -1U };
> ?struct sbi_scratch *hartindex_to_scratch_table[SBI_HARTMASK_MAX_BITS + 1] = { 0 };
> @@ -70,8 +77,15 @@ unsigned long sbi_scratch_alloc_offset(unsigned long size)
> ? if (!size)
> ? return 0;
> ?
> - size += __SIZEOF_POINTER__ - 1;
> - size &= ~((unsigned long)__SIZEOF_POINTER__ - 1);
> + /*
> + * We let the allocation align to the cache line size, so
> + * certain platforms due to atomic variables from the same cache line.
> + * This ensures that the LR/SC variables are in a separate cache line
> + * to avoid live lock.
> + */
> +
> + size += SCRATCH_ALLOC_ALIGNMENT- 1;
> + size &= ~((unsigned long)SCRATCH_ALLOC_ALIGNMENT - 1);
> ?
> ? spin_lock(&extra_lock);
> ?
> --
> 2.43.0
>
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* [EXTERNAL]Re: [PATCH v2] We add a new configuration CONFIG_SBI_SCRATCH_ALLOC_ALIGNMENT as an int
2025-01-20 3:58 ` Xiang W
@ 2025-01-20 18:56 ` Raj Vishwanathan
0 siblings, 0 replies; 15+ messages in thread
From: Raj Vishwanathan @ 2025-01-20 18:56 UTC (permalink / raw)
To: opensbi
Xiang W.
Sounds good. Will update it the text in Kconfig. The reason for 64 bytes was to align it with the?cacheline size.
Raj
-----Original Message-----
From: Xiang W <wxjstz@126.com>
Sent: Sunday, January 19, 2025 7:58 PM
To: Raj Vishwanathan <raj.vishwanathan@gmail.com>; opensbi at lists.infradead.org
Cc: Raj Vishwanathan <rvishwanathan@mips.com>
Subject: [EXTERNAL]Re: [PATCH v2] We add a new configuration CONFIG_SBI_SCRATCH_ALLOC_ALIGNMENT as an int
[You don't often get email from wxjstz at 126.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
? 2025-01-19?? 12:04 -0800?Raj Vishwanathan???
> If it is set to 0 or not defined, we will continue with the previous
> defintion of allocating pointer size chunks. Otherwise we will use the
> chacheline size of 64.
> We have the option of increasing the scratch allocation alignment to
> 64 bytes as the cache line size, to avoid two atomic variables from
> the same cache line that may cause livelock on some platforms.
> ---
> lib/sbi/Kconfig |? 8 ++++++++
> lib/sbi/sbi_scratch.c | 18 ++++++++++++++++--
> 2 files changed, 24 insertions(+), 2 deletions(-)
>
> diff --git a/lib/sbi/Kconfig b/lib/sbi/Kconfig index c6cc04b..1ffa170
> 100644
> --- a/lib/sbi/Kconfig
> +++ b/lib/sbi/Kconfig
> @@ -69,4 +69,12 @@ config SBI_ECALL_SSE config SBI_ECALL_MPXY
> bool "MPXY extension"
> default y
> +config SBI_SCRATCH_ALLOC_ALIGNMENT
> + int "Scratch allocation alignment"
> + default 0
> + help
> + We provide the option for allocation alignment to 64 bytes to avoid
> + livelock on certain platforms due to atomic variables from the same
> + cache line. Leave it 0 for default behaviour.
Don't stress 64 bytes specifically
Customise the size of the alignment to allocate from the extra space in sbi_scratch. Leave it 0 for default behaviour
Regards,
Xiang W
> +
> endmenu
> diff --git a/lib/sbi/sbi_scratch.c b/lib/sbi/sbi_scratch.c index
> ccbbc68..88ea3c7 100644
> --- a/lib/sbi/sbi_scratch.c
> +++ b/lib/sbi/sbi_scratch.c
> @@ -14,6 +14,13 @@
> #include <sbi/sbi_scratch.h>
> #include <sbi/sbi_string.h>
>
> +#if !defined(CONFIG_SBI_SCRATCH_ALLOC_ALIGNMENT) ||
> +(CONFIG_SBI_SCRATCH_ALLOC_ALIGNMENT==0)
> +#define SCRATCH_ALLOC_ALIGNMENT __SIZEOF_POINTER__ #else #define
> +SCRATCH_ALLOC_ALIGNMENT CONFIG_SBI_SCRATCH_ALLOC_ALIGNMENT
> +#endif
> +
> +
> u32 last_hartindex_having_scratch = 0;
> u32 hartindex_to_hartid_table[SBI_HARTMASK_MAX_BITS + 1] = { -1U };
> struct sbi_scratch *hartindex_to_scratch_table[SBI_HARTMASK_MAX_BITS +
> 1] = { 0 }; @@ -70,8 +77,15 @@ unsigned long sbi_scratch_alloc_offset(unsigned long size)
> if (!size)
> return 0;
>
> - size += __SIZEOF_POINTER__ - 1;
> - size &= ~((unsigned long)__SIZEOF_POINTER__ - 1);
> + /*
> + * We let the allocation align to the cache line size, so
> + * certain platforms due to atomic variables from the same cache line.
> + * This ensures that the LR/SC variables are in a separate cache line
> + * to avoid live lock.
> + */
> +
> + size += SCRATCH_ALLOC_ALIGNMENT- 1;
> + size &= ~((unsigned long)SCRATCH_ALLOC_ALIGNMENT - 1);
>
> spin_lock(&extra_lock);
>
> --
> 2.43.0
>
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v3] New configuration CONFIG_SBI_SCRATCH_ALLOC_ALIGNMENT
2025-01-19 20:04 ` [PATCH v2] We add a new configuration CONFIG_SBI_SCRATCH_ALLOC_ALIGNMENT as an int Raj Vishwanathan
2025-01-20 3:58 ` Xiang W
@ 2025-01-21 1:46 ` Raj Vishwanathan
2025-01-21 3:21 ` Xiang W
2025-01-21 22:55 ` Raj Vishwanathan
2025-01-24 23:26 ` Raj Vishwanathan
3 siblings, 1 reply; 15+ messages in thread
From: Raj Vishwanathan @ 2025-01-21 1:46 UTC (permalink / raw)
To: opensbi
We add a new configuration CONFIG_SBI_SCRATCH_ALLOC_ALIGNMENT as an int
If it is set to 0 or not defined, we will continue with the previous
defintion of allocating pointer size chunks. Otherwise we will use the
chacheline size of 64.
We have the option of increasing the scratch allocation alignment to 64 bytes
as the cache line size, to avoid two atomic variables from
the same cache line that may cause livelock on some platforms.
Update: Agreeing with the reviewer's comment about not stressing 64 bytes.
---
lib/sbi/Kconfig | 7 +++++++
lib/sbi/sbi_scratch.c | 18 ++++++++++++++++--
2 files changed, 23 insertions(+), 2 deletions(-)
diff --git a/lib/sbi/Kconfig b/lib/sbi/Kconfig
index c6cc04b..5f7eb70 100644
--- a/lib/sbi/Kconfig
+++ b/lib/sbi/Kconfig
@@ -69,4 +69,11 @@ config SBI_ECALL_SSE
config SBI_ECALL_MPXY
bool "MPXY extension"
default y
+config SBI_SCRATCH_ALLOC_ALIGNMENT
+ int "Scratch allocation alignment"
+ default 0
+ help
+ We provide the option to customize the alignment to allocate from
+ the extra space in sbi_scratch. Leave it 0 for default behaviour.
+
endmenu
diff --git a/lib/sbi/sbi_scratch.c b/lib/sbi/sbi_scratch.c
index ccbbc68..88ea3c7 100644
--- a/lib/sbi/sbi_scratch.c
+++ b/lib/sbi/sbi_scratch.c
@@ -14,6 +14,13 @@
#include <sbi/sbi_scratch.h>
#include <sbi/sbi_string.h>
+#if !defined(CONFIG_SBI_SCRATCH_ALLOC_ALIGNMENT) || (CONFIG_SBI_SCRATCH_ALLOC_ALIGNMENT==0)
+#define SCRATCH_ALLOC_ALIGNMENT __SIZEOF_POINTER__
+#else
+#define SCRATCH_ALLOC_ALIGNMENT CONFIG_SBI_SCRATCH_ALLOC_ALIGNMENT
+#endif
+
+
u32 last_hartindex_having_scratch = 0;
u32 hartindex_to_hartid_table[SBI_HARTMASK_MAX_BITS + 1] = { -1U };
struct sbi_scratch *hartindex_to_scratch_table[SBI_HARTMASK_MAX_BITS + 1] = { 0 };
@@ -70,8 +77,15 @@ unsigned long sbi_scratch_alloc_offset(unsigned long size)
if (!size)
return 0;
- size += __SIZEOF_POINTER__ - 1;
- size &= ~((unsigned long)__SIZEOF_POINTER__ - 1);
+ /*
+ * We let the allocation align to the cache line size, so
+ * certain platforms due to atomic variables from the same cache line.
+ * This ensures that the LR/SC variables are in a separate cache line
+ * to avoid live lock.
+ */
+
+ size += SCRATCH_ALLOC_ALIGNMENT- 1;
+ size &= ~((unsigned long)SCRATCH_ALLOC_ALIGNMENT - 1);
spin_lock(&extra_lock);
--
2.43.0
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v3] New configuration CONFIG_SBI_SCRATCH_ALLOC_ALIGNMENT
2025-01-21 1:46 ` [PATCH v3] New configuration CONFIG_SBI_SCRATCH_ALLOC_ALIGNMENT Raj Vishwanathan
@ 2025-01-21 3:21 ` Xiang W
0 siblings, 0 replies; 15+ messages in thread
From: Xiang W @ 2025-01-21 3:21 UTC (permalink / raw)
To: opensbi
? 2025-01-20?? 17:46 -0800?Raj Vishwanathan???
> We add a new configuration CONFIG_SBI_SCRATCH_ALLOC_ALIGNMENT as an int
> If it is set to 0 or not defined, we will continue with the previous
> defintion of allocating pointer size chunks. Otherwise we will use the
> chacheline size of 64.
> We have the option of increasing the scratch allocation alignment to 64 bytes
> as the cache line size, to avoid two atomic variables from
> the same cache line that may cause livelock on some platforms.
>
missing your Signed-off-by
> Update: Agreeing with the reviewer's comment about not stressing 64 bytes.
The previous line need move after '---'
> ---
> ?lib/sbi/Kconfig?????? |? 7 +++++++
> ?lib/sbi/sbi_scratch.c | 18 ++++++++++++++++--
> ?2 files changed, 23 insertions(+), 2 deletions(-)
>
> diff --git a/lib/sbi/Kconfig b/lib/sbi/Kconfig
> index c6cc04b..5f7eb70 100644
> --- a/lib/sbi/Kconfig
> +++ b/lib/sbi/Kconfig
> @@ -69,4 +69,11 @@ config SBI_ECALL_SSE
> ?config SBI_ECALL_MPXY
> ? bool "MPXY extension"
> ? default y
> +config SBI_SCRATCH_ALLOC_ALIGNMENT
> + int? "Scratch allocation alignment"
> + default 0
> + help
> + ? We provide the option to customize the alignment to allocate from
> + ? the extra space in sbi_scratch. Leave it 0 for default behaviour.
> +
> ?endmenu
> diff --git a/lib/sbi/sbi_scratch.c b/lib/sbi/sbi_scratch.c
> index ccbbc68..88ea3c7 100644
> --- a/lib/sbi/sbi_scratch.c
> +++ b/lib/sbi/sbi_scratch.c
> @@ -14,6 +14,13 @@
> ?#include <sbi/sbi_scratch.h>
> ?#include <sbi/sbi_string.h>
> ?
> +#if !defined(CONFIG_SBI_SCRATCH_ALLOC_ALIGNMENT) || (CONFIG_SBI_SCRATCH_ALLOC_ALIGNMENT==0)
> +#define SCRATCH_ALLOC_ALIGNMENT __SIZEOF_POINTER__
> +#else
> +#define SCRATCH_ALLOC_ALIGNMENT CONFIG_SBI_SCRATCH_ALLOC_ALIGNMENT
> +#endif
> +
> +
> ?u32 last_hartindex_having_scratch = 0;
> ?u32 hartindex_to_hartid_table[SBI_HARTMASK_MAX_BITS + 1] = { -1U };
> ?struct sbi_scratch *hartindex_to_scratch_table[SBI_HARTMASK_MAX_BITS + 1] = { 0 };
> @@ -70,8 +77,15 @@ unsigned long sbi_scratch_alloc_offset(unsigned long size)
> ? if (!size)
> ? return 0;
> ?
> - size += __SIZEOF_POINTER__ - 1;
> - size &= ~((unsigned long)__SIZEOF_POINTER__ - 1);
> + /*
> + * We let the allocation align to the cache line size, so
> + * certain platforms due to atomic variables from the same cache line.
> + * This ensures that the LR/SC variables are in a separate cache line
> + * to avoid live lock.
> + */
These comments are platform-specific. Better change it.
Regards,
Xiang W
> +
> + size += SCRATCH_ALLOC_ALIGNMENT- 1;
> + size &= ~((unsigned long)SCRATCH_ALLOC_ALIGNMENT - 1);
> ?
> ? spin_lock(&extra_lock);
> ?
> --
> 2.43.0
>
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v3] New configuration CONFIG_SBI_SCRATCH_ALLOC_ALIGNMENT
2025-01-19 20:04 ` [PATCH v2] We add a new configuration CONFIG_SBI_SCRATCH_ALLOC_ALIGNMENT as an int Raj Vishwanathan
2025-01-20 3:58 ` Xiang W
2025-01-21 1:46 ` [PATCH v3] New configuration CONFIG_SBI_SCRATCH_ALLOC_ALIGNMENT Raj Vishwanathan
@ 2025-01-21 22:55 ` Raj Vishwanathan
2025-01-24 23:26 ` Raj Vishwanathan
3 siblings, 0 replies; 15+ messages in thread
From: Raj Vishwanathan @ 2025-01-21 22:55 UTC (permalink / raw)
To: opensbi
We add a new configuration CONFIG_SBI_SCRATCH_ALLOC_ALIGNMENT as an int
If it is set to 0 or not defined, we will continue with the previous
defintion of allocating pointer size chunks. Otherwise we will use the
chacheline size of 64.
We have the option of increasing the scratch allocation alignment to 64 bytes
as the cache line size, to avoid two atomic variables from
the same cache line that may cause livelock on some platforms.
---
Update: Agreeing with the reviewer's comment about not stressing 64 bytes.
Signed-off-by: Raj Vishwanathan <Raj.Vishwanathan@gmail.com>
---
lib/sbi/Kconfig | 7 +++++++
lib/sbi/sbi_scratch.c | 18 ++++++++++++++++--
2 files changed, 23 insertions(+), 2 deletions(-)
diff --git a/lib/sbi/Kconfig b/lib/sbi/Kconfig
index c6cc04b..5f7eb70 100644
--- a/lib/sbi/Kconfig
+++ b/lib/sbi/Kconfig
@@ -69,4 +69,11 @@ config SBI_ECALL_SSE
config SBI_ECALL_MPXY
bool "MPXY extension"
default y
+config SBI_SCRATCH_ALLOC_ALIGNMENT
+ int "Scratch allocation alignment"
+ default 0
+ help
+ We provide the option to customize the alignment to allocate from
+ the extra space in sbi_scratch. Leave it 0 for default behaviour.
+
endmenu
diff --git a/lib/sbi/sbi_scratch.c b/lib/sbi/sbi_scratch.c
index ccbbc68..3fd7b4b 100644
--- a/lib/sbi/sbi_scratch.c
+++ b/lib/sbi/sbi_scratch.c
@@ -14,6 +14,13 @@
#include <sbi/sbi_scratch.h>
#include <sbi/sbi_string.h>
+#if !defined(CONFIG_SBI_SCRATCH_ALLOC_ALIGNMENT) || (CONFIG_SBI_SCRATCH_ALLOC_ALIGNMENT==0)
+#define SCRATCH_ALLOC_ALIGNMENT __SIZEOF_POINTER__
+#else
+#define SCRATCH_ALLOC_ALIGNMENT CONFIG_SBI_SCRATCH_ALLOC_ALIGNMENT
+#endif
+
+
u32 last_hartindex_having_scratch = 0;
u32 hartindex_to_hartid_table[SBI_HARTMASK_MAX_BITS + 1] = { -1U };
struct sbi_scratch *hartindex_to_scratch_table[SBI_HARTMASK_MAX_BITS + 1] = { 0 };
@@ -70,8 +77,15 @@ unsigned long sbi_scratch_alloc_offset(unsigned long size)
if (!size)
return 0;
- size += __SIZEOF_POINTER__ - 1;
- size &= ~((unsigned long)__SIZEOF_POINTER__ - 1);
+ /*
+ * We do the scratch allocation on the basis of a user configuration.
+ * We have the option of increasing the scratch allocation alignment
+ * to the cache line size (64 bytes) to avoid two atomic variables
+ * from the same cache line that may cause livelock on some platforms.
+ */
+
+ size += SCRATCH_ALLOC_ALIGNMENT- 1;
+ size &= ~((unsigned long)SCRATCH_ALLOC_ALIGNMENT - 1);
spin_lock(&extra_lock);
--
2.43.0
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v3] New configuration CONFIG_SBI_SCRATCH_ALLOC_ALIGNMENT
2025-01-19 20:04 ` [PATCH v2] We add a new configuration CONFIG_SBI_SCRATCH_ALLOC_ALIGNMENT as an int Raj Vishwanathan
` (2 preceding siblings ...)
2025-01-21 22:55 ` Raj Vishwanathan
@ 2025-01-24 23:26 ` Raj Vishwanathan
3 siblings, 0 replies; 15+ messages in thread
From: Raj Vishwanathan @ 2025-01-24 23:26 UTC (permalink / raw)
To: opensbi
We add a new configuration CONFIG_SBI_SCRATCH_ALLOC_ALIGNMENT as an int
If it is set to 0 or not defined, we will continue with the previous
defintion of allocating pointer size chunks. Otherwise we will use the
user specified size.
We use the scratch allocation alignment to 64 bytes or cacheline size,
to avoid two atomic variables from the same cache line causing livelock
on some platforms.
Changes in V2:
Remove platform specific references to 64 bytes in the configuration
Changes in V3:
Remove platform specific references in 64 bytes in the comments
Signed-off-by: Raj Vishwanathan <Raj.Vishwanathan@gmail.com>
---
lib/sbi/Kconfig | 7 +++++++
lib/sbi/sbi_scratch.c | 14 ++++++++++++--
2 files changed, 19 insertions(+), 2 deletions(-)
diff --git a/lib/sbi/Kconfig b/lib/sbi/Kconfig
index c6cc04b..5f7eb70 100644
--- a/lib/sbi/Kconfig
+++ b/lib/sbi/Kconfig
@@ -69,4 +69,11 @@ config SBI_ECALL_SSE
config SBI_ECALL_MPXY
bool "MPXY extension"
default y
+config SBI_SCRATCH_ALLOC_ALIGNMENT
+ int "Scratch allocation alignment"
+ default 0
+ help
+ We provide the option to customize the alignment to allocate from
+ the extra space in sbi_scratch. Leave it 0 for default behaviour.
+
endmenu
diff --git a/lib/sbi/sbi_scratch.c b/lib/sbi/sbi_scratch.c
index ccbbc68..0f1be58 100644
--- a/lib/sbi/sbi_scratch.c
+++ b/lib/sbi/sbi_scratch.c
@@ -14,6 +14,16 @@
#include <sbi/sbi_scratch.h>
#include <sbi/sbi_string.h>
+/*
+ * We do the scratch allocation on the basis of a user configuration.
+ */
+#if !defined(CONFIG_SBI_SCRATCH_ALLOC_ALIGNMENT) || (CONFIG_SBI_SCRATCH_ALLOC_ALIGNMENT==0)
+#define SCRATCH_ALLOC_ALIGNMENT __SIZEOF_POINTER__
+#else
+#define SCRATCH_ALLOC_ALIGNMENT CONFIG_SBI_SCRATCH_ALLOC_ALIGNMENT
+#endif
+
+
u32 last_hartindex_having_scratch = 0;
u32 hartindex_to_hartid_table[SBI_HARTMASK_MAX_BITS + 1] = { -1U };
struct sbi_scratch *hartindex_to_scratch_table[SBI_HARTMASK_MAX_BITS + 1] = { 0 };
@@ -70,8 +80,8 @@ unsigned long sbi_scratch_alloc_offset(unsigned long size)
if (!size)
return 0;
- size += __SIZEOF_POINTER__ - 1;
- size &= ~((unsigned long)__SIZEOF_POINTER__ - 1);
+ size += SCRATCH_ALLOC_ALIGNMENT- 1;
+ size &= ~((unsigned long)SCRATCH_ALLOC_ALIGNMENT - 1);
spin_lock(&extra_lock);
--
2.43.0
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v3] New configuration CONFIG_SBI_SCRATCH_ALLOC_ALIGNMENT
2025-01-15 23:46 [PATCH v2] Work with hartid equal to or greater than SBI_HARTMASK_MAX_BITS Raj Vishwanathan
@ 2025-01-27 20:20 ` Raj Vishwanathan
2025-01-28 4:36 ` Xiang W
2025-02-11 4:45 ` Anup Patel
0 siblings, 2 replies; 15+ messages in thread
From: Raj Vishwanathan @ 2025-01-27 20:20 UTC (permalink / raw)
To: opensbi
We add a new configuration CONFIG_SBI_SCRATCH_ALLOC_ALIGNMENT as an int
If it is set to 0 or not defined, we will continue with the previous
defintion of allocating pointer size chunks. Otherwise we will use the
user specified size.
We use the scratch allocation alignment to 64 bytes or cacheline size,
to avoid two atomic variables from the same cache line causing livelock
on some platforms.
Signed-off-by: Raj Vishwanathan <Raj.Vishwanathan@gmail.com>
---
Changes in V2:
Remove platform specific references to 64 bytes in the configuration
Changes in V3:
Remove platform specific references in 64 bytes in the comments
---
lib/sbi/Kconfig | 7 +++++++
lib/sbi/sbi_scratch.c | 14 ++++++++++++--
2 files changed, 19 insertions(+), 2 deletions(-)
diff --git a/lib/sbi/Kconfig b/lib/sbi/Kconfig
index c6cc04b..5f7eb70 100644
--- a/lib/sbi/Kconfig
+++ b/lib/sbi/Kconfig
@@ -69,4 +69,11 @@ config SBI_ECALL_SSE
config SBI_ECALL_MPXY
bool "MPXY extension"
default y
+config SBI_SCRATCH_ALLOC_ALIGNMENT
+ int "Scratch allocation alignment"
+ default 0
+ help
+ We provide the option to customize the alignment to allocate from
+ the extra space in sbi_scratch. Leave it 0 for default behaviour.
+
endmenu
diff --git a/lib/sbi/sbi_scratch.c b/lib/sbi/sbi_scratch.c
index ccbbc68..0f1be58 100644
--- a/lib/sbi/sbi_scratch.c
+++ b/lib/sbi/sbi_scratch.c
@@ -14,6 +14,16 @@
#include <sbi/sbi_scratch.h>
#include <sbi/sbi_string.h>
+/*
+ * We do the scratch allocation on the basis of a user configuration.
+ */
+#if !defined(CONFIG_SBI_SCRATCH_ALLOC_ALIGNMENT) || (CONFIG_SBI_SCRATCH_ALLOC_ALIGNMENT==0)
+#define SCRATCH_ALLOC_ALIGNMENT __SIZEOF_POINTER__
+#else
+#define SCRATCH_ALLOC_ALIGNMENT CONFIG_SBI_SCRATCH_ALLOC_ALIGNMENT
+#endif
+
+
u32 last_hartindex_having_scratch = 0;
u32 hartindex_to_hartid_table[SBI_HARTMASK_MAX_BITS + 1] = { -1U };
struct sbi_scratch *hartindex_to_scratch_table[SBI_HARTMASK_MAX_BITS + 1] = { 0 };
@@ -70,8 +80,8 @@ unsigned long sbi_scratch_alloc_offset(unsigned long size)
if (!size)
return 0;
- size += __SIZEOF_POINTER__ - 1;
- size &= ~((unsigned long)__SIZEOF_POINTER__ - 1);
+ size += SCRATCH_ALLOC_ALIGNMENT- 1;
+ size &= ~((unsigned long)SCRATCH_ALLOC_ALIGNMENT - 1);
spin_lock(&extra_lock);
--
2.43.0
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v3] New configuration CONFIG_SBI_SCRATCH_ALLOC_ALIGNMENT
2025-01-27 20:20 ` [PATCH v3] New configuration CONFIG_SBI_SCRATCH_ALLOC_ALIGNMENT Raj Vishwanathan
@ 2025-01-28 4:36 ` Xiang W
2025-02-11 4:45 ` Anup Patel
1 sibling, 0 replies; 15+ messages in thread
From: Xiang W @ 2025-01-28 4:36 UTC (permalink / raw)
To: opensbi
? 2025-01-27?? 12:20 -0800?Raj Vishwanathan???
> We add a new configuration CONFIG_SBI_SCRATCH_ALLOC_ALIGNMENT as an int
> If it is set to 0 or not defined, we will continue with the previous
> defintion of allocating pointer size chunks. Otherwise we will use the
> user specified size.
> We use the scratch allocation alignment to 64 bytes or cacheline size,
> to avoid two atomic variables from the same cache line causing livelock
> on some platforms.
>
> Signed-off-by: Raj Vishwanathan <Raj.Vishwanathan@gmail.com>
LGMT
Reviewed-by: Xiang W <wxjstz@126.com>
> ---
>
> Changes in V2:
> Remove platform specific references to 64 bytes in the configuration
>
> Changes in V3:
> Remove platform specific references in 64 bytes in the comments
> ---
> ?lib/sbi/Kconfig?????? |? 7 +++++++
> ?lib/sbi/sbi_scratch.c | 14 ++++++++++++--
> ?2 files changed, 19 insertions(+), 2 deletions(-)
>
> diff --git a/lib/sbi/Kconfig b/lib/sbi/Kconfig
> index c6cc04b..5f7eb70 100644
> --- a/lib/sbi/Kconfig
> +++ b/lib/sbi/Kconfig
> @@ -69,4 +69,11 @@ config SBI_ECALL_SSE
> ?config SBI_ECALL_MPXY
> ? bool "MPXY extension"
> ? default y
> +config SBI_SCRATCH_ALLOC_ALIGNMENT
> + int? "Scratch allocation alignment"
> + default 0
> + help
> + ? We provide the option to customize the alignment to allocate from
> + ? the extra space in sbi_scratch. Leave it 0 for default behaviour.
> +
> ?endmenu
> diff --git a/lib/sbi/sbi_scratch.c b/lib/sbi/sbi_scratch.c
> index ccbbc68..0f1be58 100644
> --- a/lib/sbi/sbi_scratch.c
> +++ b/lib/sbi/sbi_scratch.c
> @@ -14,6 +14,16 @@
> ?#include <sbi/sbi_scratch.h>
> ?#include <sbi/sbi_string.h>
> ?
> +/*
> + * We do the scratch allocation on the basis of a user configuration.
> + */
> +#if !defined(CONFIG_SBI_SCRATCH_ALLOC_ALIGNMENT) || (CONFIG_SBI_SCRATCH_ALLOC_ALIGNMENT==0)
> +#define SCRATCH_ALLOC_ALIGNMENT __SIZEOF_POINTER__
> +#else
> +#define SCRATCH_ALLOC_ALIGNMENT CONFIG_SBI_SCRATCH_ALLOC_ALIGNMENT
> +#endif
> +
> +
> ?u32 last_hartindex_having_scratch = 0;
> ?u32 hartindex_to_hartid_table[SBI_HARTMASK_MAX_BITS + 1] = { -1U };
> ?struct sbi_scratch *hartindex_to_scratch_table[SBI_HARTMASK_MAX_BITS + 1] = { 0 };
> @@ -70,8 +80,8 @@ unsigned long sbi_scratch_alloc_offset(unsigned long size)
> ? if (!size)
> ? return 0;
> ?
> - size += __SIZEOF_POINTER__ - 1;
> - size &= ~((unsigned long)__SIZEOF_POINTER__ - 1);
> + size += SCRATCH_ALLOC_ALIGNMENT- 1;
> + size &= ~((unsigned long)SCRATCH_ALLOC_ALIGNMENT - 1);
> ?
> ? spin_lock(&extra_lock);
> ?
> --
> 2.43.0
>
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v3] New configuration CONFIG_SBI_SCRATCH_ALLOC_ALIGNMENT
2025-01-27 20:20 ` [PATCH v3] New configuration CONFIG_SBI_SCRATCH_ALLOC_ALIGNMENT Raj Vishwanathan
2025-01-28 4:36 ` Xiang W
@ 2025-02-11 4:45 ` Anup Patel
2025-03-05 0:31 ` Raj Vishwanathan
1 sibling, 1 reply; 15+ messages in thread
From: Anup Patel @ 2025-02-11 4:45 UTC (permalink / raw)
To: opensbi
On Tue, Jan 28, 2025 at 1:50?AM Raj Vishwanathan
<raj.vishwanathan@gmail.com> wrote:
>
> We add a new configuration CONFIG_SBI_SCRATCH_ALLOC_ALIGNMENT as an int
> If it is set to 0 or not defined, we will continue with the previous
> defintion of allocating pointer size chunks. Otherwise we will use the
> user specified size.
> We use the scratch allocation alignment to 64 bytes or cacheline size,
> to avoid two atomic variables from the same cache line causing livelock
> on some platforms.
>
> Signed-off-by: Raj Vishwanathan <Raj.Vishwanathan@gmail.com>
> ---
>
> Changes in V2:
> Remove platform specific references to 64 bytes in the configuration
>
> Changes in V3:
> Remove platform specific references in 64 bytes in the comments
> ---
> lib/sbi/Kconfig | 7 +++++++
> lib/sbi/sbi_scratch.c | 14 ++++++++++++--
> 2 files changed, 19 insertions(+), 2 deletions(-)
>
> diff --git a/lib/sbi/Kconfig b/lib/sbi/Kconfig
> index c6cc04b..5f7eb70 100644
> --- a/lib/sbi/Kconfig
> +++ b/lib/sbi/Kconfig
> @@ -69,4 +69,11 @@ config SBI_ECALL_SSE
> config SBI_ECALL_MPXY
> bool "MPXY extension"
> default y
> +config SBI_SCRATCH_ALLOC_ALIGNMENT
> + int "Scratch allocation alignment"
> + default 0
> + help
> + We provide the option to customize the alignment to allocate from
> + the extra space in sbi_scratch. Leave it 0 for default behaviour.
> +
Introducing a kconfig option is not going to fly since we have to
re-compile separately for platforms while we are trying our best
to support the same firmware binary for multiple platforms.
Instead, I suggest the following:
1) The default scratch allocation alignment can be __SIZEOF_POINTER__
2) Introduce get_cache_line_size() operation in "struct sbi_platform_operations"
which platforms can use to return desired "cache line size for allocations".
3) For generic platform, the get_cache_line_size() return value can be based
on the "riscv,cbom-block-size" property of CPU DT nodes OR based on
platform_overrride.
4) The sbi_scratch_init() should sanity check the value returned by platform
get_cache_line_size() before using it as minimum allocation size for
scratch allocations.
Regards,
Anup
> endmenu
> diff --git a/lib/sbi/sbi_scratch.c b/lib/sbi/sbi_scratch.c
> index ccbbc68..0f1be58 100644
> --- a/lib/sbi/sbi_scratch.c
> +++ b/lib/sbi/sbi_scratch.c
> @@ -14,6 +14,16 @@
> #include <sbi/sbi_scratch.h>
> #include <sbi/sbi_string.h>
>
> +/*
> + * We do the scratch allocation on the basis of a user configuration.
> + */
> +#if !defined(CONFIG_SBI_SCRATCH_ALLOC_ALIGNMENT) || (CONFIG_SBI_SCRATCH_ALLOC_ALIGNMENT==0)
> +#define SCRATCH_ALLOC_ALIGNMENT __SIZEOF_POINTER__
> +#else
> +#define SCRATCH_ALLOC_ALIGNMENT CONFIG_SBI_SCRATCH_ALLOC_ALIGNMENT
> +#endif
> +
> +
> u32 last_hartindex_having_scratch = 0;
> u32 hartindex_to_hartid_table[SBI_HARTMASK_MAX_BITS + 1] = { -1U };
> struct sbi_scratch *hartindex_to_scratch_table[SBI_HARTMASK_MAX_BITS + 1] = { 0 };
> @@ -70,8 +80,8 @@ unsigned long sbi_scratch_alloc_offset(unsigned long size)
> if (!size)
> return 0;
>
> - size += __SIZEOF_POINTER__ - 1;
> - size &= ~((unsigned long)__SIZEOF_POINTER__ - 1);
> + size += SCRATCH_ALLOC_ALIGNMENT- 1;
> + size &= ~((unsigned long)SCRATCH_ALLOC_ALIGNMENT - 1);
>
> spin_lock(&extra_lock);
>
> --
> 2.43.0
>
>
> --
> opensbi mailing list
> opensbi at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/opensbi
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v3] New configuration CONFIG_SBI_SCRATCH_ALLOC_ALIGNMENT
2025-02-11 4:45 ` Anup Patel
@ 2025-03-05 0:31 ` Raj Vishwanathan
2025-03-05 2:01 ` Samuel Holland
0 siblings, 1 reply; 15+ messages in thread
From: Raj Vishwanathan @ 2025-03-05 0:31 UTC (permalink / raw)
To: opensbi
Hi Anup
I have been reviewing your suggestions, and I am a bit confused by them.
---------------------- Snipped to retain only the relevant bits
--------------------------
The scratch allocation happens too early in the boot to get the
platform override return values (from parsing the device tree.)
The other issue is that riscv,cbom-block-size is a per-hart value and
testing all kinds of combinations may become unnecessarily
complicated.
If the general configuration value is not acceptable, I can move the
configuration to a platform specific configuration.
Let me know your thoughts
On Mon, Feb 10, 2025 at 8:45?PM Anup Patel <apatel@ventanamicro.com> wrote:
> Introducing a kconfig option is not going to fly since we have to
> re-compile separately for platforms while we are trying our best
> to support the same firmware binary for multiple platforms.
>
> Instead, I suggest the following:
> 1) The default scratch allocation alignment can be __SIZEOF_POINTER__
> 2) Introduce get_cache_line_size() operation in "struct sbi_platform_operations"
> which platforms can use to return desired "cache line size for allocations".
> 3) For generic platform, the get_cache_line_size() return value can be based
> on the "riscv,cbom-block-size" property of CPU DT nodes OR based on
> platform_overrride.
> 4) The sbi_scratch_init() should sanity check the value returned by platform
> get_cache_line_size() before using it as minimum allocation size for
> scratch allocations.
>
> Regards,
> Anup
>
>
> > endmenu
> > diff --git a/lib/sbi/sbi_scratch.c b/lib/sbi/sbi_scratch.c
> > index ccbbc68..0f1be58 100644
> > --- a/lib/sbi/sbi_scratch.c
> > +++ b/lib/sbi/sbi_scratch.c
> > @@ -14,6 +14,16 @@
> > #include <sbi/sbi_scratch.h>
> > #include <sbi/sbi_string.h>
> >
> > +/*
> > + * We do the scratch allocation on the basis of a user configuration.
> > + */
> > +#if !defined(CONFIG_SBI_SCRATCH_ALLOC_ALIGNMENT) || (CONFIG_SBI_SCRATCH_ALLOC_ALIGNMENT==0)
> > +#define SCRATCH_ALLOC_ALIGNMENT __SIZEOF_POINTER__
> > +#else
> > +#define SCRATCH_ALLOC_ALIGNMENT CONFIG_SBI_SCRATCH_ALLOC_ALIGNMENT
> > +#endif
> > +
> > +
> > u32 last_hartindex_having_scratch = 0;
> > u32 hartindex_to_hartid_table[SBI_HARTMASK_MAX_BITS + 1] = { -1U };
> > struct sbi_scratch *hartindex_to_scratch_table[SBI_HARTMASK_MAX_BITS + 1] = { 0 };
> > @@ -70,8 +80,8 @@ unsigned long sbi_scratch_alloc_offset(unsigned long size)
> > if (!size)
> > return 0;
> >
> > - size += __SIZEOF_POINTER__ - 1;
> > - size &= ~((unsigned long)__SIZEOF_POINTER__ - 1);
> > + size += SCRATCH_ALLOC_ALIGNMENT- 1;
> > + size &= ~((unsigned long)SCRATCH_ALLOC_ALIGNMENT - 1);
> >
> > spin_lock(&extra_lock);
> >
> > --
> > 2.43.0
> >
> >
> > --
> > opensbi mailing list
> > opensbi at lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/opensbi
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v3] New configuration CONFIG_SBI_SCRATCH_ALLOC_ALIGNMENT
2025-03-05 0:31 ` Raj Vishwanathan
@ 2025-03-05 2:01 ` Samuel Holland
0 siblings, 0 replies; 15+ messages in thread
From: Samuel Holland @ 2025-03-05 2:01 UTC (permalink / raw)
To: opensbi
Hi Raj,
On 2025-03-04 6:31 PM, Raj Vishwanathan wrote:
> Hi Anup
>
> I have been reviewing your suggestions, and I am a bit confused by them.
>
> ---------------------- Snipped to retain only the relevant bits
> --------------------------
> The scratch allocation happens too early in the boot to get the
> platform override return values (from parsing the device tree.)
Overrides for the generic platform happen in fw_platform_init(), which is called
before scratch space is set up. This function also already has logic to parse
properties of harts from the devicetree, so it is a good place to put your new
override logic.
> The other issue is that riscv,cbom-block-size is a per-hart value and
> testing all kinds of combinations may become unnecessarily
> complicated.
If the goal is to have each allocation in a separate cache line, then you always
want the maximum value across all harts, right?
> If the general configuration value is not acceptable, I can move the
> configuration to a platform specific configuration.
As Anup said, we want to avoid build-time configuration as much as possible, so
a single OpenSBI binary can run on the widest variety of platforms. So a runtime
override is the best way to go, even if it requires some refactoring.
Regards,
Samuel
> Let me know your thoughts
>
>
> On Mon, Feb 10, 2025 at 8:45?PM Anup Patel <apatel@ventanamicro.com> wrote:
>
>> Introducing a kconfig option is not going to fly since we have to
>> re-compile separately for platforms while we are trying our best
>> to support the same firmware binary for multiple platforms.
>>
>> Instead, I suggest the following:
>> 1) The default scratch allocation alignment can be __SIZEOF_POINTER__
>> 2) Introduce get_cache_line_size() operation in "struct sbi_platform_operations"
>> which platforms can use to return desired "cache line size for allocations".
>> 3) For generic platform, the get_cache_line_size() return value can be based
>> on the "riscv,cbom-block-size" property of CPU DT nodes OR based on
>> platform_overrride.
>> 4) The sbi_scratch_init() should sanity check the value returned by platform
>> get_cache_line_size() before using it as minimum allocation size for
>> scratch allocations.
>>
>> Regards,
>> Anup
>>
>>
>>> endmenu
>>> diff --git a/lib/sbi/sbi_scratch.c b/lib/sbi/sbi_scratch.c
>>> index ccbbc68..0f1be58 100644
>>> --- a/lib/sbi/sbi_scratch.c
>>> +++ b/lib/sbi/sbi_scratch.c
>>> @@ -14,6 +14,16 @@
>>> #include <sbi/sbi_scratch.h>
>>> #include <sbi/sbi_string.h>
>>>
>>> +/*
>>> + * We do the scratch allocation on the basis of a user configuration.
>>> + */
>>> +#if !defined(CONFIG_SBI_SCRATCH_ALLOC_ALIGNMENT) || (CONFIG_SBI_SCRATCH_ALLOC_ALIGNMENT==0)
>>> +#define SCRATCH_ALLOC_ALIGNMENT __SIZEOF_POINTER__
>>> +#else
>>> +#define SCRATCH_ALLOC_ALIGNMENT CONFIG_SBI_SCRATCH_ALLOC_ALIGNMENT
>>> +#endif
>>> +
>>> +
>>> u32 last_hartindex_having_scratch = 0;
>>> u32 hartindex_to_hartid_table[SBI_HARTMASK_MAX_BITS + 1] = { -1U };
>>> struct sbi_scratch *hartindex_to_scratch_table[SBI_HARTMASK_MAX_BITS + 1] = { 0 };
>>> @@ -70,8 +80,8 @@ unsigned long sbi_scratch_alloc_offset(unsigned long size)
>>> if (!size)
>>> return 0;
>>>
>>> - size += __SIZEOF_POINTER__ - 1;
>>> - size &= ~((unsigned long)__SIZEOF_POINTER__ - 1);
>>> + size += SCRATCH_ALLOC_ALIGNMENT- 1;
>>> + size &= ~((unsigned long)SCRATCH_ALLOC_ALIGNMENT - 1);
>>>
>>> spin_lock(&extra_lock);
>>>
>>> --
>>> 2.43.0
>>>
>>>
>>> --
>>> opensbi mailing list
>>> opensbi at lists.infradead.org
>>> http://lists.infradead.org/mailman/listinfo/opensbi
>
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2025-03-05 2:01 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-01-10 0:34 [PATCH] Align the scratch allocation to 64 bytes Chao-ying Fu
2025-01-10 4:15 ` Xiang W
2025-01-10 2:36 ` Chao-ying Fu
2025-01-19 20:04 ` [PATCH v2] We add a new configuration CONFIG_SBI_SCRATCH_ALLOC_ALIGNMENT as an int Raj Vishwanathan
2025-01-20 3:58 ` Xiang W
2025-01-20 18:56 ` [EXTERNAL]Re: " Raj Vishwanathan
2025-01-21 1:46 ` [PATCH v3] New configuration CONFIG_SBI_SCRATCH_ALLOC_ALIGNMENT Raj Vishwanathan
2025-01-21 3:21 ` Xiang W
2025-01-21 22:55 ` Raj Vishwanathan
2025-01-24 23:26 ` Raj Vishwanathan
-- strict thread matches above, loose matches on Subject: below --
2025-01-15 23:46 [PATCH v2] Work with hartid equal to or greater than SBI_HARTMASK_MAX_BITS Raj Vishwanathan
2025-01-27 20:20 ` [PATCH v3] New configuration CONFIG_SBI_SCRATCH_ALLOC_ALIGNMENT Raj Vishwanathan
2025-01-28 4:36 ` Xiang W
2025-02-11 4:45 ` Anup Patel
2025-03-05 0:31 ` Raj Vishwanathan
2025-03-05 2:01 ` Samuel Holland
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox