[PATCH 0/3] Sparse HART id improvements

[PATCH 0/3] Sparse HART id improvements

[Xiang W]

Modify last_hartindex_having_scratch/hartindex_to_hartid_table
/hartindex_to_scratch_table as static variables to prevent accidental
modification and reduce memory consumption

Xiang W (3):
  lib: sbi: Refactor sbi_scratch_last_hartindex as a function
  lib: sbi: Refactor sbi_hartindex_to_hartid as a function
  lib: sbi: Refactor sbi_hartindex_to_scratch as a function

 include/sbi/sbi_scratch.h | 35 ++++++++++++-----------------------
 lib/sbi/sbi_scratch.c     | 30 ++++++++++++++++++++++++++----
 2 files changed, 38 insertions(+), 27 deletions(-)

-- 
2.40.1

[PATCH 1/3] lib: sbi: Refactor sbi_scratch_last_hartindex as a function

[Xiang W]

Refactor sbi_scratch_last_hartindex as a function. This prevents the
last_hartindex_having_scratch variable from being corrupted elsewhere

Signed-off-by: Xiang W <wxjstz@126.com>
---
 include/sbi/sbi_scratch.h | 5 +----
 lib/sbi/sbi_scratch.c     | 7 ++++++-
 2 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/include/sbi/sbi_scratch.h b/include/sbi/sbi_scratch.h
index e6a33ba..28f91ac 100644
--- a/include/sbi/sbi_scratch.h
+++ b/include/sbi/sbi_scratch.h
@@ -202,11 +202,8 @@ do {									\
 					= (__type)(__ptr);		\
 } while (0)
 
-/** Last HART index having a sbi_scratch pointer */
-extern u32 last_hartindex_having_scratch;
-
 /** Get last HART index having a sbi_scratch pointer */
-#define sbi_scratch_last_hartindex()	last_hartindex_having_scratch
+u32 sbi_scratch_last_hartindex(void);
 
 /** Check whether a particular HART index is valid or not */
 #define sbi_hartindex_valid(__hartindex) \
diff --git a/lib/sbi/sbi_scratch.c b/lib/sbi/sbi_scratch.c
index ccbbc68..5dab2e6 100644
--- a/lib/sbi/sbi_scratch.c
+++ b/lib/sbi/sbi_scratch.c
@@ -14,13 +14,18 @@
 #include <sbi/sbi_scratch.h>
 #include <sbi/sbi_string.h>
 
-u32 last_hartindex_having_scratch = 0;
+static 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 };
 
 static spinlock_t extra_lock = SPIN_LOCK_INITIALIZER;
 static unsigned long extra_offset = SBI_SCRATCH_EXTRA_SPACE_OFFSET;
 
+u32 sbi_scratch_last_hartindex(void)
+{
+	return last_hartindex_having_scratch;
+}
+
 u32 sbi_hartid_to_hartindex(u32 hartid)
 {
 	u32 i;
-- 
2.40.1

[PATCH 2/3] lib: sbi: Refactor sbi_hartindex_to_hartid as a function

[Xiang W]

Refactor sbi_hartindex_to_hartid as a function.
hartindex_to_hartid_table is modified to a pointer and points to
plat->hart_index2id to reduce memory consumption.

Signed-off-by: Xiang W <wxjstz@126.com>
---
 include/sbi/sbi_scratch.h | 14 +++++---------
 lib/sbi/sbi_scratch.c     | 14 ++++++++++++--
 2 files changed, 17 insertions(+), 11 deletions(-)

diff --git a/include/sbi/sbi_scratch.h b/include/sbi/sbi_scratch.h
index 28f91ac..6119ab4 100644
--- a/include/sbi/sbi_scratch.h
+++ b/include/sbi/sbi_scratch.h
@@ -209,15 +209,11 @@ u32 sbi_scratch_last_hartindex(void);
 #define sbi_hartindex_valid(__hartindex) \
 (((__hartindex) <= sbi_scratch_last_hartindex()) ? true : false)
 
-/** HART index to HART id table */
-extern u32 hartindex_to_hartid_table[];
-
-/** Get sbi_scratch from HART index */
-#define sbi_hartindex_to_hartid(__hartindex)		\
-({							\
-	((__hartindex) <= sbi_scratch_last_hartindex()) ?\
-	hartindex_to_hartid_table[__hartindex] : -1U;	\
-})
+/** Get sbi_scratch from HART index
+ * @param hartindex hart index
+ * @return phtsical HART id, -1U indicate input is invalid
+*/
+u32 sbi_hartindex_to_hartid(u32 hartindex);
 
 /** HART index to scratch table */
 extern struct sbi_scratch *hartindex_to_scratch_table[];
diff --git a/lib/sbi/sbi_scratch.c b/lib/sbi/sbi_scratch.c
index 5dab2e6..74288bd 100644
--- a/lib/sbi/sbi_scratch.c
+++ b/lib/sbi/sbi_scratch.c
@@ -15,7 +15,7 @@
 #include <sbi/sbi_string.h>
 
 static u32 last_hartindex_having_scratch = 0;
-u32 hartindex_to_hartid_table[SBI_HARTMASK_MAX_BITS + 1] = { -1U };
+static const u32 *hartindex_to_hartid_table = NULL;
 struct sbi_scratch *hartindex_to_scratch_table[SBI_HARTMASK_MAX_BITS + 1] = { 0 };
 
 static spinlock_t extra_lock = SPIN_LOCK_INITIALIZER;
@@ -37,6 +37,16 @@ u32 sbi_hartid_to_hartindex(u32 hartid)
 	return -1U;
 }
 
