[PATCH] Work with hartid equal to or greater than SBI_HARTMASK_MAX_BITS.

[PATCH] Work with hartid equal to or greater than SBI_HARTMASK_MAX_BITS.

[Chao-ying Fu]

Some platforms use hartid that is equal to greater than SBI_HARTMASK_MAX_BITS,
so we should remove the check.
---
 lib/utils/fdt/fdt_domain.c  | 3 ---
 lib/utils/fdt/fdt_helper.c  | 9 ---------
 platform/generic/platform.c | 3 ---
 3 files changed, 15 deletions(-)

diff --git a/lib/utils/fdt/fdt_domain.c b/lib/utils/fdt/fdt_domain.c
index 4bc7ed8..6bc63c2 100644
--- a/lib/utils/fdt/fdt_domain.c
+++ b/lib/utils/fdt/fdt_domain.c
@@ -471,9 +471,6 @@ static int __fdt_parse_domain(const void *fdt, int domain_offset, void *opaque)
 		if (err)
 			continue;
 
-		if (SBI_HARTMASK_MAX_BITS <= val32)
-			continue;
-
 		if (!fdt_node_is_enabled(fdt, cpu_offset))
 			continue;
 
diff --git a/lib/utils/fdt/fdt_helper.c b/lib/utils/fdt/fdt_helper.c
index cb350e5..aec08fa 100644
--- a/lib/utils/fdt/fdt_helper.c
+++ b/lib/utils/fdt/fdt_helper.c
@@ -1032,9 +1032,6 @@ int fdt_parse_aclint_node(const void *fdt, int nodeoffset,
 		if (rc)
 			continue;
 
-		if (SBI_HARTMASK_MAX_BITS <= hartid)
-			continue;
-
 		if (match_hwirq == hwirq) {
 			if (hartid < first_hartid)
 				first_hartid = hartid;
@@ -1097,9 +1094,6 @@ int fdt_parse_plmt_node(const void *fdt, int nodeoffset, unsigned long *plmt_bas
 		if (rc)
 			continue;
 
-		if (SBI_HARTMASK_MAX_BITS <= hartid)
-			continue;
-
 		if (hwirq == IRQ_M_TIMER)
 			hcount++;
 	}
@@ -1153,9 +1147,6 @@ int fdt_parse_plicsw_node(const void *fdt, int nodeoffset, unsigned long *plicsw
 		if (rc)
 			continue;
 
-		if (SBI_HARTMASK_MAX_BITS <= hartid)
-			continue;
-
 		if (hwirq == IRQ_M_SOFT)
 			hcount++;
 	}
diff --git a/platform/generic/platform.c b/platform/generic/platform.c
index c03ed88..331ff8e 100644
--- a/platform/generic/platform.c
+++ b/platform/generic/platform.c
@@ -200,9 +200,6 @@ unsigned long fw_platform_init(unsigned long arg0, unsigned long arg1,
 		if (rc)
 			continue;
 
-		if (SBI_HARTMASK_MAX_BITS <= hartid)
-			continue;
-
 		if (!fdt_node_is_enabled(fdt, cpu_offset))
 			continue;
 
-- 
2.47.1

[PATCH] Work with hartid equal to or greater than SBI_HARTMASK_MAX_BITS.

[Inochi Amaoto]

On Thu, Jan 09, 2025 at 03:08:53PM -0800, Chao-ying Fu wrote:
> Some platforms use hartid that is equal to greater than SBI_HARTMASK_MAX_BITS,
> so we should remove the check.
> ---
>  lib/utils/fdt/fdt_domain.c  | 3 ---
>  lib/utils/fdt/fdt_helper.c  | 9 ---------
>  platform/generic/platform.c | 3 ---
>  3 files changed, 15 deletions(-)
> 

Is there a specific platform to test this patch? This also applies
to your other patches.

Regards,
Inochi

[PATCH] Work with hartid equal to or greater than SBI_HARTMASK_MAX_BITS.

[Chao-ying Fu]

On Thu, Jan 9, 2025 at 4:43?PM Inochi Amaoto <inochiama@gmail.com> wrote:
>
> On Thu, Jan 09, 2025 at 03:08:53PM -0800, Chao-ying Fu wrote:
> > Some platforms use hartid that is equal to greater than SBI_HARTMASK_MAX_BITS,
> > so we should remove the check.
> > ---
> >  lib/utils/fdt/fdt_domain.c  | 3 ---
> >  lib/utils/fdt/fdt_helper.c  | 9 ---------
> >  platform/generic/platform.c | 3 ---
> >  3 files changed, 15 deletions(-)
> >
>
> Is there a specific platform to test this patch? This also applies
> to your other patches.
>
> Regards,
> Inochi
We run OpenSBI and Linux on FPGA boards, RTL simulations, or QEMU. One
example of 24 hart ids is as follows.
0x0, 0x1, 0x10, 0x11, 0x20, 0x21, 0x30, 0x31,
0x10000, 0x10001, 0x10010, 0x10011, 0x10020, 0x10021, 0x10030, 0x10031,
0x20000, 0x20001, 0x20010, 0x20011, 0x20020, 0x20021, 0x20030, 0x20031.
After removing the hart id check, OpenSBI is fine.
Note that last year OpenSBI changed to use hart index to access the array.
Thanks!

Regards,
Chao-ying

[PATCH] Work with hartid equal to or greater than SBI_HARTMASK_MAX_BITS.

[Jessica Clarke]

On 9 Jan 2025, at 23:08, Chao-ying Fu <icebergfu@gmail.com> wrote:
> 
> Some platforms use hartid that is equal to greater than SBI_HARTMASK_MAX_BITS,
> so we should remove the check.

The checks aren?t there for fun, they?re required. From sbi_hartmask.h:

> /**
>  * Maximum number of bits in a hartmask
>  *
>  * The hartmask is indexed using physical HART id so this define
>  * also represents the maximum number of HART ids generic OpenSBI
>  * can handle.
>  */

OpenSBI does not support hart IDs outside this range today, so there is
real work to do if that?s needed, not just deleting checks. Otherwise
you?re going to end up with out-of-bounds memory accesses.

I?m concerned that none of the patches you?ve sent just now seem to
have been thought through properly. Whilst I?m sure you have the best
intentions, I also note that this kind of patch is precisely the kind
of patch one might make were one to want to insert vulnerabilities into
core system components.

Jess

> ---
> lib/utils/fdt/fdt_domain.c  | 3 ---
> lib/utils/fdt/fdt_helper.c  | 9 ---------
> platform/generic/platform.c | 3 ---
> 3 files changed, 15 deletions(-)
> 
> diff --git a/lib/utils/fdt/fdt_domain.c b/lib/utils/fdt/fdt_domain.c
> index 4bc7ed8..6bc63c2 100644
> --- a/lib/utils/fdt/fdt_domain.c
> +++ b/lib/utils/fdt/fdt_domain.c
> @@ -471,9 +471,6 @@ static int __fdt_parse_domain(const void *fdt, int domain_offset, void *opaque)
> if (err)
> continue;
> 
> - if (SBI_HARTMASK_MAX_BITS <= val32)
> - continue;
> -
> if (!fdt_node_is_enabled(fdt, cpu_offset))
> continue;
> 
> diff --git a/lib/utils/fdt/fdt_helper.c b/lib/utils/fdt/fdt_helper.c
> index cb350e5..aec08fa 100644
> --- a/lib/utils/fdt/fdt_helper.c
> +++ b/lib/utils/fdt/fdt_helper.c
> @@ -1032,9 +1032,6 @@ int fdt_parse_aclint_node(const void *fdt, int nodeoffset,
> if (rc)
> continue;
> 
> - if (SBI_HARTMASK_MAX_BITS <= hartid)
> - continue;
> -
> if (match_hwirq == hwirq) {
> if (hartid < first_hartid)
> first_hartid = hartid;
> @@ -1097,9 +1094,6 @@ int fdt_parse_plmt_node(const void *fdt, int nodeoffset, unsigned long *plmt_bas
> if (rc)
> continue;
> 
> - if (SBI_HARTMASK_MAX_BITS <= hartid)
> - continue;
> -
> if (hwirq == IRQ_M_TIMER)
> hcount++;
> }
> @@ -1153,9 +1147,6 @@ int fdt_parse_plicsw_node(const void *fdt, int nodeoffset, unsigned long *plicsw
> if (rc)
> continue;
> 
> - if (SBI_HARTMASK_MAX_BITS <= hartid)
> - continue;
> -
> if (hwirq == IRQ_M_SOFT)
> hcount++;
> }
> diff --git a/platform/generic/platform.c b/platform/generic/platform.c
> index c03ed88..331ff8e 100644
> --- a/platform/generic/platform.c
> +++ b/platform/generic/platform.c
> @@ -200,9 +200,6 @@ unsigned long fw_platform_init(unsigned long arg0, unsigned long arg1,
> if (rc)
> continue;
> 
> - if (SBI_HARTMASK_MAX_BITS <= hartid)
> - continue;
> -
> if (!fdt_node_is_enabled(fdt, cpu_offset))
> continue;
> 
> -- 
> 2.47.1
> 
> 
> -- 
> opensbi mailing list
> opensbi at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/opensbi

[PATCH] Work with hartid equal to or greater than SBI_HARTMASK_MAX_BITS.

[Chao-ying Fu]

On Thu, Jan 9, 2025 at 4:57?PM Jessica Clarke <jrtc27@jrtc27.com> wrote:
>
> On 9 Jan 2025, at 23:08, Chao-ying Fu <icebergfu@gmail.com> wrote:
> >
> > Some platforms use hartid that is equal to greater than SBI_HARTMASK_MAX_BITS,
> > so we should remove the check.
>
> The checks aren?t there for fun, they?re required. From sbi_hartmask.h:
>
> > /**
> >  * Maximum number of bits in a hartmask
> >  *
> >  * The hartmask is indexed using physical HART id so this define
> >  * also represents the maximum number of HART ids generic OpenSBI
> >  * can handle.
> >  */

This comment may not be accurate, as OpenSBI changed to use hart index
to access the array last year.

>
> OpenSBI does not support hart IDs outside this range today, so there is
> real work to do if that?s needed, not just deleting checks. Otherwise
> you?re going to end up with out-of-bounds memory accesses.
>
> I?m concerned that none of the patches you?ve sent just now seem to
> have been thought through properly. Whilst I?m sure you have the best
> intentions, I also note that this kind of patch is precisely the kind
> of patch one might make were one to want to insert vulnerabilities into
> core system components.
>
> Jess

We did run OpenSBI 1.6 and Linux 6.12 with hart ids (some of them >=
SBI_HARTMASK_MAX_BITS) with this patch on FPGA boards, RTL
simulations, and QEMU.
Thanks!

Regards,
Chao-ying

[PATCH v2] Work with hartid equal to or greater than SBI_HARTMASK_MAX_BITS

[Raj Vishwanathan]

Some platforms use hartid that is equal to greater than SBI_HARTMASK_MAX_BITS,
so we should remove the original check. Instead we check the hart index
against SBI_HARTMASK_MAX_BITS.
---
 lib/utils/fdt/fdt_domain.c  | 2 +-
 lib/utils/fdt/fdt_helper.c  | 6 +++---
 platform/generic/platform.c | 2 +-
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/lib/utils/fdt/fdt_domain.c b/lib/utils/fdt/fdt_domain.c
index 4bc7ed8..7d10d10 100644
--- a/lib/utils/fdt/fdt_domain.c
+++ b/lib/utils/fdt/fdt_domain.c
@@ -471,7 +471,7 @@ static int __fdt_parse_domain(const void *fdt, int domain_offset, void *opaque)
 		if (err)
 			continue;
 
-		if (SBI_HARTMASK_MAX_BITS <= val32)
+		if (SBI_HARTMASK_MAX_BITS <= sbi_hartid_to_hartindex(val32))
 			continue;
 
 		if (!fdt_node_is_enabled(fdt, cpu_offset))
diff --git a/lib/utils/fdt/fdt_helper.c b/lib/utils/fdt/fdt_helper.c
index cb350e5..4673921 100644
--- a/lib/utils/fdt/fdt_helper.c
+++ b/lib/utils/fdt/fdt_helper.c
@@ -1032,7 +1032,7 @@ int fdt_parse_aclint_node(const void *fdt, int nodeoffset,
 		if (rc)
 			continue;
 
-		if (SBI_HARTMASK_MAX_BITS <= hartid)
+		if (SBI_HARTMASK_MAX_BITS <= sbi_hartid_to_hartindex(hartid))
 			continue;
 
 		if (match_hwirq == hwirq) {
@@ -1097,7 +1097,7 @@ int fdt_parse_plmt_node(const void *fdt, int nodeoffset, unsigned long *plmt_bas
 		if (rc)
 			continue;
 
-		if (SBI_HARTMASK_MAX_BITS <= hartid)
+		if (SBI_HARTMASK_MAX_BITS <= sbi_hartid_to_hartindex(hartid))
 			continue;
 
 		if (hwirq == IRQ_M_TIMER)
@@ -1153,7 +1153,7 @@ int fdt_parse_plicsw_node(const void *fdt, int nodeoffset, unsigned long *plicsw
 		if (rc)
 			continue;
 
-		if (SBI_HARTMASK_MAX_BITS <= hartid)
+		if (SBI_HARTMASK_MAX_BITS <= sbi_hartid_to_hartindex(hartid))
 			continue;
 
 		if (hwirq == IRQ_M_SOFT)
diff --git a/platform/generic/platform.c b/platform/generic/platform.c
index c03ed88..6ec0d73 100644
--- a/platform/generic/platform.c
+++ b/platform/generic/platform.c
@@ -200,7 +200,7 @@ unsigned long fw_platform_init(unsigned long arg0, unsigned long arg1,
 		if (rc)
 			continue;
 
-		if (SBI_HARTMASK_MAX_BITS <= hartid)
+		if (SBI_HARTMASK_MAX_BITS <= hart_count)
 			continue;
 
 		if (!fdt_node_is_enabled(fdt, cpu_offset))
-- 
2.43.0

[PATCH v2] Work with hartid equal to or greater than SBI_HARTMASK_MAX_BITS

[Xiang W]

? 2025-01-15?? 15:46 -0800?Raj Vishwanathan???
> Some platforms use hartid that is equal to greater than SBI_HARTMASK_MAX_BITS,
> so we should remove the original check. Instead we check the hart index
> against SBI_HARTMASK_MAX_BITS.
> ---
> ?lib/utils/fdt/fdt_domain.c? | 2 +-
> ?lib/utils/fdt/fdt_helper.c? | 6 +++---
> ?platform/generic/platform.c | 2 +-
> ?3 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/lib/utils/fdt/fdt_domain.c b/lib/utils/fdt/fdt_domain.c
> index 4bc7ed8..7d10d10 100644
> --- a/lib/utils/fdt/fdt_domain.c
> +++ b/lib/utils/fdt/fdt_domain.c
> @@ -471,7 +471,7 @@ static int __fdt_parse_domain(const void *fdt, int domain_offset, void *opaque)
> ?		if (err)
> ?			continue;
> ?
> -		if (SBI_HARTMASK_MAX_BITS <= val32)
> +		if (SBI_HARTMASK_MAX_BITS <= sbi_hartid_to_hartindex(val32))
> ?			continue;
> ?
> ?		if (!fdt_node_is_enabled(fdt, cpu_offset))
> diff --git a/lib/utils/fdt/fdt_helper.c b/lib/utils/fdt/fdt_helper.c
> index cb350e5..4673921 100644
> --- a/lib/utils/fdt/fdt_helper.c
> +++ b/lib/utils/fdt/fdt_helper.c
> @@ -1032,7 +1032,7 @@ int fdt_parse_aclint_node(const void *fdt, int nodeoffset,
> ?		if (rc)
> ?			continue;
> ?
> -		if (SBI_HARTMASK_MAX_BITS <= hartid)
> +		if (SBI_HARTMASK_MAX_BITS <= sbi_hartid_to_hartindex(hartid))
> ?			continue;
> ?
> ?		if (match_hwirq == hwirq) {
> @@ -1097,7 +1097,7 @@ int fdt_parse_plmt_node(const void *fdt, int nodeoffset, unsigned long *plmt_bas
> ?		if (rc)
> ?			continue;
> ?
> -		if (SBI_HARTMASK_MAX_BITS <= hartid)
> +		if (SBI_HARTMASK_MAX_BITS <= sbi_hartid_to_hartindex(hartid))
> ?			continue;
> ?
> ?		if (hwirq == IRQ_M_TIMER)
> @@ -1153,7 +1153,7 @@ int fdt_parse_plicsw_node(const void *fdt, int nodeoffset, unsigned long *plicsw
> ?		if (rc)
> ?			continue;
> ?
> -		if (SBI_HARTMASK_MAX_BITS <= hartid)
> +		if (SBI_HARTMASK_MAX_BITS <= sbi_hartid_to_hartindex(hartid))
> ?			continue;
> ?
> ?		if (hwirq == IRQ_M_SOFT)
> diff --git a/platform/generic/platform.c b/platform/generic/platform.c
> index c03ed88..6ec0d73 100644
> --- a/platform/generic/platform.c
> +++ b/platform/generic/platform.c
> @@ -200,7 +200,7 @@ unsigned long fw_platform_init(unsigned long arg0, unsigned long arg1,
> ?		if (rc)
> ?			continue;
> ?
> -		if (SBI_HARTMASK_MAX_BITS <= hartid)
> +		if (SBI_HARTMASK_MAX_BITS <= hart_count)
> ?			continue;
s/continue/break

Regards,
Xiang W
> ?
> ?		if (!fdt_node_is_enabled(fdt, cpu_offset))
> -- 
> 2.43.0
> 
> 

[PATCH v3] Work with hartid equal to or greater than SBI_HARTMASK_MAX_BITS

[Raj Vishwanathan]

Some platforms use hartid that is equal to greater than SBI_HARTMASK_MAX_BITS,
so we should remove the original check. Instead we check the hart index
against SBI_HARTMASK_MAX_BITS.

Update: Make hart_count > SBI_HARTMASK_MAX_BITS a catastrophic failure. The reviewer suggested continue, immediate failure quickly will localize the issue.
---
 lib/utils/fdt/fdt_domain.c  | 2 +-
 lib/utils/fdt/fdt_helper.c  | 6 +++---
 platform/generic/platform.c | 4 ++--
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/lib/utils/fdt/fdt_domain.c b/lib/utils/fdt/fdt_domain.c
index 4bc7ed8..7d10d10 100644
--- a/lib/utils/fdt/fdt_domain.c
+++ b/lib/utils/fdt/fdt_domain.c
@@ -471,7 +471,7 @@ static int __fdt_parse_domain(const void *fdt, int domain_offset, void *opaque)
 		if (err)
 			continue;
 
-		if (SBI_HARTMASK_MAX_BITS <= val32)
+		if (SBI_HARTMASK_MAX_BITS <= sbi_hartid_to_hartindex(val32))
 			continue;
 
 		if (!fdt_node_is_enabled(fdt, cpu_offset))
diff --git a/lib/utils/fdt/fdt_helper.c b/lib/utils/fdt/fdt_helper.c
index cb350e5..4673921 100644
--- a/lib/utils/fdt/fdt_helper.c
+++ b/lib/utils/fdt/fdt_helper.c
@@ -1032,7 +1032,7 @@ int fdt_parse_aclint_node(const void *fdt, int nodeoffset,
 		if (rc)
 			continue;
 
-		if (SBI_HARTMASK_MAX_BITS <= hartid)
+		if (SBI_HARTMASK_MAX_BITS <= sbi_hartid_to_hartindex(hartid))
 			continue;
 
 		if (match_hwirq == hwirq) {
@@ -1097,7 +1097,7 @@ int fdt_parse_plmt_node(const void *fdt, int nodeoffset, unsigned long *plmt_bas
 		if (rc)
 			continue;
 
-		if (SBI_HARTMASK_MAX_BITS <= hartid)
+		if (SBI_HARTMASK_MAX_BITS <= sbi_hartid_to_hartindex(hartid))
 			continue;
 
 		if (hwirq == IRQ_M_TIMER)
@@ -1153,7 +1153,7 @@ int fdt_parse_plicsw_node(const void *fdt, int nodeoffset, unsigned long *plicsw
 		if (rc)
 			continue;
 
-		if (SBI_HARTMASK_MAX_BITS <= hartid)
+		if (SBI_HARTMASK_MAX_BITS <= sbi_hartid_to_hartindex(hartid))
 			continue;
 
 		if (hwirq == IRQ_M_SOFT)
diff --git a/platform/generic/platform.c b/platform/generic/platform.c
index c03ed88..c8a7a59 100644
--- a/platform/generic/platform.c
+++ b/platform/generic/platform.c
@@ -200,8 +200,8 @@ unsigned long fw_platform_init(unsigned long arg0, unsigned long arg1,
 		if (rc)
 			continue;
 
-		if (SBI_HARTMASK_MAX_BITS <= hartid)
-			continue;
+		if (SBI_HARTMASK_MAX_BITS <= hart_count)
+			goto fail;
 
 		if (!fdt_node_is_enabled(fdt, cpu_offset))
 			continue;
-- 
2.43.0

[PATCH v3] Work with hartid equal to or greater than SBI_HARTMASK_MAX_BITS

[Xiang W]

? 2025-01-20?? 17:12 -0800?Raj Vishwanathan???
> Some platforms use hartid that is equal to greater than SBI_HARTMASK_MAX_BITS,
> so we should remove the original check. Instead we check the hart index
> against SBI_HARTMASK_MAX_BITS.
> 
missing your Signed-off-by
> Update: Make hart_count > SBI_HARTMASK_MAX_BITS a catastrophic failure. The reviewer suggested continue, immediate failure quickly will
> localize the issue.
The previous line need move after '---'
> ---
> ?lib/utils/fdt/fdt_domain.c? | 2 +-
> ?lib/utils/fdt/fdt_helper.c? | 6 +++---
> ?platform/generic/platform.c | 4 ++--
> ?3 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/lib/utils/fdt/fdt_domain.c b/lib/utils/fdt/fdt_domain.c
> index 4bc7ed8..7d10d10 100644
> --- a/lib/utils/fdt/fdt_domain.c
> +++ b/lib/utils/fdt/fdt_domain.c
> @@ -471,7 +471,7 @@ static int __fdt_parse_domain(const void *fdt, int domain_offset, void *opaque)
> ?		if (err)
> ?			continue;
> ?
> -		if (SBI_HARTMASK_MAX_BITS <= val32)
> +		if (SBI_HARTMASK_MAX_BITS <= sbi_hartid_to_hartindex(val32))
> ?			continue;
> ?
> ?		if (!fdt_node_is_enabled(fdt, cpu_offset))
> diff --git a/lib/utils/fdt/fdt_helper.c b/lib/utils/fdt/fdt_helper.c
> index cb350e5..4673921 100644
> --- a/lib/utils/fdt/fdt_helper.c
> +++ b/lib/utils/fdt/fdt_helper.c
> @@ -1032,7 +1032,7 @@ int fdt_parse_aclint_node(const void *fdt, int nodeoffset,
> ?		if (rc)
> ?			continue;
> ?
> -		if (SBI_HARTMASK_MAX_BITS <= hartid)
> +		if (SBI_HARTMASK_MAX_BITS <= sbi_hartid_to_hartindex(hartid))
> ?			continue;
> ?
> ?		if (match_hwirq == hwirq) {
> @@ -1097,7 +1097,7 @@ int fdt_parse_plmt_node(const void *fdt, int nodeoffset, unsigned long *plmt_bas
> ?		if (rc)
> ?			continue;
> ?
> -		if (SBI_HARTMASK_MAX_BITS <= hartid)
> +		if (SBI_HARTMASK_MAX_BITS <= sbi_hartid_to_hartindex(hartid))
> ?			continue;
> ?
> ?		if (hwirq == IRQ_M_TIMER)
> @@ -1153,7 +1153,7 @@ int fdt_parse_plicsw_node(const void *fdt, int nodeoffset, unsigned long *plicsw
> ?		if (rc)
> ?			continue;
> ?
> -		if (SBI_HARTMASK_MAX_BITS <= hartid)
> +		if (SBI_HARTMASK_MAX_BITS <= sbi_hartid_to_hartindex(hartid))
> ?			continue;
> ?
> ?		if (hwirq == IRQ_M_SOFT)
> diff --git a/platform/generic/platform.c b/platform/generic/platform.c
> index c03ed88..c8a7a59 100644
> --- a/platform/generic/platform.c
> +++ b/platform/generic/platform.c
> @@ -200,8 +200,8 @@ unsigned long fw_platform_init(unsigned long arg0, unsigned long arg1,
> ?		if (rc)
> ?			continue;
> ?
> -		if (SBI_HARTMASK_MAX_BITS <= hartid)
> -			continue;
> +		if (SBI_HARTMASK_MAX_BITS <= hart_count)
> +			goto fail;
It can be ignored.

Regards,
Xiang W
> ?
> ?		if (!fdt_node_is_enabled(fdt, cpu_offset))
> ?			continue;
> -- 
> 2.43.0
> 
> 

[PATCH v3] Work with hartid equal to or greater than SBI_HARTMASK_MAX_BITS

[Raj Vishwanathan]

Some platforms use hartid that is equal to greater than SBI_HARTMASK_MAX_BITS,
so we should remove the original check. Instead we check the hart index
against SBI_HARTMASK_MAX_BITS.

Changes in V2:
    Update: Make hart_count > SBI_HARTMASK_MAX_BITS a catastrophic failure.

Changes in V3:
    Ignore if hart_count is more than SBI_HARTMASK_MAX_BITS.

Signed-off-by: Raj Vishwanathan <Raj.Vishwanathan@gmail.com>
---
 lib/utils/fdt/fdt_domain.c  | 2 +-
 lib/utils/fdt/fdt_helper.c  | 6 +++---
 platform/generic/platform.c | 4 ++--
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/lib/utils/fdt/fdt_domain.c b/lib/utils/fdt/fdt_domain.c
index 4bc7ed8..7d10d10 100644
--- a/lib/utils/fdt/fdt_domain.c
+++ b/lib/utils/fdt/fdt_domain.c
@@ -471,7 +471,7 @@ static int __fdt_parse_domain(const void *fdt, int domain_offset, void *opaque)
 		if (err)
 			continue;
 
-		if (SBI_HARTMASK_MAX_BITS <= val32)
+		if (SBI_HARTMASK_MAX_BITS <= sbi_hartid_to_hartindex(val32))
 			continue;
 
 		if (!fdt_node_is_enabled(fdt, cpu_offset))
diff --git a/lib/utils/fdt/fdt_helper.c b/lib/utils/fdt/fdt_helper.c
index cb350e5..4673921 100644
--- a/lib/utils/fdt/fdt_helper.c
+++ b/lib/utils/fdt/fdt_helper.c
@@ -1032,7 +1032,7 @@ int fdt_parse_aclint_node(const void *fdt, int nodeoffset,
 		if (rc)
 			continue;
 
-		if (SBI_HARTMASK_MAX_BITS <= hartid)
+		if (SBI_HARTMASK_MAX_BITS <= sbi_hartid_to_hartindex(hartid))
 			continue;
 
 		if (match_hwirq == hwirq) {
@@ -1097,7 +1097,7 @@ int fdt_parse_plmt_node(const void *fdt, int nodeoffset, unsigned long *plmt_bas
 		if (rc)
 			continue;
 
-		if (SBI_HARTMASK_MAX_BITS <= hartid)
+		if (SBI_HARTMASK_MAX_BITS <= sbi_hartid_to_hartindex(hartid))
 			continue;
 
 		if (hwirq == IRQ_M_TIMER)
@@ -1153,7 +1153,7 @@ int fdt_parse_plicsw_node(const void *fdt, int nodeoffset, unsigned long *plicsw
 		if (rc)
 			continue;
 
-		if (SBI_HARTMASK_MAX_BITS <= hartid)
+		if (SBI_HARTMASK_MAX_BITS <= sbi_hartid_to_hartindex(hartid))
 			continue;
 
 		if (hwirq == IRQ_M_SOFT)
diff --git a/platform/generic/platform.c b/platform/generic/platform.c
index c03ed88..027ec89 100644
--- a/platform/generic/platform.c
+++ b/platform/generic/platform.c
@@ -200,8 +200,8 @@ unsigned long fw_platform_init(unsigned long arg0, unsigned long arg1,
 		if (rc)
 			continue;
 
-		if (SBI_HARTMASK_MAX_BITS <= hartid)
-			continue;
+		if (SBI_HARTMASK_MAX_BITS <= hart_count)
+			break;
 
 		if (!fdt_node_is_enabled(fdt, cpu_offset))
 			continue;
-- 
2.43.0

[PATCH v3] Work with hartid equal to or greater than SBI_HARTMASK_MAX_BITS

[Anup Patel]

Simplify patch subject and use "lib: utils:" as subject prefix.

On Wed, Jan 22, 2025 at 6:13?AM Raj Vishwanathan
<raj.vishwanathan@gmail.com> wrote:
>
> Some platforms use hartid that is equal to greater than SBI_HARTMASK_MAX_BITS,
> so we should remove the original check. Instead we check the hart index
> against SBI_HARTMASK_MAX_BITS.
>
> Changes in V2:
>     Update: Make hart_count > SBI_HARTMASK_MAX_BITS a catastrophic failure.
>
> Changes in V3:
>     Ignore if hart_count is more than SBI_HARTMASK_MAX_BITS.

The change log should be below after "---" so that it is not included as part
of the commit description.

Regards,
Anup

>
> Signed-off-by: Raj Vishwanathan <Raj.Vishwanathan@gmail.com>
> ---
>  lib/utils/fdt/fdt_domain.c  | 2 +-
>  lib/utils/fdt/fdt_helper.c  | 6 +++---
>  platform/generic/platform.c | 4 ++--
>  3 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/lib/utils/fdt/fdt_domain.c b/lib/utils/fdt/fdt_domain.c
> index 4bc7ed8..7d10d10 100644
> --- a/lib/utils/fdt/fdt_domain.c
> +++ b/lib/utils/fdt/fdt_domain.c
> @@ -471,7 +471,7 @@ static int __fdt_parse_domain(const void *fdt, int domain_offset, void *opaque)
>                 if (err)
>                         continue;
>
> -               if (SBI_HARTMASK_MAX_BITS <= val32)
> +               if (SBI_HARTMASK_MAX_BITS <= sbi_hartid_to_hartindex(val32))
>                         continue;
>
>                 if (!fdt_node_is_enabled(fdt, cpu_offset))
> diff --git a/lib/utils/fdt/fdt_helper.c b/lib/utils/fdt/fdt_helper.c
> index cb350e5..4673921 100644
> --- a/lib/utils/fdt/fdt_helper.c
> +++ b/lib/utils/fdt/fdt_helper.c
> @@ -1032,7 +1032,7 @@ int fdt_parse_aclint_node(const void *fdt, int nodeoffset,
>                 if (rc)
>                         continue;
>
> -               if (SBI_HARTMASK_MAX_BITS <= hartid)
> +               if (SBI_HARTMASK_MAX_BITS <= sbi_hartid_to_hartindex(hartid))
>                         continue;
>
>                 if (match_hwirq == hwirq) {
> @@ -1097,7 +1097,7 @@ int fdt_parse_plmt_node(const void *fdt, int nodeoffset, unsigned long *plmt_bas
>                 if (rc)
>                         continue;
>
> -               if (SBI_HARTMASK_MAX_BITS <= hartid)
> +               if (SBI_HARTMASK_MAX_BITS <= sbi_hartid_to_hartindex(hartid))
>                         continue;
>
>                 if (hwirq == IRQ_M_TIMER)
> @@ -1153,7 +1153,7 @@ int fdt_parse_plicsw_node(const void *fdt, int nodeoffset, unsigned long *plicsw
>                 if (rc)
>                         continue;
>
> -               if (SBI_HARTMASK_MAX_BITS <= hartid)
> +               if (SBI_HARTMASK_MAX_BITS <= sbi_hartid_to_hartindex(hartid))
>                         continue;
>
>                 if (hwirq == IRQ_M_SOFT)
> diff --git a/platform/generic/platform.c b/platform/generic/platform.c
> index c03ed88..027ec89 100644
> --- a/platform/generic/platform.c
> +++ b/platform/generic/platform.c
> @@ -200,8 +200,8 @@ unsigned long fw_platform_init(unsigned long arg0, unsigned long arg1,
>                 if (rc)
>                         continue;
>
> -               if (SBI_HARTMASK_MAX_BITS <= hartid)
> -                       continue;
> +               if (SBI_HARTMASK_MAX_BITS <= hart_count)
> +                       break;
>
>                 if (!fdt_node_is_enabled(fdt, cpu_offset))
>                         continue;
> --
> 2.43.0
>
>
> --
> opensbi mailing list
> opensbi at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/opensbi

[PATCH v3] New configuration CONFIG_SBI_SCRATCH_ALLOC_ALIGNMENT

[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>
---

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

[PATCH v3] New configuration CONFIG_SBI_SCRATCH_ALLOC_ALIGNMENT

[Xiang W]

? 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
> 
> 

[PATCH v3] New configuration CONFIG_SBI_SCRATCH_ALLOC_ALIGNMENT

[Anup Patel]

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

[PATCH v3] New configuration CONFIG_SBI_SCRATCH_ALLOC_ALIGNMENT

[Raj Vishwanathan]

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

[PATCH v3] New configuration CONFIG_SBI_SCRATCH_ALLOC_ALIGNMENT

[Samuel Holland]

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
> 

[PATCH v4] Set the scratch allocation to alignment to cacheline size.

[Raj Vishwanathan]

We set the scratch allocation alignment to cacheline size,specified by
riscv,cbom-block-size in the dts file to avoid two atomic variables from
the same cache line causing livelock on some platforms. If the cacheline
is not specified, we set it a default value.

Signed-off-by: Raj Vishwanathan <Raj.Vishwanathan@gmail.com>
---
Changes in V3:
    Remove platform specific references to 64 Bytes.
Changes in V2:
    Added a new configuration to get the alignment size.
Change in V1:
    Original Patch
---
 include/sbi/sbi_platform.h         |  2 ++
 include/sbi_utils/fdt/fdt_helper.h |  1 +
 lib/sbi/sbi_scratch.c              | 34 ++++++++++++++++++++++++++++--
 lib/utils/fdt/fdt_helper.c         | 24 +++++++++++++++++++++
 platform/generic/platform.c        |  9 ++++++++
 5 files changed, 68 insertions(+), 2 deletions(-)

diff --git a/include/sbi/sbi_platform.h b/include/sbi/sbi_platform.h
index 6d5fbc7..0cea0fe 100644
--- a/include/sbi/sbi_platform.h
+++ b/include/sbi/sbi_platform.h
@@ -197,6 +197,8 @@ struct sbi_platform {
 	 * 2. HART id < SBI_HARTMASK_MAX_BITS
 	 */
 	const u32 *hart_index2id;
+	/** Allocation alignment for Scratch */
+	u32 cbom_block_size;
 };
 
 /**
diff --git a/include/sbi_utils/fdt/fdt_helper.h b/include/sbi_utils/fdt/fdt_helper.h
index 7329b84..0b82159 100644
--- a/include/sbi_utils/fdt/fdt_helper.h
+++ b/include/sbi_utils/fdt/fdt_helper.h
@@ -55,6 +55,7 @@ bool fdt_node_is_enabled(const void *fdt, int nodeoff);
 
 int fdt_parse_hart_id(const void *fdt, int cpu_offset, u32 *hartid);
 
+int fdt_parse_cbom_block_size(const void *fdt, int cpu_offset, u32 *cbom_block_size);
 int fdt_parse_max_enabled_hart_id(const void *fdt, u32 *max_hartid);
 
 int fdt_parse_timebase_frequency(const void *fdt, unsigned long *freq);
diff --git a/lib/sbi/sbi_scratch.c b/lib/sbi/sbi_scratch.c
index ccbbc68..8df54cc 100644
--- a/lib/sbi/sbi_scratch.c
+++ b/lib/sbi/sbi_scratch.c
@@ -14,6 +14,8 @@
 #include <sbi/sbi_scratch.h>
 #include <sbi/sbi_string.h>
 
+#define DEFAULT_SCRATCH_ALLOC_ALIGN __SIZEOF_POINTER__
+
 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 };
@@ -21,6 +23,27 @@ struct sbi_scratch *hartindex_to_scratch_table[SBI_HARTMASK_MAX_BITS + 1] = { 0
 static spinlock_t extra_lock = SPIN_LOCK_INITIALIZER;
 static unsigned long extra_offset = SBI_SCRATCH_EXTRA_SPACE_OFFSET;
 
+static u32 sbi_get_scratch_alloc_align(void)
+{
+	struct sbi_scratch *rscratch;
+	const struct sbi_platform *plat;
+	u32 hartid;
+	u32 hart_index;
+	/*
+	 * Get the alignment size. We will return DEFAULT_SCRATCH_ALLOC_ALIGNMENT
+	 * or riscv,cbom_block_size
+	 */
+	hartid = current_hartid();
+	hart_index = sbi_hartid_to_hartindex(hartid);
+	rscratch = sbi_hartindex_to_scratch(hart_index);
+	if (!rscratch)
+		return DEFAULT_SCRATCH_ALLOC_ALIGN;
+	plat = sbi_platform_ptr(rscratch);
+	if (!plat)
+		return DEFAULT_SCRATCH_ALLOC_ALIGN;
+	return plat->cbom_block_size ? plat->cbom_block_size : \
+									DEFAULT_SCRATCH_ALLOC_ALIGN;
+}
 u32 sbi_hartid_to_hartindex(u32 hartid)
 {
 	u32 i;
@@ -57,6 +80,7 @@ unsigned long sbi_scratch_alloc_offset(unsigned long size)
 	void *ptr;
 	unsigned long ret = 0;
 	struct sbi_scratch *rscratch;
+	u32 scratch_alloc_align = 0;
 
 	/*
 	 * We have a simple brain-dead allocator which never expects
@@ -70,8 +94,14 @@ unsigned long sbi_scratch_alloc_offset(unsigned long size)
 	if (!size)
 		return 0;
 
-	size += __SIZEOF_POINTER__ - 1;
-	size &= ~((unsigned long)__SIZEOF_POINTER__ - 1);
+	scratch_alloc_align = sbi_get_scratch_alloc_align();
+
+	/*
+     * We let the allocation align to cacheline 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);
 
diff --git a/lib/utils/fdt/fdt_helper.c b/lib/utils/fdt/fdt_helper.c
index cb350e5..bea4fdc 100644
--- a/lib/utils/fdt/fdt_helper.c
+++ b/lib/utils/fdt/fdt_helper.c
@@ -287,6 +287,30 @@ int fdt_parse_hart_id(const void *fdt, int cpu_offset, u32 *hartid)
 
 	return 0;
 }
+int fdt_parse_cbom_block_size(const void *fdt,int cpu_offset,u32 *cbom_block_size)
+{
+    int len;
+    const void *prop;
+    const fdt32_t *val;
+
+    if (!fdt || cpu_offset < 0)
+        return SBI_EINVAL;
+
+    prop = fdt_getprop(fdt, cpu_offset, "device_type", &len);
+    if (!prop || !len)
+        return SBI_EINVAL;
+    if (strncmp (prop, "cpu", strlen ("cpu")))
+        return SBI_EINVAL;
+
+    val = fdt_getprop(fdt, cpu_offset, "riscv,cbom-block-size", &len);
+    if (!val || len < sizeof(fdt32_t))
+        return SBI_EINVAL;
+
+    if (cbom_block_size)
+        *cbom_block_size = fdt32_to_cpu(*val);
+    return 0;
+
+}
 
 int fdt_parse_max_enabled_hart_id(const void *fdt, u32 *max_hartid)
 {
diff --git a/platform/generic/platform.c b/platform/generic/platform.c
index c03ed88..53abf0b 100644
--- a/platform/generic/platform.c
+++ b/platform/generic/platform.c
@@ -174,6 +174,9 @@ unsigned long fw_platform_init(unsigned long arg0, unsigned long arg1,
 	const void *fdt = (void *)arg1;
 	u32 hartid, hart_count = 0;
 	int rc, root_offset, cpus_offset, cpu_offset, len;
+	u32 cbom_block_size = __SIZEOF_POINTER__;
+	u32 tmp;
+
 
 	root_offset = fdt_path_offset(fdt, "/");
 	if (root_offset < 0)
@@ -207,11 +210,17 @@ unsigned long fw_platform_init(unsigned long arg0, unsigned long arg1,
 			continue;
 
 		generic_hart_index2id[hart_count++] = hartid;
+		rc = fdt_parse_cbom_block_size(fdt, cpu_offset,&tmp);
+		if (rc)
+			continue;
+		tmp = tmp ? tmp : __SIZEOF_POINTER__;
+		cbom_block_size = MAX(tmp,cbom_block_size);
 	}
 
 	platform.hart_count = hart_count;
 	platform.heap_size = fw_platform_get_heap_size(fdt, hart_count);
 	platform_has_mlevel_imsic = fdt_check_imsic_mlevel(fdt);
+	platform.cbom_block_size = cbom_block_size;
 
 	fw_platform_coldboot_harts_init(fdt);
 
-- 
2.43.0

[PATCH v4] Set the scratch allocation to alignment to cacheline size.

[Xiang W]

? 2025-03-09?? 07:44 -0700?Raj Vishwanathan???
> We set the scratch allocation alignment to cacheline size,specified by
> riscv,cbom-block-size in the dts file to avoid two atomic variables from
> the same cache line causing livelock on some platforms. If the cacheline
> is not specified, we set it a default value.
> 
> Signed-off-by: Raj Vishwanathan <Raj.Vishwanathan@gmail.com>
> ---
> Changes in V3:
> ??? Remove platform specific references to 64 Bytes.
> Changes in V2:
> ??? Added a new configuration to get the alignment size.
> Change in V1:
> ??? Original Patch
> ---
> ?include/sbi/sbi_platform.h???????? |? 2 ++
> ?include/sbi_utils/fdt/fdt_helper.h |? 1 +
> ?lib/sbi/sbi_scratch.c????????????? | 34 ++++++++++++++++++++++++++++--
> ?lib/utils/fdt/fdt_helper.c???????? | 24 +++++++++++++++++++++
> ?platform/generic/platform.c??????? |? 9 ++++++++
> ?5 files changed, 68 insertions(+), 2 deletions(-)
> 
> diff --git a/include/sbi/sbi_platform.h b/include/sbi/sbi_platform.h
> index 6d5fbc7..0cea0fe 100644
> --- a/include/sbi/sbi_platform.h
> +++ b/include/sbi/sbi_platform.h
> @@ -197,6 +197,8 @@ struct sbi_platform {
> ?	 * 2. HART id < SBI_HARTMASK_MAX_BITS
> ?	 */
> ?	const u32 *hart_index2id;
> +	/** Allocation alignment for Scratch */
> +	u32 cbom_block_size;
> ?};
> ?
> ?/**
> diff --git a/include/sbi_utils/fdt/fdt_helper.h b/include/sbi_utils/fdt/fdt_helper.h
> index 7329b84..0b82159 100644
> --- a/include/sbi_utils/fdt/fdt_helper.h
> +++ b/include/sbi_utils/fdt/fdt_helper.h
> @@ -55,6 +55,7 @@ bool fdt_node_is_enabled(const void *fdt, int nodeoff);
> ?
> ?int fdt_parse_hart_id(const void *fdt, int cpu_offset, u32 *hartid);
> ?
> +int fdt_parse_cbom_block_size(const void *fdt, int cpu_offset, u32 *cbom_block_size);
> ?int fdt_parse_max_enabled_hart_id(const void *fdt, u32 *max_hartid);
> ?
> ?int fdt_parse_timebase_frequency(const void *fdt, unsigned long *freq);
> diff --git a/lib/sbi/sbi_scratch.c b/lib/sbi/sbi_scratch.c
> index ccbbc68..8df54cc 100644
> --- a/lib/sbi/sbi_scratch.c
> +++ b/lib/sbi/sbi_scratch.c
> @@ -14,6 +14,8 @@
> ?#include <sbi/sbi_scratch.h>
> ?#include <sbi/sbi_string.h>
> ?
> +#define DEFAULT_SCRATCH_ALLOC_ALIGN __SIZEOF_POINTER__
> +
> ?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 };
> @@ -21,6 +23,27 @@ struct sbi_scratch *hartindex_to_scratch_table[SBI_HARTMASK_MAX_BITS + 1] = { 0
> ?static spinlock_t extra_lock = SPIN_LOCK_INITIALIZER;
> ?static unsigned long extra_offset = SBI_SCRATCH_EXTRA_SPACE_OFFSET;
> ?
> +static u32 sbi_get_scratch_alloc_align(void)
> +{
> +	struct sbi_scratch *rscratch;
> +	const struct sbi_platform *plat;
> +	u32 hartid;
> +	u32 hart_index;
> +	/*
> +	 * Get the alignment size. We will return DEFAULT_SCRATCH_ALLOC_ALIGNMENT
> +	 * or riscv,cbom_block_size
> +	 */
> +	hartid = current_hartid();
> +	hart_index = sbi_hartid_to_hartindex(hartid);
> +	rscratch = sbi_hartindex_to_scratch(hart_index);
> +	if (!rscratch)
> +		return DEFAULT_SCRATCH_ALLOC_ALIGN;
> +	plat = sbi_platform_ptr(rscratch);
plat can be directly obtained through sbi_platform_thishart_ptr

Regards,
Xiang W
> +	if (!plat)
> +		return DEFAULT_SCRATCH_ALLOC_ALIGN;
> +	return plat->cbom_block_size ? plat->cbom_block_size : \
> +									DEFAULT_SCRATCH_ALLOC_ALIGN;
> +}
> ?u32 sbi_hartid_to_hartindex(u32 hartid)
> ?{
> ?	u32 i;
> @@ -57,6 +80,7 @@ unsigned long sbi_scratch_alloc_offset(unsigned long size)
> ?	void *ptr;
> ?	unsigned long ret = 0;
> ?	struct sbi_scratch *rscratch;
> +	u32 scratch_alloc_align = 0;
> ?
> ?	/*
> ?	 * We have a simple brain-dead allocator which never expects
> @@ -70,8 +94,14 @@ unsigned long sbi_scratch_alloc_offset(unsigned long size)
> ?	if (!size)
> ?		return 0;
> ?
> -	size += __SIZEOF_POINTER__ - 1;
> -	size &= ~((unsigned long)__SIZEOF_POINTER__ - 1);
> +	scratch_alloc_align = sbi_get_scratch_alloc_align();
> +
> +	/*
> +???? * We let the allocation align to cacheline 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);
> ?
> diff --git a/lib/utils/fdt/fdt_helper.c b/lib/utils/fdt/fdt_helper.c
> index cb350e5..bea4fdc 100644
> --- a/lib/utils/fdt/fdt_helper.c
> +++ b/lib/utils/fdt/fdt_helper.c
> @@ -287,6 +287,30 @@ int fdt_parse_hart_id(const void *fdt, int cpu_offset, u32 *hartid)
> ?
> ?	return 0;
> ?}
> +int fdt_parse_cbom_block_size(const void *fdt,int cpu_offset,u32 *cbom_block_size)
> +{
> +??? int len;
> +??? const void *prop;
> +??? const fdt32_t *val;
> +
> +??? if (!fdt || cpu_offset < 0)
> +??????? return SBI_EINVAL;
> +
> +??? prop = fdt_getprop(fdt, cpu_offset, "device_type", &len);
> +??? if (!prop || !len)
> +??????? return SBI_EINVAL;
> +??? if (strncmp (prop, "cpu", strlen ("cpu")))
> +??????? return SBI_EINVAL;
> +
> +??? val = fdt_getprop(fdt, cpu_offset, "riscv,cbom-block-size", &len);
> +??? if (!val || len < sizeof(fdt32_t))
> +??????? return SBI_EINVAL;
> +
> +??? if (cbom_block_size)
> +??????? *cbom_block_size = fdt32_to_cpu(*val);
> +??? return 0;
> +
> +}
> ?
> ?int fdt_parse_max_enabled_hart_id(const void *fdt, u32 *max_hartid)
> ?{
> diff --git a/platform/generic/platform.c b/platform/generic/platform.c
> index c03ed88..53abf0b 100644
> --- a/platform/generic/platform.c
> +++ b/platform/generic/platform.c
> @@ -174,6 +174,9 @@ unsigned long fw_platform_init(unsigned long arg0, unsigned long arg1,
> ?	const void *fdt = (void *)arg1;
> ?	u32 hartid, hart_count = 0;
> ?	int rc, root_offset, cpus_offset, cpu_offset, len;
> +	u32 cbom_block_size = __SIZEOF_POINTER__;
> +	u32 tmp;
> +
> ?
> ?	root_offset = fdt_path_offset(fdt, "/");
> ?	if (root_offset < 0)
> @@ -207,11 +210,17 @@ unsigned long fw_platform_init(unsigned long arg0, unsigned long arg1,
> ?			continue;
> ?
> ?		generic_hart_index2id[hart_count++] = hartid;
> +		rc = fdt_parse_cbom_block_size(fdt, cpu_offset,&tmp);
> +		if (rc)
> +			continue;
> +		tmp = tmp ? tmp : __SIZEOF_POINTER__;
> +		cbom_block_size = MAX(tmp,cbom_block_size);
> ?	}
> ?
> ?	platform.hart_count = hart_count;
> ?	platform.heap_size = fw_platform_get_heap_size(fdt, hart_count);
> ?	platform_has_mlevel_imsic = fdt_check_imsic_mlevel(fdt);
> +	platform.cbom_block_size = cbom_block_size;
> ?
> ?	fw_platform_coldboot_harts_init(fdt);
> ?
> -- 
> 2.43.0
> 

[PATCH v4] Set the scratch allocation to alignment to cacheline size.

[Raj Vishwanathan]

Hi Xiang, Anup and Samuel

Please let me know if you think any other changes need to be done.
Otherwise I will make the changes suggested by Xiang and send an
updated patch.

Many thanks for all your reviews and suggestions.

Raj

On Sun, Mar 9, 2025 at 10:27?PM Xiang W <wxjstz@126.com> wrote:
>
> ? 2025-03-09?? 07:44 -0700?Raj Vishwanathan???
> > We set the scratch allocation alignment to cacheline size,specified by
> > riscv,cbom-block-size in the dts file to avoid two atomic variables from
> > the same cache line causing livelock on some platforms. If the cacheline
> > is not specified, we set it a default value.
> >
> > Signed-off-by: Raj Vishwanathan <Raj.Vishwanathan@gmail.com>
> > ---
> > Changes in V3:
> >     Remove platform specific references to 64 Bytes.
> > Changes in V2:
> >     Added a new configuration to get the alignment size.
> > Change in V1:
> >     Original Patch
> > ---
> >  include/sbi/sbi_platform.h         |  2 ++
> >  include/sbi_utils/fdt/fdt_helper.h |  1 +
> >  lib/sbi/sbi_scratch.c              | 34 ++++++++++++++++++++++++++++--
> >  lib/utils/fdt/fdt_helper.c         | 24 +++++++++++++++++++++
> >  platform/generic/platform.c        |  9 ++++++++
> >  5 files changed, 68 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/sbi/sbi_platform.h b/include/sbi/sbi_platform.h
> > index 6d5fbc7..0cea0fe 100644
> > --- a/include/sbi/sbi_platform.h
> > +++ b/include/sbi/sbi_platform.h
> > @@ -197,6 +197,8 @@ struct sbi_platform {
> >        * 2. HART id < SBI_HARTMASK_MAX_BITS
> >        */
> >       const u32 *hart_index2id;
> > +     /** Allocation alignment for Scratch */
> > +     u32 cbom_block_size;
> >  };
> >
> >  /**
> > diff --git a/include/sbi_utils/fdt/fdt_helper.h b/include/sbi_utils/fdt/fdt_helper.h
> > index 7329b84..0b82159 100644
> > --- a/include/sbi_utils/fdt/fdt_helper.h
> > +++ b/include/sbi_utils/fdt/fdt_helper.h
> > @@ -55,6 +55,7 @@ bool fdt_node_is_enabled(const void *fdt, int nodeoff);
> >
> >  int fdt_parse_hart_id(const void *fdt, int cpu_offset, u32 *hartid);
> >
> > +int fdt_parse_cbom_block_size(const void *fdt, int cpu_offset, u32 *cbom_block_size);
> >  int fdt_parse_max_enabled_hart_id(const void *fdt, u32 *max_hartid);
> >
> >  int fdt_parse_timebase_frequency(const void *fdt, unsigned long *freq);
> > diff --git a/lib/sbi/sbi_scratch.c b/lib/sbi/sbi_scratch.c
> > index ccbbc68..8df54cc 100644
> > --- a/lib/sbi/sbi_scratch.c
> > +++ b/lib/sbi/sbi_scratch.c
> > @@ -14,6 +14,8 @@
> >  #include <sbi/sbi_scratch.h>
> >  #include <sbi/sbi_string.h>
> >
> > +#define DEFAULT_SCRATCH_ALLOC_ALIGN __SIZEOF_POINTER__
> > +
> >  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 };
> > @@ -21,6 +23,27 @@ struct sbi_scratch *hartindex_to_scratch_table[SBI_HARTMASK_MAX_BITS + 1] = { 0
> >  static spinlock_t extra_lock = SPIN_LOCK_INITIALIZER;
> >  static unsigned long extra_offset = SBI_SCRATCH_EXTRA_SPACE_OFFSET;
> >
> > +static u32 sbi_get_scratch_alloc_align(void)
> > +{
> > +     struct sbi_scratch *rscratch;
> > +     const struct sbi_platform *plat;
> > +     u32 hartid;
> > +     u32 hart_index;
> > +     /*
> > +      * Get the alignment size. We will return DEFAULT_SCRATCH_ALLOC_ALIGNMENT
> > +      * or riscv,cbom_block_size
> > +      */
> > +     hartid = current_hartid();
> > +     hart_index = sbi_hartid_to_hartindex(hartid);
> > +     rscratch = sbi_hartindex_to_scratch(hart_index);
> > +     if (!rscratch)
> > +             return DEFAULT_SCRATCH_ALLOC_ALIGN;
> > +     plat = sbi_platform_ptr(rscratch);
> plat can be directly obtained through sbi_platform_thishart_ptr
>
> Regards,
> Xiang W
> > +     if (!plat)
> > +             return DEFAULT_SCRATCH_ALLOC_ALIGN;
> > +     return plat->cbom_block_size ? plat->cbom_block_size : \
> > +                                                                     DEFAULT_SCRATCH_ALLOC_ALIGN;
> > +}
> >  u32 sbi_hartid_to_hartindex(u32 hartid)
> >  {
> >       u32 i;
> > @@ -57,6 +80,7 @@ unsigned long sbi_scratch_alloc_offset(unsigned long size)
> >       void *ptr;
> >       unsigned long ret = 0;
> >       struct sbi_scratch *rscratch;
> > +     u32 scratch_alloc_align = 0;
> >
> >       /*
> >        * We have a simple brain-dead allocator which never expects
> > @@ -70,8 +94,14 @@ unsigned long sbi_scratch_alloc_offset(unsigned long size)
> >       if (!size)
> >               return 0;
> >
> > -     size += __SIZEOF_POINTER__ - 1;
> > -     size &= ~((unsigned long)__SIZEOF_POINTER__ - 1);
> > +     scratch_alloc_align = sbi_get_scratch_alloc_align();
> > +
> > +     /*
> > +     * We let the allocation align to cacheline 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);
> >
> > diff --git a/lib/utils/fdt/fdt_helper.c b/lib/utils/fdt/fdt_helper.c
> > index cb350e5..bea4fdc 100644
> > --- a/lib/utils/fdt/fdt_helper.c
> > +++ b/lib/utils/fdt/fdt_helper.c
> > @@ -287,6 +287,30 @@ int fdt_parse_hart_id(const void *fdt, int cpu_offset, u32 *hartid)
> >
> >       return 0;
> >  }
> > +int fdt_parse_cbom_block_size(const void *fdt,int cpu_offset,u32 *cbom_block_size)
> > +{
> > +    int len;
> > +    const void *prop;
> > +    const fdt32_t *val;
> > +
> > +    if (!fdt || cpu_offset < 0)
> > +        return SBI_EINVAL;
> > +
> > +    prop = fdt_getprop(fdt, cpu_offset, "device_type", &len);
> > +    if (!prop || !len)
> > +        return SBI_EINVAL;
> > +    if (strncmp (prop, "cpu", strlen ("cpu")))
> > +        return SBI_EINVAL;
> > +
> > +    val = fdt_getprop(fdt, cpu_offset, "riscv,cbom-block-size", &len);
> > +    if (!val || len < sizeof(fdt32_t))
> > +        return SBI_EINVAL;
> > +
> > +    if (cbom_block_size)
> > +        *cbom_block_size = fdt32_to_cpu(*val);
> > +    return 0;
> > +
> > +}
> >
> >  int fdt_parse_max_enabled_hart_id(const void *fdt, u32 *max_hartid)
> >  {
> > diff --git a/platform/generic/platform.c b/platform/generic/platform.c
> > index c03ed88..53abf0b 100644
> > --- a/platform/generic/platform.c
> > +++ b/platform/generic/platform.c
> > @@ -174,6 +174,9 @@ unsigned long fw_platform_init(unsigned long arg0, unsigned long arg1,
> >       const void *fdt = (void *)arg1;
> >       u32 hartid, hart_count = 0;
> >       int rc, root_offset, cpus_offset, cpu_offset, len;
> > +     u32 cbom_block_size = __SIZEOF_POINTER__;
> > +     u32 tmp;
> > +
> >
> >       root_offset = fdt_path_offset(fdt, "/");
> >       if (root_offset < 0)
> > @@ -207,11 +210,17 @@ unsigned long fw_platform_init(unsigned long arg0, unsigned long arg1,
> >                       continue;
> >
> >               generic_hart_index2id[hart_count++] = hartid;
> > +             rc = fdt_parse_cbom_block_size(fdt, cpu_offset,&tmp);
> > +             if (rc)
> > +                     continue;
> > +             tmp = tmp ? tmp : __SIZEOF_POINTER__;
> > +             cbom_block_size = MAX(tmp,cbom_block_size);
> >       }
> >
> >       platform.hart_count = hart_count;
> >       platform.heap_size = fw_platform_get_heap_size(fdt, hart_count);
> >       platform_has_mlevel_imsic = fdt_check_imsic_mlevel(fdt);
> > +     platform.cbom_block_size = cbom_block_size;
> >
> >       fw_platform_coldboot_harts_init(fdt);
> >
> > --
> > 2.43.0
> >
>

[PATCH v4] Set the scratch allocation to alignment to cacheline size.

[Xiang W]

? 2025-03-09?? 07:44 -0700?Raj Vishwanathan???
> We set the scratch allocation alignment to cacheline size,specified by
> riscv,cbom-block-size in the dts file to avoid two atomic variables from
> the same cache line causing livelock on some platforms. If the cacheline
> is not specified, we set it a default value.
> 
> Signed-off-by: Raj Vishwanathan <Raj.Vishwanathan@gmail.com>
> ---
> Changes in V3:
> ??? Remove platform specific references to 64 Bytes.
> Changes in V2:
> ??? Added a new configuration to get the alignment size.
> Change in V1:
> ??? Original Patch
> ---
> ?include/sbi/sbi_platform.h???????? |? 2 ++
> ?include/sbi_utils/fdt/fdt_helper.h |? 1 +
> ?lib/sbi/sbi_scratch.c????????????? | 34 ++++++++++++++++++++++++++++--
> ?lib/utils/fdt/fdt_helper.c???????? | 24 +++++++++++++++++++++
> ?platform/generic/platform.c??????? |? 9 ++++++++
> ?5 files changed, 68 insertions(+), 2 deletions(-)
> 
> diff --git a/include/sbi/sbi_platform.h b/include/sbi/sbi_platform.h
> index 6d5fbc7..0cea0fe 100644
> --- a/include/sbi/sbi_platform.h
> +++ b/include/sbi/sbi_platform.h
> @@ -197,6 +197,8 @@ struct sbi_platform {
> ?	 * 2. HART id < SBI_HARTMASK_MAX_BITS
> ?	 */
> ?	const u32 *hart_index2id;
> +	/** Allocation alignment for Scratch */
> +	u32 cbom_block_size;
> ?};
> ?
> ?/**
> diff --git a/include/sbi_utils/fdt/fdt_helper.h b/include/sbi_utils/fdt/fdt_helper.h
> index 7329b84..0b82159 100644
> --- a/include/sbi_utils/fdt/fdt_helper.h
> +++ b/include/sbi_utils/fdt/fdt_helper.h
> @@ -55,6 +55,7 @@ bool fdt_node_is_enabled(const void *fdt, int nodeoff);
> ?
> ?int fdt_parse_hart_id(const void *fdt, int cpu_offset, u32 *hartid);
> ?
> +int fdt_parse_cbom_block_size(const void *fdt, int cpu_offset, u32 *cbom_block_size);
> ?int fdt_parse_max_enabled_hart_id(const void *fdt, u32 *max_hartid);
> ?
> ?int fdt_parse_timebase_frequency(const void *fdt, unsigned long *freq);
> diff --git a/lib/sbi/sbi_scratch.c b/lib/sbi/sbi_scratch.c
> index ccbbc68..8df54cc 100644
> --- a/lib/sbi/sbi_scratch.c
> +++ b/lib/sbi/sbi_scratch.c
> @@ -14,6 +14,8 @@
> ?#include <sbi/sbi_scratch.h>
> ?#include <sbi/sbi_string.h>
> ?
> +#define DEFAULT_SCRATCH_ALLOC_ALIGN __SIZEOF_POINTER__
> +
> ?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 };
> @@ -21,6 +23,27 @@ struct sbi_scratch *hartindex_to_scratch_table[SBI_HARTMASK_MAX_BITS + 1] = { 0
> ?static spinlock_t extra_lock = SPIN_LOCK_INITIALIZER;
> ?static unsigned long extra_offset = SBI_SCRATCH_EXTRA_SPACE_OFFSET;
> ?
> +static u32 sbi_get_scratch_alloc_align(void)
> +{
> +	struct sbi_scratch *rscratch;
> +	const struct sbi_platform *plat;
> +	u32 hartid;
> +	u32 hart_index;
> +	/*
> +	 * Get the alignment size. We will return DEFAULT_SCRATCH_ALLOC_ALIGNMENT
> +	 * or riscv,cbom_block_size
> +	 */
> +	hartid = current_hartid();
> +	hart_index = sbi_hartid_to_hartindex(hartid);
> +	rscratch = sbi_hartindex_to_scratch(hart_index);
> +	if (!rscratch)
> +		return DEFAULT_SCRATCH_ALLOC_ALIGN;
> +	plat = sbi_platform_ptr(rscratch);
> +	if (!plat)
> +		return DEFAULT_SCRATCH_ALLOC_ALIGN;
> +	return plat->cbom_block_size ? plat->cbom_block_size : \
> +									DEFAULT_SCRATCH_ALLOC_ALIGN;

The minimum value of plat->cbom_block_size resolved in fw_platform_init?
is DEFAULT_SCRATCH_ALLOC_ALIGN, so plat->cbom_block_size?can be returned?
directly.

> +}
> ?u32 sbi_hartid_to_hartindex(u32 hartid)
> ?{
> ?	u32 i;
> @@ -57,6 +80,7 @@ unsigned long sbi_scratch_alloc_offset(unsigned long size)
> ?	void *ptr;
> ?	unsigned long ret = 0;
> ?	struct sbi_scratch *rscratch;
> +	u32 scratch_alloc_align = 0;
> ?
> ?	/*
> ?	 * We have a simple brain-dead allocator which never expects
> @@ -70,8 +94,14 @@ unsigned long sbi_scratch_alloc_offset(unsigned long size)
> ?	if (!size)
> ?		return 0;
> ?
> -	size += __SIZEOF_POINTER__ - 1;
> -	size &= ~((unsigned long)__SIZEOF_POINTER__ - 1);
> +	scratch_alloc_align = sbi_get_scratch_alloc_align();
> +
> +	/*
> +???? * We let the allocation align to cacheline 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);
> ?
> diff --git a/lib/utils/fdt/fdt_helper.c b/lib/utils/fdt/fdt_helper.c
> index cb350e5..bea4fdc 100644
> --- a/lib/utils/fdt/fdt_helper.c
> +++ b/lib/utils/fdt/fdt_helper.c
> @@ -287,6 +287,30 @@ int fdt_parse_hart_id(const void *fdt, int cpu_offset, u32 *hartid)
> ?
> ?	return 0;
> ?}
> +int fdt_parse_cbom_block_size(const void *fdt,int cpu_offset,u32 *cbom_block_size)
> +{
> +??? int len;
> +??? const void *prop;
> +??? const fdt32_t *val;
> +
> +??? if (!fdt || cpu_offset < 0)
> +??????? return SBI_EINVAL;
> +
> +??? prop = fdt_getprop(fdt, cpu_offset, "device_type", &len);
> +??? if (!prop || !len)
> +??????? return SBI_EINVAL;
> +??? if (strncmp (prop, "cpu", strlen ("cpu")))
> +??????? return SBI_EINVAL;
> +
> +??? val = fdt_getprop(fdt, cpu_offset, "riscv,cbom-block-size", &len);
> +??? if (!val || len < sizeof(fdt32_t))
> +??????? return SBI_EINVAL;
> +
> +??? if (cbom_block_size)
> +??????? *cbom_block_size = fdt32_to_cpu(*val);
> +??? return 0;
> +
> +}
> ?
> ?int fdt_parse_max_enabled_hart_id(const void *fdt, u32 *max_hartid)
> ?{
> diff --git a/platform/generic/platform.c b/platform/generic/platform.c
> index c03ed88..53abf0b 100644
> --- a/platform/generic/platform.c
> +++ b/platform/generic/platform.c
> @@ -174,6 +174,9 @@ unsigned long fw_platform_init(unsigned long arg0, unsigned long arg1,
> ?	const void *fdt = (void *)arg1;
> ?	u32 hartid, hart_count = 0;
> ?	int rc, root_offset, cpus_offset, cpu_offset, len;
> +	u32 cbom_block_size = __SIZEOF_POINTER__;
replace __SIZEOF_POINTER__ with DEFAULT_SCRATCH_ALLOC_ALIGN
> +	u32 tmp;
> +
> ?
> ?	root_offset = fdt_path_offset(fdt, "/");
> ?	if (root_offset < 0)
> @@ -207,11 +210,17 @@ unsigned long fw_platform_init(unsigned long arg0, unsigned long arg1,
> ?			continue;
> ?
> ?		generic_hart_index2id[hart_count++] = hartid;
> +		rc = fdt_parse_cbom_block_size(fdt, cpu_offset,&tmp);
> +		if (rc)
> +			continue;
> +		tmp = tmp ? tmp : __SIZEOF_POINTER__;
delete before line
> +		cbom_block_size = MAX(tmp,cbom_block_size);
> ?	}
> ?
> ?	platform.hart_count = hart_count;
> ?	platform.heap_size = fw_platform_get_heap_size(fdt, hart_count);
> ?	platform_has_mlevel_imsic = fdt_check_imsic_mlevel(fdt);
> +	platform.cbom_block_size = cbom_block_size;
> ?
> ?	fw_platform_coldboot_harts_init(fdt);
> ?
> -- 
> 2.43.0
> 
Regards,
Xiang W

[PATCH v3] Check that hartid is within the SBI_HARTMASK_MAX_BITS

[Raj Vishwanathan]

It is possible that hartid may not be sequential and it should not be validated
against SBI_HARTMASK_MAX_BITS. Instead we should check the index of the hartid, hart index, against SBI_HARTMASK_MAX_BITS.

Signed-off-by: Raj Vishwanathan <Raj.Vishwanathan@gmail.com>
--
Changes in V2:
    Update: Make hart_count > SBI_HARTMASK_MAX_BITS a catastrophic failure.

Changes in V1:
    Ignore if hart_count is more than SBI_HARTMASK_MAX_BITS.
---
 lib/utils/fdt/fdt_domain.c  | 2 +-
 lib/utils/fdt/fdt_helper.c  | 6 +++---
 platform/generic/platform.c | 4 ++--
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/lib/utils/fdt/fdt_domain.c b/lib/utils/fdt/fdt_domain.c
index 4bc7ed8..7d10d10 100644
--- a/lib/utils/fdt/fdt_domain.c
+++ b/lib/utils/fdt/fdt_domain.c
@@ -471,7 +471,7 @@ static int __fdt_parse_domain(const void *fdt, int domain_offset, void *opaque)
 		if (err)
 			continue;
 
-		if (SBI_HARTMASK_MAX_BITS <= val32)
+		if (SBI_HARTMASK_MAX_BITS <= sbi_hartid_to_hartindex(val32))
 			continue;
 
 		if (!fdt_node_is_enabled(fdt, cpu_offset))
diff --git a/lib/utils/fdt/fdt_helper.c b/lib/utils/fdt/fdt_helper.c
index cb350e5..4673921 100644
--- a/lib/utils/fdt/fdt_helper.c
+++ b/lib/utils/fdt/fdt_helper.c
@@ -1032,7 +1032,7 @@ int fdt_parse_aclint_node(const void *fdt, int nodeoffset,
 		if (rc)
 			continue;
 
-		if (SBI_HARTMASK_MAX_BITS <= hartid)
+		if (SBI_HARTMASK_MAX_BITS <= sbi_hartid_to_hartindex(hartid))
 			continue;
 
 		if (match_hwirq == hwirq) {
@@ -1097,7 +1097,7 @@ int fdt_parse_plmt_node(const void *fdt, int nodeoffset, unsigned long *plmt_bas
 		if (rc)
 			continue;
 
-		if (SBI_HARTMASK_MAX_BITS <= hartid)
+		if (SBI_HARTMASK_MAX_BITS <= sbi_hartid_to_hartindex(hartid))
 			continue;
 
 		if (hwirq == IRQ_M_TIMER)
@@ -1153,7 +1153,7 @@ int fdt_parse_plicsw_node(const void *fdt, int nodeoffset, unsigned long *plicsw
 		if (rc)
 			continue;
 
-		if (SBI_HARTMASK_MAX_BITS <= hartid)
+		if (SBI_HARTMASK_MAX_BITS <= sbi_hartid_to_hartindex(hartid))
 			continue;
 
 		if (hwirq == IRQ_M_SOFT)
diff --git a/platform/generic/platform.c b/platform/generic/platform.c
index c03ed88..027ec89 100644
--- a/platform/generic/platform.c
+++ b/platform/generic/platform.c
@@ -200,8 +200,8 @@ unsigned long fw_platform_init(unsigned long arg0, unsigned long arg1,
 		if (rc)
 			continue;
 
-		if (SBI_HARTMASK_MAX_BITS <= hartid)
-			continue;
+		if (SBI_HARTMASK_MAX_BITS <= hart_count)
+			break;
 
 		if (!fdt_node_is_enabled(fdt, cpu_offset))
 			continue;
-- 
2.43.0

[PATCH v3] Check that hartid is within the SBI_HARTMASK_MAX_BITS

[Xiang W]

? 2025-01-28?? 13:33 -0800?Raj Vishwanathan???
> It is possible that hartid may not be sequential and it should not be validated
> against SBI_HARTMASK_MAX_BITS. Instead we should check the index of the hartid, hart index, against SBI_HARTMASK_MAX_BITS.
> 
> Signed-off-by: Raj Vishwanathan <Raj.Vishwanathan@gmail.com>
LGTM

Reviewed-by: Xiang W <wxjstz@126.com>
> --
> Changes in V2:
> ??? Update: Make hart_count > SBI_HARTMASK_MAX_BITS a catastrophic failure.
> 
> Changes in V1:
> ??? Ignore if hart_count is more than SBI_HARTMASK_MAX_BITS.
> ---
> ?lib/utils/fdt/fdt_domain.c? | 2 +-
> ?lib/utils/fdt/fdt_helper.c? | 6 +++---
> ?platform/generic/platform.c | 4 ++--
> ?3 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/lib/utils/fdt/fdt_domain.c b/lib/utils/fdt/fdt_domain.c
> index 4bc7ed8..7d10d10 100644
> --- a/lib/utils/fdt/fdt_domain.c
> +++ b/lib/utils/fdt/fdt_domain.c
> @@ -471,7 +471,7 @@ static int __fdt_parse_domain(const void *fdt, int domain_offset, void *opaque)
> ?		if (err)
> ?			continue;
> ?
> -		if (SBI_HARTMASK_MAX_BITS <= val32)
> +		if (SBI_HARTMASK_MAX_BITS <= sbi_hartid_to_hartindex(val32))
> ?			continue;
> ?
> ?		if (!fdt_node_is_enabled(fdt, cpu_offset))
> diff --git a/lib/utils/fdt/fdt_helper.c b/lib/utils/fdt/fdt_helper.c
> index cb350e5..4673921 100644
> --- a/lib/utils/fdt/fdt_helper.c
> +++ b/lib/utils/fdt/fdt_helper.c
> @@ -1032,7 +1032,7 @@ int fdt_parse_aclint_node(const void *fdt, int nodeoffset,
> ?		if (rc)
> ?			continue;
> ?
> -		if (SBI_HARTMASK_MAX_BITS <= hartid)
> +		if (SBI_HARTMASK_MAX_BITS <= sbi_hartid_to_hartindex(hartid))
> ?			continue;
> ?
> ?		if (match_hwirq == hwirq) {
> @@ -1097,7 +1097,7 @@ int fdt_parse_plmt_node(const void *fdt, int nodeoffset, unsigned long *plmt_bas
> ?		if (rc)
> ?			continue;
> ?
> -		if (SBI_HARTMASK_MAX_BITS <= hartid)
> +		if (SBI_HARTMASK_MAX_BITS <= sbi_hartid_to_hartindex(hartid))
> ?			continue;
> ?
> ?		if (hwirq == IRQ_M_TIMER)
> @@ -1153,7 +1153,7 @@ int fdt_parse_plicsw_node(const void *fdt, int nodeoffset, unsigned long *plicsw
> ?		if (rc)
> ?			continue;
> ?
> -		if (SBI_HARTMASK_MAX_BITS <= hartid)
> +		if (SBI_HARTMASK_MAX_BITS <= sbi_hartid_to_hartindex(hartid))
> ?			continue;
> ?
> ?		if (hwirq == IRQ_M_SOFT)
> diff --git a/platform/generic/platform.c b/platform/generic/platform.c
> index c03ed88..027ec89 100644
> --- a/platform/generic/platform.c
> +++ b/platform/generic/platform.c
> @@ -200,8 +200,8 @@ unsigned long fw_platform_init(unsigned long arg0, unsigned long arg1,
> ?		if (rc)
> ?			continue;
> ?
> -		if (SBI_HARTMASK_MAX_BITS <= hartid)
> -			continue;
> +		if (SBI_HARTMASK_MAX_BITS <= hart_count)
> +			break;
> ?
> ?		if (!fdt_node_is_enabled(fdt, cpu_offset))
> ?			continue;
> -- 
> 2.43.0
> 
> 

[PATCH v3] lib: utils:Check that hartid is valid

[Raj Vishwanathan]

It is possible that hartid may not be sequential and it should not be validated
against SBI_HARTMASK_MAX_BITS. Instead we should check the index of the hartid, hart index, against SBI_HARTMASK_MAX_BITS.

Signed-off-by: Raj Vishwanathan <Raj.Vishwanathan@gmail.com>
---
Changes in V2:
    Update: Make hart_count > SBI_HARTMASK_MAX_BITS a catastrophic failure.

Changes in V1:
    Ignore if hart_count is more than SBI_HARTMASK_MAX_BITS.
---
 lib/utils/fdt/fdt_domain.c  | 2 +-
 lib/utils/fdt/fdt_helper.c  | 6 +++---
 platform/generic/platform.c | 4 ++--
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/lib/utils/fdt/fdt_domain.c b/lib/utils/fdt/fdt_domain.c
index 4bc7ed8..7d10d10 100644
--- a/lib/utils/fdt/fdt_domain.c
+++ b/lib/utils/fdt/fdt_domain.c
@@ -471,7 +471,7 @@ static int __fdt_parse_domain(const void *fdt, int domain_offset, void *opaque)
 		if (err)
 			continue;
 
-		if (SBI_HARTMASK_MAX_BITS <= val32)
+		if (SBI_HARTMASK_MAX_BITS <= sbi_hartid_to_hartindex(val32))
 			continue;
 
 		if (!fdt_node_is_enabled(fdt, cpu_offset))
diff --git a/lib/utils/fdt/fdt_helper.c b/lib/utils/fdt/fdt_helper.c
index cb350e5..4673921 100644
--- a/lib/utils/fdt/fdt_helper.c
+++ b/lib/utils/fdt/fdt_helper.c
@@ -1032,7 +1032,7 @@ int fdt_parse_aclint_node(const void *fdt, int nodeoffset,
 		if (rc)
 			continue;
 
-		if (SBI_HARTMASK_MAX_BITS <= hartid)
+		if (SBI_HARTMASK_MAX_BITS <= sbi_hartid_to_hartindex(hartid))
 			continue;
 
 		if (match_hwirq == hwirq) {
@@ -1097,7 +1097,7 @@ int fdt_parse_plmt_node(const void *fdt, int nodeoffset, unsigned long *plmt_bas
 		if (rc)
 			continue;
 
-		if (SBI_HARTMASK_MAX_BITS <= hartid)
+		if (SBI_HARTMASK_MAX_BITS <= sbi_hartid_to_hartindex(hartid))
 			continue;
 
 		if (hwirq == IRQ_M_TIMER)
@@ -1153,7 +1153,7 @@ int fdt_parse_plicsw_node(const void *fdt, int nodeoffset, unsigned long *plicsw
 		if (rc)
 			continue;
 
-		if (SBI_HARTMASK_MAX_BITS <= hartid)
+		if (SBI_HARTMASK_MAX_BITS <= sbi_hartid_to_hartindex(hartid))
 			continue;
 
 		if (hwirq == IRQ_M_SOFT)
diff --git a/platform/generic/platform.c b/platform/generic/platform.c
index c03ed88..027ec89 100644
--- a/platform/generic/platform.c
+++ b/platform/generic/platform.c
@@ -200,8 +200,8 @@ unsigned long fw_platform_init(unsigned long arg0, unsigned long arg1,
 		if (rc)
 			continue;
 
-		if (SBI_HARTMASK_MAX_BITS <= hartid)
-			continue;
+		if (SBI_HARTMASK_MAX_BITS <= hart_count)
+			break;
 
 		if (!fdt_node_is_enabled(fdt, cpu_offset))
 			continue;
-- 
2.43.0

[PATCH v3] lib: utils:Check that hartid is valid

[Anup Patel]

On Wed, Feb 12, 2025 at 3:30?AM Raj Vishwanathan
<raj.vishwanathan@gmail.com> wrote:
>
> It is possible that hartid may not be sequential and it should not be validated
> against SBI_HARTMASK_MAX_BITS. Instead we should check the index of the hartid, hart index, against SBI_HARTMASK_MAX_BITS.

Minor nit: Try to wrap the line around 75 characters

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

> ---
> Changes in V2:
>     Update: Make hart_count > SBI_HARTMASK_MAX_BITS a catastrophic failure.
>
> Changes in V1:
>     Ignore if hart_count is more than SBI_HARTMASK_MAX_BITS.
> ---
>  lib/utils/fdt/fdt_domain.c  | 2 +-
>  lib/utils/fdt/fdt_helper.c  | 6 +++---
>  platform/generic/platform.c | 4 ++--
>  3 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/lib/utils/fdt/fdt_domain.c b/lib/utils/fdt/fdt_domain.c
> index 4bc7ed8..7d10d10 100644
> --- a/lib/utils/fdt/fdt_domain.c
> +++ b/lib/utils/fdt/fdt_domain.c
> @@ -471,7 +471,7 @@ static int __fdt_parse_domain(const void *fdt, int domain_offset, void *opaque)
>                 if (err)
>                         continue;
>
> -               if (SBI_HARTMASK_MAX_BITS <= val32)
> +               if (SBI_HARTMASK_MAX_BITS <= sbi_hartid_to_hartindex(val32))
>                         continue;
>
>                 if (!fdt_node_is_enabled(fdt, cpu_offset))
> diff --git a/lib/utils/fdt/fdt_helper.c b/lib/utils/fdt/fdt_helper.c
> index cb350e5..4673921 100644
> --- a/lib/utils/fdt/fdt_helper.c
> +++ b/lib/utils/fdt/fdt_helper.c
> @@ -1032,7 +1032,7 @@ int fdt_parse_aclint_node(const void *fdt, int nodeoffset,
>                 if (rc)
>                         continue;
>
> -               if (SBI_HARTMASK_MAX_BITS <= hartid)
> +               if (SBI_HARTMASK_MAX_BITS <= sbi_hartid_to_hartindex(hartid))
>                         continue;
>
>                 if (match_hwirq == hwirq) {
> @@ -1097,7 +1097,7 @@ int fdt_parse_plmt_node(const void *fdt, int nodeoffset, unsigned long *plmt_bas
>                 if (rc)
>                         continue;
>
> -               if (SBI_HARTMASK_MAX_BITS <= hartid)
> +               if (SBI_HARTMASK_MAX_BITS <= sbi_hartid_to_hartindex(hartid))
>                         continue;
>
>                 if (hwirq == IRQ_M_TIMER)
> @@ -1153,7 +1153,7 @@ int fdt_parse_plicsw_node(const void *fdt, int nodeoffset, unsigned long *plicsw
>                 if (rc)
>                         continue;
>
> -               if (SBI_HARTMASK_MAX_BITS <= hartid)
> +               if (SBI_HARTMASK_MAX_BITS <= sbi_hartid_to_hartindex(hartid))
>                         continue;
>
>                 if (hwirq == IRQ_M_SOFT)
> diff --git a/platform/generic/platform.c b/platform/generic/platform.c
> index c03ed88..027ec89 100644
> --- a/platform/generic/platform.c
> +++ b/platform/generic/platform.c
> @@ -200,8 +200,8 @@ unsigned long fw_platform_init(unsigned long arg0, unsigned long arg1,
>                 if (rc)
>                         continue;
>
> -               if (SBI_HARTMASK_MAX_BITS <= hartid)
> -                       continue;
> +               if (SBI_HARTMASK_MAX_BITS <= hart_count)
> +                       break;
>
>                 if (!fdt_node_is_enabled(fdt, cpu_offset))
>                         continue;
> --
> 2.43.0
>
>
> --
> opensbi mailing list
> opensbi at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/opensbi

[PATCH v2] We add a new configuration CONFIG_SBI_SCRATCH_ALLOC_ALIGNMENT as an int

[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.
+
 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

[PATCH v3] New configuration CONFIG_SBI_SCRATCH_ALLOC_ALIGNMENT

[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.

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

[PATCH v3] New configuration CONFIG_SBI_SCRATCH_ALLOC_ALIGNMENT

[Xiang W]

? 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
> 
> 

[PATCH v3] New configuration CONFIG_SBI_SCRATCH_ALLOC_ALIGNMENT

[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.

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

[PATCH v3] New configuration CONFIG_SBI_SCRATCH_ALLOC_ALIGNMENT

[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.

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