+u32 sbi_hartindex_to_hartid(u32 hartindex)
+{
+	if (hartindex < last_hartindex_having_scratch) {
+		if (hartindex_to_hartid_table)
+			return hartindex_to_hartid_table[hartindex];
+		return hartindex;
+	}
+	return -1U;
+}
+
 typedef struct sbi_scratch *(*hartid2scratch)(ulong hartid, ulong hartindex);
 
 int sbi_scratch_init(struct sbi_scratch *scratch)
@@ -46,12 +56,12 @@ int sbi_scratch_init(struct sbi_scratch *scratch)
 
 	for (i = 0; i < plat->hart_count; i++) {
 		h = (plat->hart_index2id) ? plat->hart_index2id[i] : i;
-		hartindex_to_hartid_table[i] = h;
 		hartindex_to_scratch_table[i] =
 			((hartid2scratch)scratch->hartid_to_scratch)(h, i);
 	}
 
 	last_hartindex_having_scratch = plat->hart_count - 1;
+	hartindex_to_hartid_table = plat->hart_index2id;
 
 	return 0;
 }
-- 
2.40.1

[PATCH 3/3] lib: sbi: Refactor sbi_hartindex_to_scratch as a function

[Xiang W]

Refactor sbi_hartindex_to_scratch as a function. This prevents the
hartindex_to_scratch_table variable from being corrupted elsewhere.

Signed-off-by: Xiang W <wxjstz@126.com>
---
 include/sbi/sbi_scratch.h | 14 +++++---------
 lib/sbi/sbi_scratch.c     |  9 ++++++++-
 2 files changed, 13 insertions(+), 10 deletions(-)

diff --git a/include/sbi/sbi_scratch.h b/include/sbi/sbi_scratch.h
index 6119ab4..3145880 100644
--- a/include/sbi/sbi_scratch.h
+++ b/include/sbi/sbi_scratch.h
@@ -215,15 +215,11 @@ u32 sbi_scratch_last_hartindex(void);
 */
 u32 sbi_hartindex_to_hartid(u32 hartindex);
 
-/** HART index to scratch table */
-extern struct sbi_scratch *hartindex_to_scratch_table[];
-
-/** Get sbi_scratch from HART index */
-#define sbi_hartindex_to_scratch(__hartindex)		\
-({							\
-	((__hartindex) <= sbi_scratch_last_hartindex()) ?\
-	hartindex_to_scratch_table[__hartindex] : NULL;\
-})
+/** Get sbi_scratch from HART index
+ * @param hartindex HART index
+ * @return scratch of HART
+*/
+struct sbi_scratch *sbi_hartindex_to_scratch(u32 hartindex);
 
 /**
  * Get logical index for given HART id
diff --git a/lib/sbi/sbi_scratch.c b/lib/sbi/sbi_scratch.c
index 74288bd..dc9ab6c 100644
--- a/lib/sbi/sbi_scratch.c
+++ b/lib/sbi/sbi_scratch.c
@@ -16,7 +16,7 @@
 
 static u32 last_hartindex_having_scratch = 0;
 static const u32 *hartindex_to_hartid_table = NULL;
-struct sbi_scratch *hartindex_to_scratch_table[SBI_HARTMASK_MAX_BITS + 1] = { 0 };
+static 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;
@@ -47,6 +47,13 @@ u32 sbi_hartindex_to_hartid(u32 hartindex)
 	return -1U;
 }
 
+struct sbi_scratch *sbi_hartindex_to_scratch(u32 hartindex)
+{
+	if (hartindex < last_hartindex_having_scratch)
+		return hartindex_to_scratch_table[hartindex];
+	return NULL;
+}
+
 typedef struct sbi_scratch *(*hartid2scratch)(ulong hartid, ulong hartindex);
 
 int sbi_scratch_init(struct sbi_scratch *scratch)
-- 
2.40.1

[PATCH 0/3] Sparse HART id improvements

[Anup Patel]

On Sun, Sep 24, 2023 at 9:28?PM Xiang W <wxjstz@126.com> wrote:
>
> Modify last_hartindex_having_scratch/hartindex_to_hartid_table
> /hartindex_to_scratch_table as static variables to prevent accidental
> modification and reduce memory consumption
>
> Xiang W (3):
>   lib: sbi: Refactor sbi_scratch_last_hartindex as a function
>   lib: sbi: Refactor sbi_hartindex_to_hartid as a function
>   lib: sbi: Refactor sbi_hartindex_to_scratch as a function

All these are in hot-path at various places so introducing a
function is certainly not helping.

Also, if some buggy code is accidentally modifying global data
then that buggy code should be fixed rather than papering over
the problem by introducing these functions.

Regards,
Anup

>
>  include/sbi/sbi_scratch.h | 35 ++++++++++++-----------------------
>  lib/sbi/sbi_scratch.c     | 30 ++++++++++++++++++++++++++----
>  2 files changed, 38 insertions(+), 27 deletions(-)
>
> --
> 2.40.1
>
>
> --
> opensbi mailing list
> opensbi at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/opensbi

[PATCH 0/3] Sparse HART id improvements

[Xiang W]

? 2023-10-06???? 10:05 +0530?Anup Patel???
> On Sun, Sep 24, 2023 at 9:28?PM Xiang W <wxjstz@126.com> wrote:
> > 
> > Modify last_hartindex_having_scratch/hartindex_to_hartid_table
> > /hartindex_to_scratch_table as static variables to prevent accidental
> > modification and reduce memory consumption
> > 
> > Xiang W (3):
> > ? lib: sbi: Refactor sbi_scratch_last_hartindex as a function
> > ? lib: sbi: Refactor sbi_hartindex_to_hartid as a function
> > ? lib: sbi: Refactor sbi_hartindex_to_scratch as a function
> 
> All these are in hot-path at various places so introducing a
> function is certainly not helping.
> 
> Also, if some buggy code is accidentally modifying global data
> then that buggy code should be fixed rather than papering over
> the problem by introducing these functions.
PATCH1:
The original code is a bit strange in that there is already a global variable
last_hartindex_having_scratch, but instead of using it outside of sbi_scratch.c,
the macro sbi_scratch_last_hartindex is used. If don't want to modify this macro
into a function, why not just change the name of the last_hartindex_having_scratch
renamed to sbi_scratch_last_hartindex and then remove the macro?

PATCH2:
The arrays hartindex_to_hartid_table and plat->hart_index2id store similar content,
and the extra length of the hartindex_to_hartid_table array is not used because
there is a determination of whether the index is less than sbi_scratch_last_ hartindex.
the array can be removed by adding just a little bit of checking.

Regards,
Xiang W
> 
> Regards,
> Anup
> 
> > 
> > ?include/sbi/sbi_scratch.h | 35 ++++++++++++-----------------------
> > ?lib/sbi/sbi_scratch.c???? | 30 ++++++++++++++++++++++++++----
> > ?2 files changed, 38 insertions(+), 27 deletions(-)
> > 
> > --
> > 2.40.1
> > 
> > 
> > --
> > opensbi mailing list
> > opensbi at lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/opensbi

[PATCH 0/3] Sparse HART id improvements

[Anup Patel]

On Sat, Oct 7, 2023 at 9:19?AM Xiang W <wxjstz@126.com> wrote:
>
> ? 2023-10-06???? 10:05 +0530?Anup Patel???
> > On Sun, Sep 24, 2023 at 9:28?PM Xiang W <wxjstz@126.com> wrote:
> > >
> > > Modify last_hartindex_having_scratch/hartindex_to_hartid_table
> > > /hartindex_to_scratch_table as static variables to prevent accidental
> > > modification and reduce memory consumption
> > >
> > > Xiang W (3):
> > >   lib: sbi: Refactor sbi_scratch_last_hartindex as a function
> > >   lib: sbi: Refactor sbi_hartindex_to_hartid as a function
> > >   lib: sbi: Refactor sbi_hartindex_to_scratch as a function
> >
> > All these are in hot-path at various places so introducing a
> > function is certainly not helping.
> >
> > Also, if some buggy code is accidentally modifying global data
> > then that buggy code should be fixed rather than papering over
> > the problem by introducing these functions.
> PATCH1:
> The original code is a bit strange in that there is already a global variable
> last_hartindex_having_scratch, but instead of using it outside of sbi_scratch.c,
> the macro sbi_scratch_last_hartindex is used. If don't want to modify this macro
> into a function, why not just change the name of the last_hartindex_having_scratch
> renamed to sbi_scratch_last_hartindex and then remove the macro?

The sbi_scratch_last_hartindex() macro allows adding instrumentation
code for debugging purposes. The caller of sbi_scratch_last_hartindex()
should not care about how it is implemented.

>
> PATCH2:
> The arrays hartindex_to_hartid_table and plat->hart_index2id store similar content,
> and the extra length of the hartindex_to_hartid_table array is not used because
> there is a determination of whether the index is less than sbi_scratch_last_ hartindex.
> the array can be removed by adding just a little bit of checking.

The "plat->hart_index2id" is optional and can be NULL as well so
we have to always use "if ()" before accessing "plat->hart_index2id".

Also, we always have to access "hart_index2id[]" via "plat" pointer
which is slower compared to a global array like "hartindex_to_hartid_table[]".

Regards,
Anup

>
> Regards,
> Xiang W
> >
> > Regards,
> > Anup
> >
> > >
> > >  include/sbi/sbi_scratch.h | 35 ++++++++++++-----------------------
> > >  lib/sbi/sbi_scratch.c     | 30 ++++++++++++++++++++++++++----
> > >  2 files changed, 38 insertions(+), 27 deletions(-)
> > >
> > > --
> > > 2.40.1
> > >
> > >
> > > --
> > > opensbi mailing list
> > > opensbi at lists.infradead.org
> > > http://lists.infradead.org/mailman/listinfo/opensbi
>

[PATCH 0/3] Sparse HART id improvements

[Xiang W]

? 2023-10-09???? 14:06 +0530?Anup Patel???
> On Sat, Oct 7, 2023 at 9:19?AM Xiang W <wxjstz@126.com> wrote:
> > 
> > ? 2023-10-06???? 10:05 +0530?Anup Patel???
> > > On Sun, Sep 24, 2023 at 9:28?PM Xiang W <wxjstz@126.com> wrote:
> > > > 
> > > > Modify last_hartindex_having_scratch/hartindex_to_hartid_table
> > > > /hartindex_to_scratch_table as static variables to prevent accidental
> > > > modification and reduce memory consumption
> > > > 
> > > > Xiang W (3):
> > > > ? lib: sbi: Refactor sbi_scratch_last_hartindex as a function
> > > > ? lib: sbi: Refactor sbi_hartindex_to_hartid as a function
> > > > ? lib: sbi: Refactor sbi_hartindex_to_scratch as a function
> > > 
> > > All these are in hot-path at various places so introducing a
> > > function is certainly not helping.
> > > 
> > > Also, if some buggy code is accidentally modifying global data
> > > then that buggy code should be fixed rather than papering over
> > > the problem by introducing these functions.
> > PATCH1:
> > The original code is a bit strange in that there is already a global variable
> > last_hartindex_having_scratch, but instead of using it outside of sbi_scratch.c,
> > the macro sbi_scratch_last_hartindex is used. If don't want to modify this macro
> > into a function, why not just change the name of the last_hartindex_having_scratch
> > renamed to sbi_scratch_last_hartindex and then remove the macro?
> 
> The sbi_scratch_last_hartindex() macro allows adding instrumentation
> code for debugging purposes. The caller of sbi_scratch_last_hartindex()
> should not care about how it is implemented.
> 
> > 
> > PATCH2:
> > The arrays hartindex_to_hartid_table and plat->hart_index2id store similar content,
> > and the extra length of the hartindex_to_hartid_table array is not used because
> > there is a determination of whether the index is less than sbi_scratch_last_ hartindex.
> > the array can be removed by adding just a little bit of checking.
> 
> The "plat->hart_index2id" is optional and can be NULL as well so
> we have to always use "if ()" before accessing "plat->hart_index2id".
> 
> Also, we always have to access "hart_index2id[]" via "plat" pointer
> which is slower compared to a global array like "hartindex_to_hartid_table[]".
My expression may not be very accurate, please review my code.
https://lists.infradead.org/pipermail/opensbi/2023-October/005703.html

Regards,
Xiang W
> 
> Regards,
> Anup
> 
> > 
> > Regards,
> > Xiang W
> > > 
> > > Regards,
> > > Anup
> > > 
> > > > 
> > > > ?include/sbi/sbi_scratch.h | 35 ++++++++++++-----------------------
> > > > ?lib/sbi/sbi_scratch.c???? | 30 ++++++++++++++++++++++++++----
> > > > ?2 files changed, 38 insertions(+), 27 deletions(-)
> > > > 
> > > > --
> > > > 2.40.1
> > > > 
> > > > 
> > > > --
> > > > opensbi mailing list
> > > > opensbi at lists.infradead.org
> > > > http://lists.infradead.org/mailman/listinfo/opensbi
> >