[kvm-unit-tests PATCH v2 0/4] riscv: sbi: Add support to test HSM extension

[kvm-unit-tests PATCH v2 0/4] riscv: sbi: Add support to test HSM extension

[James Raphael Tiovalen]

This patch series adds support for testing all 4 functions of the HSM
extension as defined in the RISC-V SBI specification. The first 3
patches add some helper routines to prepare for the HSM test, while the
last patch adds the actual test for the HSM extension.

James Raphael Tiovalen (4):
  lib/report: Add helper methods to clear multiple prefixes
  riscv: sbi: Add IPI extension support
  riscv: sbi: Add HSM extension functions
  riscv: sbi: Add tests for HSM extension

 riscv/Makefile      |   7 +-
 lib/riscv/asm/sbi.h |  23 +++
 lib/libcflat.h      |   2 +
 lib/report.c        |  13 ++
 lib/riscv/sbi.c     |  15 ++
 riscv/sbi-asm.S     |  79 +++++++++
 riscv/sbi.c         | 387 ++++++++++++++++++++++++++++++++++++++++++++
 7 files changed, 523 insertions(+), 3 deletions(-)
 create mode 100644 riscv/sbi-asm.S

--
2.43.0

[kvm-unit-tests PATCH v2 0/4] riscv: sbi: Add support to test HSM extension

[James Raphael Tiovalen]

This patch series adds support for testing all 4 functions of the HSM
extension as defined in the RISC-V SBI specification. The first 3
patches add some helper routines to prepare for the HSM test, while the
last patch adds the actual test for the HSM extension.

James Raphael Tiovalen (4):
  lib/report: Add helper methods to clear multiple prefixes
  riscv: sbi: Add IPI extension support
  riscv: sbi: Add HSM extension functions
  riscv: sbi: Add tests for HSM extension

 riscv/Makefile      |   7 +-
 lib/riscv/asm/sbi.h |  23 +++
 lib/libcflat.h      |   2 +
 lib/report.c        |  13 ++
 lib/riscv/sbi.c     |  15 ++
 riscv/sbi-asm.S     |  79 +++++++++
 riscv/sbi.c         | 387 ++++++++++++++++++++++++++++++++++++++++++++
 7 files changed, 523 insertions(+), 3 deletions(-)
 create mode 100644 riscv/sbi-asm.S

--
2.43.0

[kvm-unit-tests PATCH v2 1/4] lib/report: Add helper methods to clear multiple prefixes

[James Raphael Tiovalen]

Add a method to pop a specified number of prefixes and another method to
clear all prefixes.

Suggested-by: Andrew Jones <andrew.jones@linux.dev>
Signed-off-by: James Raphael Tiovalen <jamestiotio@gmail.com>
---
 lib/libcflat.h |  2 ++
 lib/report.c   | 13 +++++++++++++
 2 files changed, 15 insertions(+)

diff --git a/lib/libcflat.h b/lib/libcflat.h
index 16a83880..0286ddec 100644
--- a/lib/libcflat.h
+++ b/lib/libcflat.h
@@ -96,6 +96,8 @@ void report_prefix_pushf(const char *prefix_fmt, ...)
 					__attribute__((format(printf, 1, 2)));
 extern void report_prefix_push(const char *prefix);
 extern void report_prefix_pop(void);
+extern void report_prefix_popn(int n);
+extern void report_prefix_clear(void);
 extern void report(bool pass, const char *msg_fmt, ...)
 		__attribute__((format(printf, 2, 3), nonnull(2)));
 extern void report_xfail(bool xfail, bool pass, const char *msg_fmt, ...)
diff --git a/lib/report.c b/lib/report.c
index 7f3c4f05..d45afedc 100644
--- a/lib/report.c
+++ b/lib/report.c
@@ -80,6 +80,19 @@ void report_prefix_pop(void)
 	spin_unlock(&lock);
 }
 
+void report_prefix_popn(int n)
+{
+	while (n--)
+		report_prefix_pop();
+}
+
+void report_prefix_clear(void)
+{
+	spin_lock(&lock);
+	prefixes[0] = '\0';
+	spin_unlock(&lock);
+}
+
 static void va_report(const char *msg_fmt,
 		bool pass, bool xfail, bool kfail, bool skip, va_list va)
 {
-- 
2.43.0

[kvm-unit-tests PATCH v2 1/4] lib/report: Add helper methods to clear multiple prefixes

[James Raphael Tiovalen]

Add a method to pop a specified number of prefixes and another method to
clear all prefixes.

Suggested-by: Andrew Jones <andrew.jones@linux.dev>
Signed-off-by: James Raphael Tiovalen <jamestiotio@gmail.com>
---
 lib/libcflat.h |  2 ++
 lib/report.c   | 13 +++++++++++++
 2 files changed, 15 insertions(+)

diff --git a/lib/libcflat.h b/lib/libcflat.h
index 16a83880..0286ddec 100644
--- a/lib/libcflat.h
+++ b/lib/libcflat.h
@@ -96,6 +96,8 @@ void report_prefix_pushf(const char *prefix_fmt, ...)
 					__attribute__((format(printf, 1, 2)));
 extern void report_prefix_push(const char *prefix);
 extern void report_prefix_pop(void);
+extern void report_prefix_popn(int n);
+extern void report_prefix_clear(void);
 extern void report(bool pass, const char *msg_fmt, ...)
 		__attribute__((format(printf, 2, 3), nonnull(2)));
 extern void report_xfail(bool xfail, bool pass, const char *msg_fmt, ...)
diff --git a/lib/report.c b/lib/report.c
index 7f3c4f05..d45afedc 100644
--- a/lib/report.c
+++ b/lib/report.c
@@ -80,6 +80,19 @@ void report_prefix_pop(void)
 	spin_unlock(&lock);
 }
 
+void report_prefix_popn(int n)
+{
+	while (n--)
+		report_prefix_pop();
+}
+
+void report_prefix_clear(void)
+{
+	spin_lock(&lock);
+	prefixes[0] = '\0';
+	spin_unlock(&lock);
+}
+
 static void va_report(const char *msg_fmt,
 		bool pass, bool xfail, bool kfail, bool skip, va_list va)
 {
-- 
2.43.0

[kvm-unit-tests PATCH v2 1/4] lib/report: Add helper methods to clear multiple prefixes

[Andrew Jones]

On Mon, Aug 26, 2024 at 01:08:21AM GMT, James Raphael Tiovalen wrote:
> Add a method to pop a specified number of prefixes and another method to
> clear all prefixes.
> 
> Suggested-by: Andrew Jones <andrew.jones@linux.dev>
> Signed-off-by: James Raphael Tiovalen <jamestiotio@gmail.com>
> ---
>  lib/libcflat.h |  2 ++
>  lib/report.c   | 13 +++++++++++++
>  2 files changed, 15 insertions(+)
> 
> diff --git a/lib/libcflat.h b/lib/libcflat.h
> index 16a83880..0286ddec 100644
> --- a/lib/libcflat.h
> +++ b/lib/libcflat.h
> @@ -96,6 +96,8 @@ void report_prefix_pushf(const char *prefix_fmt, ...)
>  					__attribute__((format(printf, 1, 2)));
>  extern void report_prefix_push(const char *prefix);
>  extern void report_prefix_pop(void);
> +extern void report_prefix_popn(int n);
> +extern void report_prefix_clear(void);
>  extern void report(bool pass, const char *msg_fmt, ...)
>  		__attribute__((format(printf, 2, 3), nonnull(2)));
>  extern void report_xfail(bool xfail, bool pass, const char *msg_fmt, ...)
> diff --git a/lib/report.c b/lib/report.c
> index 7f3c4f05..d45afedc 100644
> --- a/lib/report.c
> +++ b/lib/report.c
> @@ -80,6 +80,19 @@ void report_prefix_pop(void)
>  	spin_unlock(&lock);
>  }
>  
> +void report_prefix_popn(int n)
> +{
> +	while (n--)
> +		report_prefix_pop();

I think I suggested this implementation, but thinking about it some more
this won't work well with other cpus pushing/popping simultaneously. We
need something like

 static void __report_prefix_pop(void)
 {
        char *p, *q;

        if (!*prefixes)
                return;

        for (p = prefixes, q = strstr(p, PREFIX_DELIMITER) + 2;
                        *q;
                        p = q, q = strstr(p, PREFIX_DELIMITER) + 2)
                ;
        *p = '\0';
 }

 void report_prefix_pop(void)
 {
        spin_lock(&lock);
	__report_prefix_pop();
        spin_unlock(&lock);
 }

 void report_prefix_popn(int n)
 {
        spin_lock(&lock);
        while (n--)
	        __report_prefix_pop();
        spin_unlock(&lock);
 }

> +}
> +
> +void report_prefix_clear(void)
> +{
> +	spin_lock(&lock);
> +	prefixes[0] = '\0';
> +	spin_unlock(&lock);
> +}

I'm also second guessing the utility of this one. We'd probably almost
never want to do this since most tests are designed with a

  main()
  {
     report_prefix_push("mytest");
     subtest1();
     subtest2();
     ...
     report_prefix_pop();
     ...
  }

type pattern and we wouldn't want to lose that "mytest" prefix when some
subtest calls clear. Let's just drop report_prefix_clear() for now.

> +
>  static void va_report(const char *msg_fmt,
>  		bool pass, bool xfail, bool kfail, bool skip, va_list va)
>  {
> -- 
> 2.43.0
>

Thanks,
drew

Re: [kvm-unit-tests PATCH v2 1/4] lib/report: Add helper methods to clear multiple prefixes

[Andrew Jones]

On Mon, Aug 26, 2024 at 01:08:21AM GMT, James Raphael Tiovalen wrote:
> Add a method to pop a specified number of prefixes and another method to
> clear all prefixes.
> 
> Suggested-by: Andrew Jones <andrew.jones@linux.dev>
> Signed-off-by: James Raphael Tiovalen <jamestiotio@gmail.com>
> ---
>  lib/libcflat.h |  2 ++
>  lib/report.c   | 13 +++++++++++++
>  2 files changed, 15 insertions(+)
> 
> diff --git a/lib/libcflat.h b/lib/libcflat.h
> index 16a83880..0286ddec 100644
> --- a/lib/libcflat.h
> +++ b/lib/libcflat.h
> @@ -96,6 +96,8 @@ void report_prefix_pushf(const char *prefix_fmt, ...)
>  					__attribute__((format(printf, 1, 2)));
>  extern void report_prefix_push(const char *prefix);
>  extern void report_prefix_pop(void);
> +extern void report_prefix_popn(int n);
> +extern void report_prefix_clear(void);
>  extern void report(bool pass, const char *msg_fmt, ...)
>  		__attribute__((format(printf, 2, 3), nonnull(2)));
>  extern void report_xfail(bool xfail, bool pass, const char *msg_fmt, ...)
> diff --git a/lib/report.c b/lib/report.c
> index 7f3c4f05..d45afedc 100644
> --- a/lib/report.c
> +++ b/lib/report.c
> @@ -80,6 +80,19 @@ void report_prefix_pop(void)
>  	spin_unlock(&lock);
>  }
>  
> +void report_prefix_popn(int n)
> +{
> +	while (n--)
> +		report_prefix_pop();

I think I suggested this implementation, but thinking about it some more
this won't work well with other cpus pushing/popping simultaneously. We
need something like

 static void __report_prefix_pop(void)
 {
        char *p, *q;

        if (!*prefixes)
                return;

        for (p = prefixes, q = strstr(p, PREFIX_DELIMITER) + 2;
                        *q;
                        p = q, q = strstr(p, PREFIX_DELIMITER) + 2)
                ;
        *p = '\0';
 }

 void report_prefix_pop(void)
 {
        spin_lock(&lock);
	__report_prefix_pop();
        spin_unlock(&lock);
 }

 void report_prefix_popn(int n)
 {
        spin_lock(&lock);
        while (n--)
	        __report_prefix_pop();
        spin_unlock(&lock);
 }

> +}
> +
> +void report_prefix_clear(void)
> +{
> +	spin_lock(&lock);
> +	prefixes[0] = '\0';
> +	spin_unlock(&lock);
> +}

I'm also second guessing the utility of this one. We'd probably almost
never want to do this since most tests are designed with a

  main()
  {
     report_prefix_push("mytest");
     subtest1();
     subtest2();
     ...
     report_prefix_pop();
     ...
  }

type pattern and we wouldn't want to lose that "mytest" prefix when some
subtest calls clear. Let's just drop report_prefix_clear() for now.

> +
>  static void va_report(const char *msg_fmt,
>  		bool pass, bool xfail, bool kfail, bool skip, va_list va)
>  {
> -- 
> 2.43.0
>

Thanks,
drew

[kvm-unit-tests PATCH v2 1/4] lib/report: Add helper methods to clear multiple prefixes

[Andrew Jones]

Also, let's separate this patch into its own series where we also apply
it with a second patch to the current riscv/sbi.c in order to remove a
bunch of pops.

Thanks,
drew

On Mon, Aug 26, 2024 at 01:08:21AM GMT, James Raphael Tiovalen wrote:
> Add a method to pop a specified number of prefixes and another method to
> clear all prefixes.
> 
> Suggested-by: Andrew Jones <andrew.jones@linux.dev>
> Signed-off-by: James Raphael Tiovalen <jamestiotio@gmail.com>
> ---
>  lib/libcflat.h |  2 ++
>  lib/report.c   | 13 +++++++++++++
>  2 files changed, 15 insertions(+)
> 
> diff --git a/lib/libcflat.h b/lib/libcflat.h
> index 16a83880..0286ddec 100644
> --- a/lib/libcflat.h
> +++ b/lib/libcflat.h
> @@ -96,6 +96,8 @@ void report_prefix_pushf(const char *prefix_fmt, ...)
>  					__attribute__((format(printf, 1, 2)));
>  extern void report_prefix_push(const char *prefix);
>  extern void report_prefix_pop(void);
> +extern void report_prefix_popn(int n);
> +extern void report_prefix_clear(void);
>  extern void report(bool pass, const char *msg_fmt, ...)
>  		__attribute__((format(printf, 2, 3), nonnull(2)));
>  extern void report_xfail(bool xfail, bool pass, const char *msg_fmt, ...)
> diff --git a/lib/report.c b/lib/report.c
> index 7f3c4f05..d45afedc 100644
> --- a/lib/report.c
> +++ b/lib/report.c
> @@ -80,6 +80,19 @@ void report_prefix_pop(void)
>  	spin_unlock(&lock);
>  }
>  
> +void report_prefix_popn(int n)
> +{
> +	while (n--)
> +		report_prefix_pop();
> +}
> +
> +void report_prefix_clear(void)
> +{
> +	spin_lock(&lock);
> +	prefixes[0] = '\0';
> +	spin_unlock(&lock);
> +}
> +
>  static void va_report(const char *msg_fmt,
>  		bool pass, bool xfail, bool kfail, bool skip, va_list va)
>  {
> -- 
> 2.43.0
> 
> 
> -- 
> kvm-riscv mailing list
> kvm-riscv at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kvm-riscv

Re: [kvm-unit-tests PATCH v2 1/4] lib/report: Add helper methods to clear multiple prefixes

[Andrew Jones]

Also, let's separate this patch into its own series where we also apply
it with a second patch to the current riscv/sbi.c in order to remove a
bunch of pops.

Thanks,
drew

On Mon, Aug 26, 2024 at 01:08:21AM GMT, James Raphael Tiovalen wrote:
> Add a method to pop a specified number of prefixes and another method to
> clear all prefixes.
> 
> Suggested-by: Andrew Jones <andrew.jones@linux.dev>
> Signed-off-by: James Raphael Tiovalen <jamestiotio@gmail.com>
> ---
>  lib/libcflat.h |  2 ++
>  lib/report.c   | 13 +++++++++++++
>  2 files changed, 15 insertions(+)
> 
> diff --git a/lib/libcflat.h b/lib/libcflat.h
> index 16a83880..0286ddec 100644
> --- a/lib/libcflat.h
> +++ b/lib/libcflat.h
> @@ -96,6 +96,8 @@ void report_prefix_pushf(const char *prefix_fmt, ...)
>  					__attribute__((format(printf, 1, 2)));
>  extern void report_prefix_push(const char *prefix);
>  extern void report_prefix_pop(void);
> +extern void report_prefix_popn(int n);
> +extern void report_prefix_clear(void);
>  extern void report(bool pass, const char *msg_fmt, ...)
>  		__attribute__((format(printf, 2, 3), nonnull(2)));
>  extern void report_xfail(bool xfail, bool pass, const char *msg_fmt, ...)
> diff --git a/lib/report.c b/lib/report.c
> index 7f3c4f05..d45afedc 100644
> --- a/lib/report.c
> +++ b/lib/report.c
> @@ -80,6 +80,19 @@ void report_prefix_pop(void)
>  	spin_unlock(&lock);
>  }
>  
> +void report_prefix_popn(int n)
> +{
> +	while (n--)
> +		report_prefix_pop();
> +}
> +
> +void report_prefix_clear(void)
> +{
> +	spin_lock(&lock);
> +	prefixes[0] = '\0';
> +	spin_unlock(&lock);
> +}
> +
>  static void va_report(const char *msg_fmt,
>  		bool pass, bool xfail, bool kfail, bool skip, va_list va)
>  {
> -- 
> 2.43.0
> 
> 
> -- 
> kvm-riscv mailing list
> kvm-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kvm-riscv

[kvm-unit-tests PATCH v2 2/4] riscv: sbi: Add IPI extension support

[James Raphael Tiovalen]

Add IPI EID and FID constants and a helper function to perform the IPI
SBI ecall.

Signed-off-by: James Raphael Tiovalen <jamestiotio@gmail.com>
---
 lib/riscv/asm/sbi.h | 6 ++++++
 lib/riscv/sbi.c     | 5 +++++
 2 files changed, 11 insertions(+)

diff --git a/lib/riscv/asm/sbi.h b/lib/riscv/asm/sbi.h
index 47e91025..a864e268 100644
--- a/lib/riscv/asm/sbi.h
+++ b/lib/riscv/asm/sbi.h
@@ -17,6 +17,7 @@
 enum sbi_ext_id {
 	SBI_EXT_BASE = 0x10,
 	SBI_EXT_TIME = 0x54494d45,
+	SBI_EXT_IPI = 0x735049,
 	SBI_EXT_HSM = 0x48534d,
 	SBI_EXT_SRST = 0x53525354,
 	SBI_EXT_DBCN = 0x4442434E,
@@ -43,6 +44,10 @@ enum sbi_ext_time_fid {
 	SBI_EXT_TIME_SET_TIMER = 0,
 };
 
+enum sbi_ext_ipi_fid {
+	SBI_EXT_IPI_SEND_IPI = 0,
+};
+
 enum sbi_ext_dbcn_fid {
 	SBI_EXT_DBCN_CONSOLE_WRITE = 0,
 	SBI_EXT_DBCN_CONSOLE_READ,
@@ -61,6 +66,7 @@ struct sbiret sbi_ecall(int ext, int fid, unsigned long arg0,
 
 void sbi_shutdown(void);
 struct sbiret sbi_hart_start(unsigned long hartid, unsigned long entry, unsigned long sp);
+struct sbiret sbi_send_ipi(unsigned long hart_mask, unsigned long hart_mask_base);
 long sbi_probe(int ext);
 
 #endif /* !__ASSEMBLY__ */
diff --git a/lib/riscv/sbi.c b/lib/riscv/sbi.c
index 3d4236e5..19d58ab7 100644
--- a/lib/riscv/sbi.c
+++ b/lib/riscv/sbi.c
@@ -39,6 +39,11 @@ struct sbiret sbi_hart_start(unsigned long hartid, unsigned long entry, unsigned
 	return sbi_ecall(SBI_EXT_HSM, SBI_EXT_HSM_HART_START, hartid, entry, sp, 0, 0, 0);
 }
 
+struct sbiret sbi_send_ipi(unsigned long hart_mask, unsigned long hart_mask_base)
+{
+	return sbi_ecall(SBI_EXT_IPI, SBI_EXT_IPI_SEND_IPI, hart_mask, hart_mask_base, 0, 0, 0, 0);
+}
+
 long sbi_probe(int ext)
 {
 	struct sbiret ret;
-- 
2.43.0

[kvm-unit-tests PATCH v2 2/4] riscv: sbi: Add IPI extension support

[James Raphael Tiovalen]

Add IPI EID and FID constants and a helper function to perform the IPI
SBI ecall.

Signed-off-by: James Raphael Tiovalen <jamestiotio@gmail.com>
---
 lib/riscv/asm/sbi.h | 6 ++++++
 lib/riscv/sbi.c     | 5 +++++
 2 files changed, 11 insertions(+)

diff --git a/lib/riscv/asm/sbi.h b/lib/riscv/asm/sbi.h
index 47e91025..a864e268 100644
--- a/lib/riscv/asm/sbi.h
+++ b/lib/riscv/asm/sbi.h
@@ -17,6 +17,7 @@
 enum sbi_ext_id {
 	SBI_EXT_BASE = 0x10,
 	SBI_EXT_TIME = 0x54494d45,
+	SBI_EXT_IPI = 0x735049,
 	SBI_EXT_HSM = 0x48534d,
 	SBI_EXT_SRST = 0x53525354,
 	SBI_EXT_DBCN = 0x4442434E,
@@ -43,6 +44,10 @@ enum sbi_ext_time_fid {
 	SBI_EXT_TIME_SET_TIMER = 0,
 };
 
+enum sbi_ext_ipi_fid {
+	SBI_EXT_IPI_SEND_IPI = 0,
+};
+
 enum sbi_ext_dbcn_fid {
 	SBI_EXT_DBCN_CONSOLE_WRITE = 0,
 	SBI_EXT_DBCN_CONSOLE_READ,
@@ -61,6 +66,7 @@ struct sbiret sbi_ecall(int ext, int fid, unsigned long arg0,
 
 void sbi_shutdown(void);
 struct sbiret sbi_hart_start(unsigned long hartid, unsigned long entry, unsigned long sp);
+struct sbiret sbi_send_ipi(unsigned long hart_mask, unsigned long hart_mask_base);
 long sbi_probe(int ext);
 
 #endif /* !__ASSEMBLY__ */
diff --git a/lib/riscv/sbi.c b/lib/riscv/sbi.c
index 3d4236e5..19d58ab7 100644
--- a/lib/riscv/sbi.c
+++ b/lib/riscv/sbi.c
@@ -39,6 +39,11 @@ struct sbiret sbi_hart_start(unsigned long hartid, unsigned long entry, unsigned
 	return sbi_ecall(SBI_EXT_HSM, SBI_EXT_HSM_HART_START, hartid, entry, sp, 0, 0, 0);
 }
 
+struct sbiret sbi_send_ipi(unsigned long hart_mask, unsigned long hart_mask_base)
+{
+	return sbi_ecall(SBI_EXT_IPI, SBI_EXT_IPI_SEND_IPI, hart_mask, hart_mask_base, 0, 0, 0, 0);
+}
+
 long sbi_probe(int ext)
 {
 	struct sbiret ret;
-- 
2.43.0

[kvm-unit-tests PATCH v2 2/4] riscv: sbi: Add IPI extension support

[Andrew Jones]

On Mon, Aug 26, 2024 at 01:08:22AM GMT, James Raphael Tiovalen wrote:
> Add IPI EID and FID constants and a helper function to perform the IPI
> SBI ecall.
> 
> Signed-off-by: James Raphael Tiovalen <jamestiotio@gmail.com>
> ---
>  lib/riscv/asm/sbi.h | 6 ++++++
>  lib/riscv/sbi.c     | 5 +++++
>  2 files changed, 11 insertions(+)
> 
> diff --git a/lib/riscv/asm/sbi.h b/lib/riscv/asm/sbi.h
> index 47e91025..a864e268 100644
> --- a/lib/riscv/asm/sbi.h
> +++ b/lib/riscv/asm/sbi.h
> @@ -17,6 +17,7 @@
>  enum sbi_ext_id {
>  	SBI_EXT_BASE = 0x10,
>  	SBI_EXT_TIME = 0x54494d45,
> +	SBI_EXT_IPI = 0x735049,
>  	SBI_EXT_HSM = 0x48534d,
>  	SBI_EXT_SRST = 0x53525354,
>  	SBI_EXT_DBCN = 0x4442434E,
> @@ -43,6 +44,10 @@ enum sbi_ext_time_fid {
>  	SBI_EXT_TIME_SET_TIMER = 0,
>  };
>  
> +enum sbi_ext_ipi_fid {
> +	SBI_EXT_IPI_SEND_IPI = 0,
> +};
> +
>  enum sbi_ext_dbcn_fid {
>  	SBI_EXT_DBCN_CONSOLE_WRITE = 0,
>  	SBI_EXT_DBCN_CONSOLE_READ,
> @@ -61,6 +66,7 @@ struct sbiret sbi_ecall(int ext, int fid, unsigned long arg0,
>  
>  void sbi_shutdown(void);
>  struct sbiret sbi_hart_start(unsigned long hartid, unsigned long entry, unsigned long sp);
> +struct sbiret sbi_send_ipi(unsigned long hart_mask, unsigned long hart_mask_base);
>  long sbi_probe(int ext);
>  
>  #endif /* !__ASSEMBLY__ */
> diff --git a/lib/riscv/sbi.c b/lib/riscv/sbi.c
> index 3d4236e5..19d58ab7 100644
> --- a/lib/riscv/sbi.c
> +++ b/lib/riscv/sbi.c
> @@ -39,6 +39,11 @@ struct sbiret sbi_hart_start(unsigned long hartid, unsigned long entry, unsigned
>  	return sbi_ecall(SBI_EXT_HSM, SBI_EXT_HSM_HART_START, hartid, entry, sp, 0, 0, 0);
>  }
>  
> +struct sbiret sbi_send_ipi(unsigned long hart_mask, unsigned long hart_mask_base)
> +{
> +	return sbi_ecall(SBI_EXT_IPI, SBI_EXT_IPI_SEND_IPI, hart_mask, hart_mask_base, 0, 0, 0, 0);
> +}
> +
>  long sbi_probe(int ext)
>  {
>  	struct sbiret ret;
> -- 
> 2.43.0
> 
>

Queued to riscv/sbi, https://gitlab.com/jones-drew/kvm-unit-tests/-/commits/riscv/sbi

Thanks,
drew

Re: [kvm-unit-tests PATCH v2 2/4] riscv: sbi: Add IPI extension support

[Andrew Jones]

On Mon, Aug 26, 2024 at 01:08:22AM GMT, James Raphael Tiovalen wrote:
> Add IPI EID and FID constants and a helper function to perform the IPI
> SBI ecall.
> 
> Signed-off-by: James Raphael Tiovalen <jamestiotio@gmail.com>
> ---
>  lib/riscv/asm/sbi.h | 6 ++++++
>  lib/riscv/sbi.c     | 5 +++++
>  2 files changed, 11 insertions(+)
> 
> diff --git a/lib/riscv/asm/sbi.h b/lib/riscv/asm/sbi.h
> index 47e91025..a864e268 100644
> --- a/lib/riscv/asm/sbi.h
> +++ b/lib/riscv/asm/sbi.h
> @@ -17,6 +17,7 @@
>  enum sbi_ext_id {
>  	SBI_EXT_BASE = 0x10,
>  	SBI_EXT_TIME = 0x54494d45,
> +	SBI_EXT_IPI = 0x735049,
>  	SBI_EXT_HSM = 0x48534d,
>  	SBI_EXT_SRST = 0x53525354,
>  	SBI_EXT_DBCN = 0x4442434E,
> @@ -43,6 +44,10 @@ enum sbi_ext_time_fid {
>  	SBI_EXT_TIME_SET_TIMER = 0,
>  };
>  
> +enum sbi_ext_ipi_fid {
> +	SBI_EXT_IPI_SEND_IPI = 0,
> +};
> +
>  enum sbi_ext_dbcn_fid {
>  	SBI_EXT_DBCN_CONSOLE_WRITE = 0,
>  	SBI_EXT_DBCN_CONSOLE_READ,
> @@ -61,6 +66,7 @@ struct sbiret sbi_ecall(int ext, int fid, unsigned long arg0,
>  
>  void sbi_shutdown(void);
>  struct sbiret sbi_hart_start(unsigned long hartid, unsigned long entry, unsigned long sp);
> +struct sbiret sbi_send_ipi(unsigned long hart_mask, unsigned long hart_mask_base);
>  long sbi_probe(int ext);
>  
>  #endif /* !__ASSEMBLY__ */
> diff --git a/lib/riscv/sbi.c b/lib/riscv/sbi.c
> index 3d4236e5..19d58ab7 100644
> --- a/lib/riscv/sbi.c
> +++ b/lib/riscv/sbi.c
> @@ -39,6 +39,11 @@ struct sbiret sbi_hart_start(unsigned long hartid, unsigned long entry, unsigned
>  	return sbi_ecall(SBI_EXT_HSM, SBI_EXT_HSM_HART_START, hartid, entry, sp, 0, 0, 0);
>  }
>  
> +struct sbiret sbi_send_ipi(unsigned long hart_mask, unsigned long hart_mask_base)
> +{
> +	return sbi_ecall(SBI_EXT_IPI, SBI_EXT_IPI_SEND_IPI, hart_mask, hart_mask_base, 0, 0, 0, 0);
> +}
> +
>  long sbi_probe(int ext)
>  {
>  	struct sbiret ret;
> -- 
> 2.43.0
> 
>

Queued to riscv/sbi, https://gitlab.com/jones-drew/kvm-unit-tests/-/commits/riscv/sbi

Thanks,
drew

[kvm-unit-tests PATCH v2 3/4] riscv: sbi: Add HSM extension functions

[James Raphael Tiovalen]

Add helper functions to perform hart-related operations to prepare for
the HSM tests. Also add the HSM state IDs and default suspend type
constants.

Signed-off-by: James Raphael Tiovalen <jamestiotio@gmail.com>
---
 lib/riscv/asm/sbi.h | 17 +++++++++++++++++
 lib/riscv/sbi.c     | 10 ++++++++++
 riscv/sbi.c         |  5 +++++
 3 files changed, 32 insertions(+)

diff --git a/lib/riscv/asm/sbi.h b/lib/riscv/asm/sbi.h
index a864e268..4e48ceaa 100644
--- a/lib/riscv/asm/sbi.h
+++ b/lib/riscv/asm/sbi.h
@@ -48,6 +48,21 @@ enum sbi_ext_ipi_fid {
 	SBI_EXT_IPI_SEND_IPI = 0,
 };
 
+enum sbi_ext_hsm_sid {
+	SBI_EXT_HSM_STARTED = 0,
+	SBI_EXT_HSM_STOPPED,
+	SBI_EXT_HSM_START_PENDING,
+	SBI_EXT_HSM_STOP_PENDING,
+	SBI_EXT_HSM_SUSPENDED,
+	SBI_EXT_HSM_SUSPEND_PENDING,
+	SBI_EXT_HSM_RESUME_PENDING,
+};
+
+enum sbi_ext_hsm_hart_suspend_type {
+	SBI_EXT_HSM_HART_SUSPEND_RETENTIVE = 0,
+	SBI_EXT_HSM_HART_SUSPEND_NON_RETENTIVE = 0x80000000,
+};
+
 enum sbi_ext_dbcn_fid {
 	SBI_EXT_DBCN_CONSOLE_WRITE = 0,
 	SBI_EXT_DBCN_CONSOLE_READ,
@@ -66,6 +81,8 @@ struct sbiret sbi_ecall(int ext, int fid, unsigned long arg0,
 
 void sbi_shutdown(void);
 struct sbiret sbi_hart_start(unsigned long hartid, unsigned long entry, unsigned long sp);
+struct sbiret sbi_hart_stop(void);
+struct sbiret sbi_hart_get_status(unsigned long hartid);
 struct sbiret sbi_send_ipi(unsigned long hart_mask, unsigned long hart_mask_base);
 long sbi_probe(int ext);
 
diff --git a/lib/riscv/sbi.c b/lib/riscv/sbi.c
index 19d58ab7..256196b7 100644
--- a/lib/riscv/sbi.c
+++ b/lib/riscv/sbi.c
@@ -39,6 +39,16 @@ struct sbiret sbi_hart_start(unsigned long hartid, unsigned long entry, unsigned
 	return sbi_ecall(SBI_EXT_HSM, SBI_EXT_HSM_HART_START, hartid, entry, sp, 0, 0, 0);
 }
 
+struct sbiret sbi_hart_stop(void)
+{
+	return sbi_ecall(SBI_EXT_HSM, SBI_EXT_HSM_HART_STOP, 0, 0, 0, 0, 0, 0);
+}
+
+struct sbiret sbi_hart_get_status(unsigned long hartid)
+{
+	return sbi_ecall(SBI_EXT_HSM, SBI_EXT_HSM_HART_STATUS, hartid, 0, 0, 0, 0, 0);
+}
+
 struct sbiret sbi_send_ipi(unsigned long hart_mask, unsigned long hart_mask_base)
 {
 	return sbi_ecall(SBI_EXT_IPI, SBI_EXT_IPI_SEND_IPI, hart_mask, hart_mask_base, 0, 0, 0, 0);
diff --git a/riscv/sbi.c b/riscv/sbi.c
index 36ddfd48..6469304b 100644
--- a/riscv/sbi.c
+++ b/riscv/sbi.c
@@ -72,6 +72,11 @@ static phys_addr_t get_highest_addr(void)
 	return highest_end - 1;
 }
 
+static struct sbiret sbi_hart_suspend(uint32_t suspend_type, unsigned long resume_addr, unsigned long opaque)
+{
+	return sbi_ecall(SBI_EXT_HSM, SBI_EXT_HSM_HART_SUSPEND, suspend_type, resume_addr, opaque, 0, 0, 0);
+}
+
 static bool env_or_skip(const char *env)
 {
 	if (!getenv(env)) {
-- 
2.43.0

[kvm-unit-tests PATCH v2 3/4] riscv: sbi: Add HSM extension functions

[James Raphael Tiovalen]

Add helper functions to perform hart-related operations to prepare for
the HSM tests. Also add the HSM state IDs and default suspend type
constants.

Signed-off-by: James Raphael Tiovalen <jamestiotio@gmail.com>
---
 lib/riscv/asm/sbi.h | 17 +++++++++++++++++
 lib/riscv/sbi.c     | 10 ++++++++++
 riscv/sbi.c         |  5 +++++
 3 files changed, 32 insertions(+)

diff --git a/lib/riscv/asm/sbi.h b/lib/riscv/asm/sbi.h
index a864e268..4e48ceaa 100644
--- a/lib/riscv/asm/sbi.h
+++ b/lib/riscv/asm/sbi.h
@@ -48,6 +48,21 @@ enum sbi_ext_ipi_fid {
 	SBI_EXT_IPI_SEND_IPI = 0,
 };
 
+enum sbi_ext_hsm_sid {
+	SBI_EXT_HSM_STARTED = 0,
+	SBI_EXT_HSM_STOPPED,
+	SBI_EXT_HSM_START_PENDING,
+	SBI_EXT_HSM_STOP_PENDING,
+	SBI_EXT_HSM_SUSPENDED,
+	SBI_EXT_HSM_SUSPEND_PENDING,
+	SBI_EXT_HSM_RESUME_PENDING,
+};
+
+enum sbi_ext_hsm_hart_suspend_type {
+	SBI_EXT_HSM_HART_SUSPEND_RETENTIVE = 0,
+	SBI_EXT_HSM_HART_SUSPEND_NON_RETENTIVE = 0x80000000,
+};
+
 enum sbi_ext_dbcn_fid {
 	SBI_EXT_DBCN_CONSOLE_WRITE = 0,
 	SBI_EXT_DBCN_CONSOLE_READ,
@@ -66,6 +81,8 @@ struct sbiret sbi_ecall(int ext, int fid, unsigned long arg0,
 
 void sbi_shutdown(void);
 struct sbiret sbi_hart_start(unsigned long hartid, unsigned long entry, unsigned long sp);
+struct sbiret sbi_hart_stop(void);
+struct sbiret sbi_hart_get_status(unsigned long hartid);
 struct sbiret sbi_send_ipi(unsigned long hart_mask, unsigned long hart_mask_base);
 long sbi_probe(int ext);
 
diff --git a/lib/riscv/sbi.c b/lib/riscv/sbi.c
index 19d58ab7..256196b7 100644
--- a/lib/riscv/sbi.c
+++ b/lib/riscv/sbi.c
@@ -39,6 +39,16 @@ struct sbiret sbi_hart_start(unsigned long hartid, unsigned long entry, unsigned
 	return sbi_ecall(SBI_EXT_HSM, SBI_EXT_HSM_HART_START, hartid, entry, sp, 0, 0, 0);
 }
 
+struct sbiret sbi_hart_stop(void)
+{
+	return sbi_ecall(SBI_EXT_HSM, SBI_EXT_HSM_HART_STOP, 0, 0, 0, 0, 0, 0);
+}
+
+struct sbiret sbi_hart_get_status(unsigned long hartid)
+{
+	return sbi_ecall(SBI_EXT_HSM, SBI_EXT_HSM_HART_STATUS, hartid, 0, 0, 0, 0, 0);
+}
+
 struct sbiret sbi_send_ipi(unsigned long hart_mask, unsigned long hart_mask_base)
 {
 	return sbi_ecall(SBI_EXT_IPI, SBI_EXT_IPI_SEND_IPI, hart_mask, hart_mask_base, 0, 0, 0, 0);
diff --git a/riscv/sbi.c b/riscv/sbi.c
index 36ddfd48..6469304b 100644
--- a/riscv/sbi.c
+++ b/riscv/sbi.c
@@ -72,6 +72,11 @@ static phys_addr_t get_highest_addr(void)
 	return highest_end - 1;
 }
 
+static struct sbiret sbi_hart_suspend(uint32_t suspend_type, unsigned long resume_addr, unsigned long opaque)
+{
+	return sbi_ecall(SBI_EXT_HSM, SBI_EXT_HSM_HART_SUSPEND, suspend_type, resume_addr, opaque, 0, 0, 0);
+}
+
 static bool env_or_skip(const char *env)
 {
 	if (!getenv(env)) {
-- 
2.43.0

[kvm-unit-tests PATCH v2 3/4] riscv: sbi: Add HSM extension functions

[Andrew Jones]

On Mon, Aug 26, 2024 at 01:08:23AM GMT, James Raphael Tiovalen wrote:
> Add helper functions to perform hart-related operations to prepare for
> the HSM tests. Also add the HSM state IDs and default suspend type
> constants.
> 
> Signed-off-by: James Raphael Tiovalen <jamestiotio@gmail.com>
> ---
>  lib/riscv/asm/sbi.h | 17 +++++++++++++++++
>  lib/riscv/sbi.c     | 10 ++++++++++
>  riscv/sbi.c         |  5 +++++
>  3 files changed, 32 insertions(+)
> 
> diff --git a/lib/riscv/asm/sbi.h b/lib/riscv/asm/sbi.h
> index a864e268..4e48ceaa 100644
> --- a/lib/riscv/asm/sbi.h
> +++ b/lib/riscv/asm/sbi.h
> @@ -48,6 +48,21 @@ enum sbi_ext_ipi_fid {
>  	SBI_EXT_IPI_SEND_IPI = 0,
>  };
>  
> +enum sbi_ext_hsm_sid {
> +	SBI_EXT_HSM_STARTED = 0,
> +	SBI_EXT_HSM_STOPPED,
> +	SBI_EXT_HSM_START_PENDING,
> +	SBI_EXT_HSM_STOP_PENDING,
> +	SBI_EXT_HSM_SUSPENDED,
> +	SBI_EXT_HSM_SUSPEND_PENDING,
> +	SBI_EXT_HSM_RESUME_PENDING,
> +};
> +
> +enum sbi_ext_hsm_hart_suspend_type {
> +	SBI_EXT_HSM_HART_SUSPEND_RETENTIVE = 0,
> +	SBI_EXT_HSM_HART_SUSPEND_NON_RETENTIVE = 0x80000000,
> +};
> +
>  enum sbi_ext_dbcn_fid {
>  	SBI_EXT_DBCN_CONSOLE_WRITE = 0,
>  	SBI_EXT_DBCN_CONSOLE_READ,
> @@ -66,6 +81,8 @@ struct sbiret sbi_ecall(int ext, int fid, unsigned long arg0,
>  
>  void sbi_shutdown(void);
>  struct sbiret sbi_hart_start(unsigned long hartid, unsigned long entry, unsigned long sp);
> +struct sbiret sbi_hart_stop(void);
> +struct sbiret sbi_hart_get_status(unsigned long hartid);
>  struct sbiret sbi_send_ipi(unsigned long hart_mask, unsigned long hart_mask_base);
>  long sbi_probe(int ext);
>  
> diff --git a/lib/riscv/sbi.c b/lib/riscv/sbi.c
> index 19d58ab7..256196b7 100644
> --- a/lib/riscv/sbi.c
> +++ b/lib/riscv/sbi.c
> @@ -39,6 +39,16 @@ struct sbiret sbi_hart_start(unsigned long hartid, unsigned long entry, unsigned
>  	return sbi_ecall(SBI_EXT_HSM, SBI_EXT_HSM_HART_START, hartid, entry, sp, 0, 0, 0);
>  }
>  
> +struct sbiret sbi_hart_stop(void)
> +{
> +	return sbi_ecall(SBI_EXT_HSM, SBI_EXT_HSM_HART_STOP, 0, 0, 0, 0, 0, 0);
> +}
> +
> +struct sbiret sbi_hart_get_status(unsigned long hartid)
> +{
> +	return sbi_ecall(SBI_EXT_HSM, SBI_EXT_HSM_HART_STATUS, hartid, 0, 0, 0, 0, 0);
> +}
> +
>  struct sbiret sbi_send_ipi(unsigned long hart_mask, unsigned long hart_mask_base)
>  {
>  	return sbi_ecall(SBI_EXT_IPI, SBI_EXT_IPI_SEND_IPI, hart_mask, hart_mask_base, 0, 0, 0, 0);
> diff --git a/riscv/sbi.c b/riscv/sbi.c
> index 36ddfd48..6469304b 100644
> --- a/riscv/sbi.c
> +++ b/riscv/sbi.c
> @@ -72,6 +72,11 @@ static phys_addr_t get_highest_addr(void)
>  	return highest_end - 1;
>  }
>  
> +static struct sbiret sbi_hart_suspend(uint32_t suspend_type, unsigned long resume_addr, unsigned long opaque)
> +{
> +	return sbi_ecall(SBI_EXT_HSM, SBI_EXT_HSM_HART_SUSPEND, suspend_type, resume_addr, opaque, 0, 0, 0);
> +}
> +

Yeah, I think I prefer this naming to the __<ext>_sbi_ecall type of naming
we currently have in this file, particularly because if we decide to
promote an SBI call from the SBI tests to the riscv lib then we don't need
to go rename everything in the test too. I have another minor riscv/sbi.c
cleanup in mind too so I'll write a riscv/sbi cleanup series at some
point.

>  static bool env_or_skip(const char *env)
>  {
>  	if (!getenv(env)) {
> -- 
> 2.43.0
>

Reviewed-by: Andrew Jones <andrew.jones@linux.dev>

Thanks,
drew

Re: [kvm-unit-tests PATCH v2 3/4] riscv: sbi: Add HSM extension functions

[Andrew Jones]

On Mon, Aug 26, 2024 at 01:08:23AM GMT, James Raphael Tiovalen wrote:
> Add helper functions to perform hart-related operations to prepare for
> the HSM tests. Also add the HSM state IDs and default suspend type
> constants.
> 
> Signed-off-by: James Raphael Tiovalen <jamestiotio@gmail.com>
> ---
>  lib/riscv/asm/sbi.h | 17 +++++++++++++++++
>  lib/riscv/sbi.c     | 10 ++++++++++
>  riscv/sbi.c         |  5 +++++
>  3 files changed, 32 insertions(+)
> 
> diff --git a/lib/riscv/asm/sbi.h b/lib/riscv/asm/sbi.h
> index a864e268..4e48ceaa 100644
> --- a/lib/riscv/asm/sbi.h
> +++ b/lib/riscv/asm/sbi.h
> @@ -48,6 +48,21 @@ enum sbi_ext_ipi_fid {
>  	SBI_EXT_IPI_SEND_IPI = 0,
>  };
>  
> +enum sbi_ext_hsm_sid {
> +	SBI_EXT_HSM_STARTED = 0,
> +	SBI_EXT_HSM_STOPPED,
> +	SBI_EXT_HSM_START_PENDING,
> +	SBI_EXT_HSM_STOP_PENDING,
> +	SBI_EXT_HSM_SUSPENDED,
> +	SBI_EXT_HSM_SUSPEND_PENDING,
> +	SBI_EXT_HSM_RESUME_PENDING,
> +};
> +
> +enum sbi_ext_hsm_hart_suspend_type {
> +	SBI_EXT_HSM_HART_SUSPEND_RETENTIVE = 0,
> +	SBI_EXT_HSM_HART_SUSPEND_NON_RETENTIVE = 0x80000000,
> +};
> +
>  enum sbi_ext_dbcn_fid {
>  	SBI_EXT_DBCN_CONSOLE_WRITE = 0,
>  	SBI_EXT_DBCN_CONSOLE_READ,
> @@ -66,6 +81,8 @@ struct sbiret sbi_ecall(int ext, int fid, unsigned long arg0,
>  
>  void sbi_shutdown(void);
>  struct sbiret sbi_hart_start(unsigned long hartid, unsigned long entry, unsigned long sp);
> +struct sbiret sbi_hart_stop(void);
> +struct sbiret sbi_hart_get_status(unsigned long hartid);
>  struct sbiret sbi_send_ipi(unsigned long hart_mask, unsigned long hart_mask_base);
>  long sbi_probe(int ext);
>  
> diff --git a/lib/riscv/sbi.c b/lib/riscv/sbi.c
> index 19d58ab7..256196b7 100644
> --- a/lib/riscv/sbi.c
> +++ b/lib/riscv/sbi.c
> @@ -39,6 +39,16 @@ struct sbiret sbi_hart_start(unsigned long hartid, unsigned long entry, unsigned
>  	return sbi_ecall(SBI_EXT_HSM, SBI_EXT_HSM_HART_START, hartid, entry, sp, 0, 0, 0);
>  }
>  
> +struct sbiret sbi_hart_stop(void)
> +{
> +	return sbi_ecall(SBI_EXT_HSM, SBI_EXT_HSM_HART_STOP, 0, 0, 0, 0, 0, 0);
> +}
> +
> +struct sbiret sbi_hart_get_status(unsigned long hartid)
> +{
> +	return sbi_ecall(SBI_EXT_HSM, SBI_EXT_HSM_HART_STATUS, hartid, 0, 0, 0, 0, 0);
> +}
> +
>  struct sbiret sbi_send_ipi(unsigned long hart_mask, unsigned long hart_mask_base)
>  {
>  	return sbi_ecall(SBI_EXT_IPI, SBI_EXT_IPI_SEND_IPI, hart_mask, hart_mask_base, 0, 0, 0, 0);
> diff --git a/riscv/sbi.c b/riscv/sbi.c
> index 36ddfd48..6469304b 100644
> --- a/riscv/sbi.c
> +++ b/riscv/sbi.c
> @@ -72,6 +72,11 @@ static phys_addr_t get_highest_addr(void)
>  	return highest_end - 1;
>  }
>  
> +static struct sbiret sbi_hart_suspend(uint32_t suspend_type, unsigned long resume_addr, unsigned long opaque)
> +{
> +	return sbi_ecall(SBI_EXT_HSM, SBI_EXT_HSM_HART_SUSPEND, suspend_type, resume_addr, opaque, 0, 0, 0);
> +}
> +

Yeah, I think I prefer this naming to the __<ext>_sbi_ecall type of naming
we currently have in this file, particularly because if we decide to
promote an SBI call from the SBI tests to the riscv lib then we don't need
to go rename everything in the test too. I have another minor riscv/sbi.c
cleanup in mind too so I'll write a riscv/sbi cleanup series at some
point.

>  static bool env_or_skip(const char *env)
>  {
>  	if (!getenv(env)) {
> -- 
> 2.43.0
>

Reviewed-by: Andrew Jones <andrew.jones@linux.dev>

Thanks,
drew

[kvm-unit-tests PATCH v2 4/4] riscv: sbi: Add tests for HSM extension

[James Raphael Tiovalen]

Add some tests for all of the HSM extension functions. These tests
ensure that the HSM extension functions follow the behavior as described
in the SBI specification.

Signed-off-by: James Raphael Tiovalen <jamestiotio@gmail.com>
---
 riscv/Makefile  |   7 +-
 riscv/sbi-asm.S |  79 ++++++++++
 riscv/sbi.c     | 382 ++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 465 insertions(+), 3 deletions(-)
 create mode 100644 riscv/sbi-asm.S

diff --git a/riscv/Makefile b/riscv/Makefile
index 179a373d..548041ad 100644
--- a/riscv/Makefile
+++ b/riscv/Makefile
@@ -43,6 +43,7 @@ cflatobjs += lib/riscv/timer.o
 ifeq ($(ARCH),riscv32)
 cflatobjs += lib/ldiv32.o
 endif
+cflatobjs += riscv/sbi-asm.o
 
 ########################################
 
@@ -99,7 +100,7 @@ cflatobjs += lib/efi.o
 .PRECIOUS: %.so
 
 %.so: EFI_LDFLAGS += -defsym=EFI_SUBSYSTEM=0xa --no-undefined
-%.so: %.o $(FLATLIBS) $(SRCDIR)/riscv/efi/elf_riscv64_efi.lds $(cstart.o) %.aux.o
+%.so: %.o $(FLATLIBS) $(SRCDIR)/riscv/efi/elf_riscv64_efi.lds $(cstart.o) $(sbi-asm.o) %.aux.o
 	$(LD) $(EFI_LDFLAGS) -o $@ -T $(SRCDIR)/riscv/efi/elf_riscv64_efi.lds \
 		$(filter %.o, $^) $(FLATLIBS) $(EFI_LIBS)
 
@@ -115,7 +116,7 @@ cflatobjs += lib/efi.o
 		-O binary $^ $@
 else
 %.elf: LDFLAGS += -pie -n -z notext
-%.elf: %.o $(FLATLIBS) $(SRCDIR)/riscv/flat.lds $(cstart.o) %.aux.o
+%.elf: %.o $(FLATLIBS) $(SRCDIR)/riscv/flat.lds $(cstart.o) $(sbi-asm.o) %.aux.o
 	$(LD) $(LDFLAGS) -o $@ -T $(SRCDIR)/riscv/flat.lds \
 		$(filter %.o, $^) $(FLATLIBS)
 	@chmod a-x $@
@@ -127,7 +128,7 @@ else
 endif
 
 generated-files = $(asm-offsets)
-$(tests:.$(exe)=.o) $(cstart.o) $(cflatobjs): $(generated-files)
+$(tests:.$(exe)=.o) $(cstart.o) $(sbi-asm.o) $(cflatobjs): $(generated-files)
 
 arch_clean: asm_offsets_clean
 	$(RM) $(TEST_DIR)/*.{o,flat,elf,so,efi,debug} \
diff --git a/riscv/sbi-asm.S b/riscv/sbi-asm.S
new file mode 100644
index 00000000..f31bc096
--- /dev/null
+++ b/riscv/sbi-asm.S
@@ -0,0 +1,79 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Helper assembly code routines for RISC-V SBI extension tests.
+ *
+ * Copyright (C) 2024, James Raphael Tiovalen <jamestiotio@gmail.com>
+ */
+#define __ASSEMBLY__
+#include <config.h>
+#include <asm/csr.h>
+#include <asm/page.h>
+
+#define SBI_HSM_TEST_DONE       (1 << 0)
+#define SBI_HSM_TEST_SATP       (1 << 1)
+#define SBI_HSM_TEST_SIE        (1 << 2)
+#define SBI_HSM_TEST_HARTID_A1  (1 << 3)
+
+.section .text
+.balign 4
+.global sbi_hsm_check_hart_start
+sbi_hsm_check_hart_start:
+	li	t0, 0
+	csrr	t1, CSR_SATP
+	bnez	t1, 1f
+	li	t0, SBI_HSM_TEST_SATP
+1:	csrr	t1, CSR_SSTATUS
+	andi	t1, t1, SR_SIE
+	bnez	t1, 2f
+	ori	t0, t0, SBI_HSM_TEST_SIE
+2:	bne	a0, a1, 3f
+	ori	t0, t0, SBI_HSM_TEST_HARTID_A1
+3:	ori	t0, t0, SBI_HSM_TEST_DONE
+	la	t1, sbi_hsm_hart_start_checks
+	add	t1, t1, a0
+	sb	t0, 0(t1)
+4:	la	t0, sbi_hsm_stop_hart
+	add	t0, t0, a0
+	lb	t0, 0(t0)
+	pause
+	beqz	t0, 4b
+	li	a7, 0x48534d	/* SBI_EXT_HSM */
+	li	a6, 1		/* SBI_EXT_HSM_HART_STOP */
+	ecall
+	j	halt
+
+.balign 4
+.global sbi_hsm_check_non_retentive_suspend
+sbi_hsm_check_non_retentive_suspend:
+	li	t0, 0
+	csrr	t1, CSR_SATP
+	bnez	t1, 1f
+	li	t0, SBI_HSM_TEST_SATP
+1:	csrr	t1, CSR_SSTATUS
+	andi	t1, t1, SR_SIE
+	bnez	t1, 2f
+	ori	t0, t0, SBI_HSM_TEST_SIE
+2:	bne	a0, a1, 3f
+	ori	t0, t0, SBI_HSM_TEST_HARTID_A1
+3:	ori	t0, t0, SBI_HSM_TEST_DONE
+	la	t1, sbi_hsm_non_retentive_hart_suspend_checks
+	add	t1, t1, a0
+	sb	t0, 0(t1)
+4:	la	t0, sbi_hsm_stop_hart
+	add	t0, t0, a0
+	lb	t0, 0(t0)
+	pause
+	beqz	t0, 4b
+	li	a7, 0x48534d	/* SBI_EXT_HSM */
+	li	a6, 1		/* SBI_EXT_HSM_HART_STOP */
+	ecall
+	j	halt
+
+.section .data
+.balign PAGE_SIZE
+.global sbi_hsm_hart_start_checks
+sbi_hsm_hart_start_checks:			.space CONFIG_NR_CPUS
+.global sbi_hsm_non_retentive_hart_suspend_checks
+sbi_hsm_non_retentive_hart_suspend_checks:	.space CONFIG_NR_CPUS
+.global sbi_hsm_stop_hart
+sbi_hsm_stop_hart:				.space CONFIG_NR_CPUS
diff --git a/riscv/sbi.c b/riscv/sbi.c
index 6469304b..25fc2e81 100644
--- a/riscv/sbi.c
+++ b/riscv/sbi.c
@@ -6,6 +6,8 @@
  */
 #include <libcflat.h>
 #include <alloc_page.h>
+#include <cpumask.h>
+#include <on-cpus.h>
 #include <stdlib.h>
 #include <string.h>
 #include <limits.h>
@@ -17,8 +19,10 @@
 #include <asm/io.h>
 #include <asm/isa.h>
 #include <asm/mmu.h>
+#include <asm/page.h>
 #include <asm/processor.h>
 #include <asm/sbi.h>
+#include <asm/setup.h>
 #include <asm/smp.h>
 #include <asm/timer.h>
 
@@ -425,6 +429,383 @@ static void check_dbcn(void)
 	report_prefix_pop();
 }
 
+#define SBI_HSM_TEST_DONE       (1 << 0)
+#define SBI_HSM_TEST_SATP       (1 << 1)
+#define SBI_HSM_TEST_SIE        (1 << 2)
+#define SBI_HSM_TEST_HARTID_A1  (1 << 3)
+
+static cpumask_t cpus_alive_after_start;
+static cpumask_t cpus_alive_after_retentive_suspend;
+extern void sbi_hsm_check_hart_start(void);
+extern void sbi_hsm_check_non_retentive_suspend(void);
+extern unsigned char sbi_hsm_stop_hart[NR_CPUS];
+extern unsigned char sbi_hsm_hart_start_checks[NR_CPUS];
+extern unsigned char sbi_hsm_non_retentive_hart_suspend_checks[NR_CPUS];
+static void on_secondary_cpus_async(void (*func)(void *data), void *data)
+{
+	int cpu, me = smp_processor_id();
+
+	for_each_present_cpu(cpu) {
+		if (cpu == me)
+			continue;
+		on_cpu_async(cpu, func, data);
+	}
+}
+
+static bool cpumask_test_secondary_cpus(cpumask_t *mask)
+{
+	int cpu, me = smp_processor_id();
+
+	for_each_present_cpu(cpu) {
+		if (cpu == me)
+			continue;
+
+		if (!cpumask_test_cpu(cpu, mask))
+			return false;
+	}
+
+	return true;
+}
+
+static void hart_execute(void *data)
+{
+	int me = smp_processor_id();
+
+	cpumask_set_cpu(me, &cpus_alive_after_start);
+}
+
+static void hart_retentive_suspend(void *data)
+{
+	int me = smp_processor_id();
+	unsigned long hartid = current_thread_info()->hartid;
+	struct sbiret ret = sbi_hart_suspend(SBI_EXT_HSM_HART_SUSPEND_RETENTIVE, __pa(NULL), __pa(NULL));
+
+	if (ret.error)
+		report_fail("failed to retentive suspend hart %ld", hartid);
+	else
+		cpumask_set_cpu(me, &cpus_alive_after_retentive_suspend);
+}
+
+static void hart_non_retentive_suspend(void *data)
+{
+	unsigned long hartid = current_thread_info()->hartid;
+
+	struct sbiret ret = sbi_hart_suspend(SBI_EXT_HSM_HART_SUSPEND_NON_RETENTIVE,
+					     virt_to_phys(&sbi_hsm_check_non_retentive_suspend), hartid);
+
+	if (ret.error)
+		report_fail("failed to non-retentive suspend hart %ld", hartid);
+}
+
+static void hart_wait_on_status(unsigned long hartid, enum sbi_ext_hsm_sid status)
+{
+	struct sbiret ret = sbi_hart_get_status(hartid);
+
+	while (!ret.error && ret.value == status)
+		ret = sbi_hart_get_status(hartid);
+
+	if (ret.error)
+		report_fail("got %ld while waiting on status %u for hart %lx\n", ret.error, status, hartid);
+}
+
+static void check_hsm(void)
+{
+	struct sbiret ret;
+	unsigned long hartid;
+	unsigned char per_hart_start_checks, per_hart_non_retentive_suspend_checks;
+	unsigned long hart_mask[NR_CPUS / BITS_PER_LONG] = {0};
+	bool ipi_failed = false;
+	int cpu, me = smp_processor_id();
+
+	report_prefix_push("hsm");
+
+	if (!sbi_probe(SBI_EXT_HSM)) {
+		report_skip("hsm extension not available");
+		report_prefix_pop();
+		return;
+	}
+
+	report_prefix_push("hart_get_status");
+
+	hartid = current_thread_info()->hartid;
+	ret = sbi_hart_get_status(hartid);
+
+	if (ret.error == SBI_ERR_INVALID_PARAM) {
+		report_fail("current hartid is invalid");
+		report_prefix_popn(2);
+		return;
+	} else if (ret.value != SBI_EXT_HSM_STARTED) {
+		report_fail("current hart is not started");
+		report_prefix_popn(2);
+		return;
+	}
+
+	report_pass("status of current hart is started");
+
+	report_prefix_pop();
+
+	report_prefix_push("hart_start");
+
+	ret = sbi_hart_start(hartid, virt_to_phys(&hart_execute), __pa(NULL));
+	report(ret.error == SBI_ERR_ALREADY_AVAILABLE, "boot hart is already started");
+
+	ret = sbi_hart_start(ULONG_MAX, virt_to_phys(&hart_execute), __pa(NULL));
+	report(ret.error == SBI_ERR_INVALID_PARAM, "invalid hartid check");
+
+	if (nr_cpus < 2) {
+		report_skip("no other cpus to run the remaining hsm tests on");
+		report_prefix_popn(2);
+		return;
+	}
+
+	for_each_present_cpu(cpu) {
+		if (cpu == me)
+			continue;
+
+		hartid = cpus[cpu].hartid;
+		ret = sbi_hart_start(hartid, virt_to_phys(&sbi_hsm_check_hart_start), hartid);
+
+		if (ret.error) {
+			report_fail("failed to start test hart %ld", hartid);
+			report_prefix_popn(2);
+			return;
+		}
+	}
+
+	for_each_present_cpu(cpu) {
+		if (cpu == me)
+			continue;
+
+		hartid = cpus[cpu].hartid;
+
+		hart_wait_on_status(hartid, SBI_EXT_HSM_STOPPED);
+		hart_wait_on_status(hartid, SBI_EXT_HSM_START_PENDING);
+		ret = sbi_hart_get_status(hartid);
+		report(!ret.error && ret.value == SBI_EXT_HSM_STARTED,
+		       "test hart with hartid %ld successfully started", hartid);
+	}
+
+	for_each_present_cpu(cpu) {
+		if (cpu == me)
+			continue;
+
+		hartid = cpus[cpu].hartid;
+
+		while (!((per_hart_start_checks = READ_ONCE(sbi_hsm_hart_start_checks[hartid]))
+			 & SBI_HSM_TEST_DONE))
+			cpu_relax();
+
+		report(per_hart_start_checks & SBI_HSM_TEST_SATP,
+		       "satp is zero for test hart %ld", hartid);
+		report(per_hart_start_checks & SBI_HSM_TEST_SIE,
+		       "sstatus.SIE is zero for test hart %ld", hartid);
+		report(per_hart_start_checks & SBI_HSM_TEST_HARTID_A1,
+		       "a0 and a1 are hartid for test hart %ld", hartid);
+	}
+
+	report_prefix_pop();
+
+	report_prefix_push("hart_stop");
+
+	for_each_present_cpu(cpu) {
+		if (cpu == me)
+			continue;
+
+		hartid = cpus[cpu].hartid;
+		WRITE_ONCE(sbi_hsm_stop_hart[hartid], true);
+	}
+
+	for_each_present_cpu(cpu) {
+		if (cpu == me)
+			continue;
+
+		hartid = cpus[cpu].hartid;
+		hart_wait_on_status(hartid, SBI_EXT_HSM_STARTED);
+		hart_wait_on_status(hartid, SBI_EXT_HSM_STOP_PENDING);
+		ret = sbi_hart_get_status(hartid);
+		report(!ret.error && (ret.value == SBI_EXT_HSM_STOPPED),
+		       "test hart %ld successfully stopped", hartid);
+	}
+
+	/* Reset the stop flags so that we can reuse them after suspension tests */
+	for_each_present_cpu(cpu) {
+		if (cpu == me)
+			continue;
+
+		hartid = cpus[cpu].hartid;
+		WRITE_ONCE(sbi_hsm_stop_hart[hartid], false);
+	}
+
+	report_prefix_pop();
+
+	report_prefix_push("hart_start");
+
+	on_secondary_cpus_async(hart_execute, NULL);
+
+	for_each_present_cpu(cpu) {
+		if (cpu == me)
+			continue;
+
+		hartid = cpus[cpu].hartid;
+		hart_wait_on_status(hartid, SBI_EXT_HSM_STOPPED);
+		hart_wait_on_status(hartid, SBI_EXT_HSM_START_PENDING);
+		ret = sbi_hart_get_status(hartid);
+		report(!ret.error && (ret.value == SBI_EXT_HSM_STARTED),
+		       "new hart with hartid %ld successfully started", hartid);
+	}
+
+	while (!cpumask_test_secondary_cpus(&cpu_idle_mask))
+		cpu_relax();
+
+	report(cpumask_full(&cpu_online_mask), "all cpus online");
+	report(cpumask_test_secondary_cpus(&cpus_alive_after_start),
+	       "all secondary harts successfully executed code after start");
+
+	report_prefix_pop();
+
+	report_prefix_push("hart_suspend");
+
+	if (sbi_probe(SBI_EXT_IPI)) {
+		on_secondary_cpus_async(hart_retentive_suspend, NULL);
+
+		for_each_present_cpu(cpu) {
+			if (cpu == me)
+				continue;
+
+			hartid = cpus[cpu].hartid;
+			hart_mask[hartid / BITS_PER_LONG] |= 1UL << hartid;
+			hart_wait_on_status(hartid, SBI_EXT_HSM_STARTED);
+			hart_wait_on_status(hartid, SBI_EXT_HSM_SUSPEND_PENDING);
+			ret = sbi_hart_get_status(hartid);
+			report(!ret.error && (ret.value == SBI_EXT_HSM_SUSPENDED),
+			       "hart %ld successfully retentive suspended", hartid);
+		}
+
+		for (int i = 0; i < NR_CPUS / BITS_PER_LONG; ++i) {
+			if (hart_mask[i]) {
+				ret = sbi_send_ipi(hart_mask[i], i * BITS_PER_LONG);
+				if (ret.error) {
+					ipi_failed = true;
+					report_fail("got %ld when sending ipi to retentive suspended harts",
+						    ret.error);
+					break;
+				}
+			}
+		}
+
+		if (!ipi_failed) {
+			for_each_present_cpu(cpu) {
+				if (cpu == me)
+					continue;
+
+				hartid = cpus[cpu].hartid;
+				hart_wait_on_status(hartid, SBI_EXT_HSM_SUSPENDED);
+				hart_wait_on_status(hartid, SBI_EXT_HSM_RESUME_PENDING);
+				ret = sbi_hart_get_status(hartid);
+				report(!ret.error && (ret.value == SBI_EXT_HSM_STARTED),
+				       "hart %ld successfully retentive resumed", hartid);
+			}
+
+			while (!cpumask_test_secondary_cpus(&cpu_idle_mask))
+				cpu_relax();
+
+			report(cpumask_full(&cpu_online_mask), "all cpus online");
+			report(cpumask_test_secondary_cpus(&cpus_alive_after_retentive_suspend),
+			       "all secondary harts successfully executed code after retentive suspend");
+		}
+
+		/* Reset the ipi_failed flag so that we can reuse it for non-retentive suspension tests */
+		ipi_failed = false;
+
+		on_secondary_cpus_async(hart_non_retentive_suspend, NULL);
+
+		for_each_present_cpu(cpu) {
+			if (cpu == me)
+				continue;
+
+			hartid = cpus[cpu].hartid;
+			hart_wait_on_status(hartid, SBI_EXT_HSM_STARTED);
+			hart_wait_on_status(hartid, SBI_EXT_HSM_SUSPEND_PENDING);
+			ret = sbi_hart_get_status(hartid);
+			report(!ret.error && (ret.value == SBI_EXT_HSM_SUSPENDED),
+			       "hart %ld successfully non-retentive suspended", hartid);
+		}
+
+		for (int i = 0; i < NR_CPUS / BITS_PER_LONG; ++i) {
+			if (hart_mask[i]) {
+				ret = sbi_send_ipi(hart_mask[i], i * BITS_PER_LONG);
+				if (ret.error) {
+					ipi_failed = true;
+					report_fail("got %ld when sending ipi to non-retentive suspended harts",
+						    ret.error);
+					break;
+				}
+			}
+		}
+
+		if (!ipi_failed) {
+			for_each_present_cpu(cpu) {
+				if (cpu == me)
+					continue;
+
+				hartid = cpus[cpu].hartid;
+				hart_wait_on_status(hartid, SBI_EXT_HSM_SUSPENDED);
+				hart_wait_on_status(hartid, SBI_EXT_HSM_RESUME_PENDING);
+				ret = sbi_hart_get_status(hartid);
+				report(!ret.error && (ret.value == SBI_EXT_HSM_STARTED),
+				       "hart %ld successfully non-retentive resumed", hartid);
+			}
+
+			for_each_present_cpu(cpu) {
+				if (cpu == me)
+					continue;
+
+				hartid = cpus[cpu].hartid;
+
+				while (!((per_hart_non_retentive_suspend_checks =
+					 READ_ONCE(sbi_hsm_non_retentive_hart_suspend_checks[hartid]))
+					 & SBI_HSM_TEST_DONE))
+					cpu_relax();
+
+				report(per_hart_non_retentive_suspend_checks & SBI_HSM_TEST_SATP,
+				       "satp is zero for test hart %ld", hartid);
+				report(per_hart_non_retentive_suspend_checks & SBI_HSM_TEST_SIE,
+				       "sstatus.SIE is zero for test hart %ld", hartid);
+				report(per_hart_non_retentive_suspend_checks & SBI_HSM_TEST_HARTID_A1,
+				       "a0 and a1 are hartid for test hart %ld", hartid);
+			}
+
+			report_prefix_pop();
+
+			report_prefix_push("hart_stop");
+
+			for_each_present_cpu(cpu) {
+				if (cpu == me)
+					continue;
+
+				hartid = cpus[cpu].hartid;
+				WRITE_ONCE(sbi_hsm_stop_hart[hartid], true);
+			}
+
+			for_each_present_cpu(cpu) {
+				if (cpu == me)
+					continue;
+
+				hartid = cpus[cpu].hartid;
+				hart_wait_on_status(hartid, SBI_EXT_HSM_STARTED);
+				hart_wait_on_status(hartid, SBI_EXT_HSM_STOP_PENDING);
+				ret = sbi_hart_get_status(hartid);
+				report(!ret.error && (ret.value == SBI_EXT_HSM_STOPPED),
+				       "test hart %ld successfully stopped", hartid);
+			}
+		}
+	} else {
+		report_skip("skipping suspension tests since ipi extension is unavailable");
+	}
+
+	report_prefix_popn(2);
+}
+
 int main(int argc, char **argv)
 {
 	if (argc > 1 && !strcmp(argv[1], "-h")) {
@@ -435,6 +816,7 @@ int main(int argc, char **argv)
 	report_prefix_push("sbi");
 	check_base();
 	check_time();
+	check_hsm();
 	check_dbcn();
 
 	return report_summary();
-- 
2.43.0

[kvm-unit-tests PATCH v2 4/4] riscv: sbi: Add tests for HSM extension

[James Raphael Tiovalen]

Add some tests for all of the HSM extension functions. These tests
ensure that the HSM extension functions follow the behavior as described
in the SBI specification.

Signed-off-by: James Raphael Tiovalen <jamestiotio@gmail.com>
---
 riscv/Makefile  |   7 +-
 riscv/sbi-asm.S |  79 ++++++++++
 riscv/sbi.c     | 382 ++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 465 insertions(+), 3 deletions(-)
 create mode 100644 riscv/sbi-asm.S

diff --git a/riscv/Makefile b/riscv/Makefile
index 179a373d..548041ad 100644
--- a/riscv/Makefile
+++ b/riscv/Makefile
@@ -43,6 +43,7 @@ cflatobjs += lib/riscv/timer.o
 ifeq ($(ARCH),riscv32)
 cflatobjs += lib/ldiv32.o
 endif
+cflatobjs += riscv/sbi-asm.o
 
 ########################################
 
@@ -99,7 +100,7 @@ cflatobjs += lib/efi.o
 .PRECIOUS: %.so
 
 %.so: EFI_LDFLAGS += -defsym=EFI_SUBSYSTEM=0xa --no-undefined
-%.so: %.o $(FLATLIBS) $(SRCDIR)/riscv/efi/elf_riscv64_efi.lds $(cstart.o) %.aux.o
+%.so: %.o $(FLATLIBS) $(SRCDIR)/riscv/efi/elf_riscv64_efi.lds $(cstart.o) $(sbi-asm.o) %.aux.o
 	$(LD) $(EFI_LDFLAGS) -o $@ -T $(SRCDIR)/riscv/efi/elf_riscv64_efi.lds \
 		$(filter %.o, $^) $(FLATLIBS) $(EFI_LIBS)
 
@@ -115,7 +116,7 @@ cflatobjs += lib/efi.o
 		-O binary $^ $@
 else
 %.elf: LDFLAGS += -pie -n -z notext
-%.elf: %.o $(FLATLIBS) $(SRCDIR)/riscv/flat.lds $(cstart.o) %.aux.o
+%.elf: %.o $(FLATLIBS) $(SRCDIR)/riscv/flat.lds $(cstart.o) $(sbi-asm.o) %.aux.o
 	$(LD) $(LDFLAGS) -o $@ -T $(SRCDIR)/riscv/flat.lds \
 		$(filter %.o, $^) $(FLATLIBS)
 	@chmod a-x $@
@@ -127,7 +128,7 @@ else
 endif
 
 generated-files = $(asm-offsets)
-$(tests:.$(exe)=.o) $(cstart.o) $(cflatobjs): $(generated-files)
+$(tests:.$(exe)=.o) $(cstart.o) $(sbi-asm.o) $(cflatobjs): $(generated-files)
 
 arch_clean: asm_offsets_clean
 	$(RM) $(TEST_DIR)/*.{o,flat,elf,so,efi,debug} \
diff --git a/riscv/sbi-asm.S b/riscv/sbi-asm.S
new file mode 100644
index 00000000..f31bc096
--- /dev/null
+++ b/riscv/sbi-asm.S
@@ -0,0 +1,79 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Helper assembly code routines for RISC-V SBI extension tests.
+ *
+ * Copyright (C) 2024, James Raphael Tiovalen <jamestiotio@gmail.com>
+ */
+#define __ASSEMBLY__
+#include <config.h>
+#include <asm/csr.h>
+#include <asm/page.h>
+
+#define SBI_HSM_TEST_DONE       (1 << 0)
+#define SBI_HSM_TEST_SATP       (1 << 1)
+#define SBI_HSM_TEST_SIE        (1 << 2)
+#define SBI_HSM_TEST_HARTID_A1  (1 << 3)
+
+.section .text
+.balign 4
+.global sbi_hsm_check_hart_start
+sbi_hsm_check_hart_start:
+	li	t0, 0
+	csrr	t1, CSR_SATP
+	bnez	t1, 1f
+	li	t0, SBI_HSM_TEST_SATP
+1:	csrr	t1, CSR_SSTATUS
+	andi	t1, t1, SR_SIE
+	bnez	t1, 2f
+	ori	t0, t0, SBI_HSM_TEST_SIE
+2:	bne	a0, a1, 3f
+	ori	t0, t0, SBI_HSM_TEST_HARTID_A1
+3:	ori	t0, t0, SBI_HSM_TEST_DONE
+	la	t1, sbi_hsm_hart_start_checks
+	add	t1, t1, a0
+	sb	t0, 0(t1)
+4:	la	t0, sbi_hsm_stop_hart
+	add	t0, t0, a0
+	lb	t0, 0(t0)
+	pause
+	beqz	t0, 4b
+	li	a7, 0x48534d	/* SBI_EXT_HSM */
+	li	a6, 1		/* SBI_EXT_HSM_HART_STOP */
+	ecall
+	j	halt
+
+.balign 4
+.global sbi_hsm_check_non_retentive_suspend
+sbi_hsm_check_non_retentive_suspend:
+	li	t0, 0
+	csrr	t1, CSR_SATP
+	bnez	t1, 1f
+	li	t0, SBI_HSM_TEST_SATP
+1:	csrr	t1, CSR_SSTATUS
+	andi	t1, t1, SR_SIE
+	bnez	t1, 2f
+	ori	t0, t0, SBI_HSM_TEST_SIE
+2:	bne	a0, a1, 3f
+	ori	t0, t0, SBI_HSM_TEST_HARTID_A1
+3:	ori	t0, t0, SBI_HSM_TEST_DONE
+	la	t1, sbi_hsm_non_retentive_hart_suspend_checks
+	add	t1, t1, a0
+	sb	t0, 0(t1)
+4:	la	t0, sbi_hsm_stop_hart
+	add	t0, t0, a0
+	lb	t0, 0(t0)
+	pause
+	beqz	t0, 4b
+	li	a7, 0x48534d	/* SBI_EXT_HSM */
+	li	a6, 1		/* SBI_EXT_HSM_HART_STOP */
+	ecall
+	j	halt
+
+.section .data
+.balign PAGE_SIZE
+.global sbi_hsm_hart_start_checks
+sbi_hsm_hart_start_checks:			.space CONFIG_NR_CPUS
+.global sbi_hsm_non_retentive_hart_suspend_checks
+sbi_hsm_non_retentive_hart_suspend_checks:	.space CONFIG_NR_CPUS
+.global sbi_hsm_stop_hart
+sbi_hsm_stop_hart:				.space CONFIG_NR_CPUS
diff --git a/riscv/sbi.c b/riscv/sbi.c
index 6469304b..25fc2e81 100644
--- a/riscv/sbi.c
+++ b/riscv/sbi.c
@@ -6,6 +6,8 @@
  */
 #include <libcflat.h>
 #include <alloc_page.h>
+#include <cpumask.h>
+#include <on-cpus.h>
 #include <stdlib.h>
 #include <string.h>
 #include <limits.h>
@@ -17,8 +19,10 @@
 #include <asm/io.h>
 #include <asm/isa.h>
 #include <asm/mmu.h>
+#include <asm/page.h>
 #include <asm/processor.h>
 #include <asm/sbi.h>
+#include <asm/setup.h>
 #include <asm/smp.h>
 #include <asm/timer.h>
 
@@ -425,6 +429,383 @@ static void check_dbcn(void)
 	report_prefix_pop();
 }
 
+#define SBI_HSM_TEST_DONE       (1 << 0)
+#define SBI_HSM_TEST_SATP       (1 << 1)
+#define SBI_HSM_TEST_SIE        (1 << 2)
+#define SBI_HSM_TEST_HARTID_A1  (1 << 3)
+
+static cpumask_t cpus_alive_after_start;
+static cpumask_t cpus_alive_after_retentive_suspend;
+extern void sbi_hsm_check_hart_start(void);
+extern void sbi_hsm_check_non_retentive_suspend(void);
+extern unsigned char sbi_hsm_stop_hart[NR_CPUS];
+extern unsigned char sbi_hsm_hart_start_checks[NR_CPUS];
+extern unsigned char sbi_hsm_non_retentive_hart_suspend_checks[NR_CPUS];
+static void on_secondary_cpus_async(void (*func)(void *data), void *data)
+{
+	int cpu, me = smp_processor_id();
+
+	for_each_present_cpu(cpu) {
+		if (cpu == me)
+			continue;
+		on_cpu_async(cpu, func, data);
+	}
+}
+
+static bool cpumask_test_secondary_cpus(cpumask_t *mask)
+{
+	int cpu, me = smp_processor_id();
+
+	for_each_present_cpu(cpu) {
+		if (cpu == me)
+			continue;
+
+		if (!cpumask_test_cpu(cpu, mask))
+			return false;
+	}
+
+	return true;
+}
+
+static void hart_execute(void *data)
+{
+	int me = smp_processor_id();
+
+	cpumask_set_cpu(me, &cpus_alive_after_start);
+}
+
+static void hart_retentive_suspend(void *data)
+{
+	int me = smp_processor_id();
+	unsigned long hartid = current_thread_info()->hartid;
+	struct sbiret ret = sbi_hart_suspend(SBI_EXT_HSM_HART_SUSPEND_RETENTIVE, __pa(NULL), __pa(NULL));
+
+	if (ret.error)
+		report_fail("failed to retentive suspend hart %ld", hartid);
+	else
+		cpumask_set_cpu(me, &cpus_alive_after_retentive_suspend);
+}
+
+static void hart_non_retentive_suspend(void *data)
+{
+	unsigned long hartid = current_thread_info()->hartid;
+
+	struct sbiret ret = sbi_hart_suspend(SBI_EXT_HSM_HART_SUSPEND_NON_RETENTIVE,
+					     virt_to_phys(&sbi_hsm_check_non_retentive_suspend), hartid);
+
+	if (ret.error)
+		report_fail("failed to non-retentive suspend hart %ld", hartid);
+}
+
+static void hart_wait_on_status(unsigned long hartid, enum sbi_ext_hsm_sid status)
+{
+	struct sbiret ret = sbi_hart_get_status(hartid);
+
+	while (!ret.error && ret.value == status)
+		ret = sbi_hart_get_status(hartid);
+
+	if (ret.error)
+		report_fail("got %ld while waiting on status %u for hart %lx\n", ret.error, status, hartid);
+}
+
+static void check_hsm(void)
+{
+	struct sbiret ret;
+	unsigned long hartid;
+	unsigned char per_hart_start_checks, per_hart_non_retentive_suspend_checks;
+	unsigned long hart_mask[NR_CPUS / BITS_PER_LONG] = {0};
+	bool ipi_failed = false;
+	int cpu, me = smp_processor_id();
+
+	report_prefix_push("hsm");
+
+	if (!sbi_probe(SBI_EXT_HSM)) {
+		report_skip("hsm extension not available");
+		report_prefix_pop();
+		return;
+	}
+
+	report_prefix_push("hart_get_status");
+
+	hartid = current_thread_info()->hartid;
+	ret = sbi_hart_get_status(hartid);
+
+	if (ret.error == SBI_ERR_INVALID_PARAM) {
+		report_fail("current hartid is invalid");
+		report_prefix_popn(2);
+		return;
+	} else if (ret.value != SBI_EXT_HSM_STARTED) {
+		report_fail("current hart is not started");
+		report_prefix_popn(2);
+		return;
+	}
+
+	report_pass("status of current hart is started");
+
+	report_prefix_pop();
+
+	report_prefix_push("hart_start");
+
+	ret = sbi_hart_start(hartid, virt_to_phys(&hart_execute), __pa(NULL));
+	report(ret.error == SBI_ERR_ALREADY_AVAILABLE, "boot hart is already started");
+
+	ret = sbi_hart_start(ULONG_MAX, virt_to_phys(&hart_execute), __pa(NULL));
+	report(ret.error == SBI_ERR_INVALID_PARAM, "invalid hartid check");
+
+	if (nr_cpus < 2) {
+		report_skip("no other cpus to run the remaining hsm tests on");
+		report_prefix_popn(2);
+		return;
+	}
+
+	for_each_present_cpu(cpu) {
+		if (cpu == me)
+			continue;
+
+		hartid = cpus[cpu].hartid;
+		ret = sbi_hart_start(hartid, virt_to_phys(&sbi_hsm_check_hart_start), hartid);
+
+		if (ret.error) {
+			report_fail("failed to start test hart %ld", hartid);
+			report_prefix_popn(2);
+			return;
+		}
+	}
+
+	for_each_present_cpu(cpu) {
+		if (cpu == me)
+			continue;
+
+		hartid = cpus[cpu].hartid;
+
+		hart_wait_on_status(hartid, SBI_EXT_HSM_STOPPED);
+		hart_wait_on_status(hartid, SBI_EXT_HSM_START_PENDING);
+		ret = sbi_hart_get_status(hartid);
+		report(!ret.error && ret.value == SBI_EXT_HSM_STARTED,
+		       "test hart with hartid %ld successfully started", hartid);
+	}
+
+	for_each_present_cpu(cpu) {
+		if (cpu == me)
+			continue;
+
+		hartid = cpus[cpu].hartid;
+
+		while (!((per_hart_start_checks = READ_ONCE(sbi_hsm_hart_start_checks[hartid]))
+			 & SBI_HSM_TEST_DONE))
+			cpu_relax();
+
+		report(per_hart_start_checks & SBI_HSM_TEST_SATP,
+		       "satp is zero for test hart %ld", hartid);
+		report(per_hart_start_checks & SBI_HSM_TEST_SIE,
+		       "sstatus.SIE is zero for test hart %ld", hartid);
+		report(per_hart_start_checks & SBI_HSM_TEST_HARTID_A1,
+		       "a0 and a1 are hartid for test hart %ld", hartid);
+	}
+
+	report_prefix_pop();
+
+	report_prefix_push("hart_stop");
+
+	for_each_present_cpu(cpu) {
+		if (cpu == me)
+			continue;
+
+		hartid = cpus[cpu].hartid;
+		WRITE_ONCE(sbi_hsm_stop_hart[hartid], true);
+	}
+
+	for_each_present_cpu(cpu) {
+		if (cpu == me)
+			continue;
+
+		hartid = cpus[cpu].hartid;
+		hart_wait_on_status(hartid, SBI_EXT_HSM_STARTED);
+		hart_wait_on_status(hartid, SBI_EXT_HSM_STOP_PENDING);
+		ret = sbi_hart_get_status(hartid);
+		report(!ret.error && (ret.value == SBI_EXT_HSM_STOPPED),
+		       "test hart %ld successfully stopped", hartid);
+	}
+
+	/* Reset the stop flags so that we can reuse them after suspension tests */
+	for_each_present_cpu(cpu) {
+		if (cpu == me)
+			continue;
+
+		hartid = cpus[cpu].hartid;
+		WRITE_ONCE(sbi_hsm_stop_hart[hartid], false);
+	}
+
+	report_prefix_pop();
+
+	report_prefix_push("hart_start");
+
+	on_secondary_cpus_async(hart_execute, NULL);
+
+	for_each_present_cpu(cpu) {
+		if (cpu == me)
+			continue;
+
+		hartid = cpus[cpu].hartid;
+		hart_wait_on_status(hartid, SBI_EXT_HSM_STOPPED);
+		hart_wait_on_status(hartid, SBI_EXT_HSM_START_PENDING);
+		ret = sbi_hart_get_status(hartid);
+		report(!ret.error && (ret.value == SBI_EXT_HSM_STARTED),
+		       "new hart with hartid %ld successfully started", hartid);
+	}
+
+	while (!cpumask_test_secondary_cpus(&cpu_idle_mask))
+		cpu_relax();
+
+	report(cpumask_full(&cpu_online_mask), "all cpus online");
+	report(cpumask_test_secondary_cpus(&cpus_alive_after_start),
+	       "all secondary harts successfully executed code after start");
+
+	report_prefix_pop();
+
+	report_prefix_push("hart_suspend");
+
+	if (sbi_probe(SBI_EXT_IPI)) {
+		on_secondary_cpus_async(hart_retentive_suspend, NULL);
+
+		for_each_present_cpu(cpu) {
+			if (cpu == me)
+				continue;
+
+			hartid = cpus[cpu].hartid;
+			hart_mask[hartid / BITS_PER_LONG] |= 1UL << hartid;
+			hart_wait_on_status(hartid, SBI_EXT_HSM_STARTED);
+			hart_wait_on_status(hartid, SBI_EXT_HSM_SUSPEND_PENDING);
+			ret = sbi_hart_get_status(hartid);
+			report(!ret.error && (ret.value == SBI_EXT_HSM_SUSPENDED),
+			       "hart %ld successfully retentive suspended", hartid);
+		}
+
+		for (int i = 0; i < NR_CPUS / BITS_PER_LONG; ++i) {
+			if (hart_mask[i]) {
+				ret = sbi_send_ipi(hart_mask[i], i * BITS_PER_LONG);
+				if (ret.error) {
+					ipi_failed = true;
+					report_fail("got %ld when sending ipi to retentive suspended harts",
+						    ret.error);
+					break;
+				}
+			}
+		}
+
+		if (!ipi_failed) {
+			for_each_present_cpu(cpu) {
+				if (cpu == me)
+					continue;
+
+				hartid = cpus[cpu].hartid;
+				hart_wait_on_status(hartid, SBI_EXT_HSM_SUSPENDED);
+				hart_wait_on_status(hartid, SBI_EXT_HSM_RESUME_PENDING);
+				ret = sbi_hart_get_status(hartid);
+				report(!ret.error && (ret.value == SBI_EXT_HSM_STARTED),
+				       "hart %ld successfully retentive resumed", hartid);
+			}
+
+			while (!cpumask_test_secondary_cpus(&cpu_idle_mask))
+				cpu_relax();
+
+			report(cpumask_full(&cpu_online_mask), "all cpus online");
+			report(cpumask_test_secondary_cpus(&cpus_alive_after_retentive_suspend),
+			       "all secondary harts successfully executed code after retentive suspend");
+		}
+
+		/* Reset the ipi_failed flag so that we can reuse it for non-retentive suspension tests */
+		ipi_failed = false;
+
+		on_secondary_cpus_async(hart_non_retentive_suspend, NULL);
+
+		for_each_present_cpu(cpu) {
+			if (cpu == me)
+				continue;
+
+			hartid = cpus[cpu].hartid;
+			hart_wait_on_status(hartid, SBI_EXT_HSM_STARTED);
+			hart_wait_on_status(hartid, SBI_EXT_HSM_SUSPEND_PENDING);
+			ret = sbi_hart_get_status(hartid);
+			report(!ret.error && (ret.value == SBI_EXT_HSM_SUSPENDED),
+			       "hart %ld successfully non-retentive suspended", hartid);
+		}
+
+		for (int i = 0; i < NR_CPUS / BITS_PER_LONG; ++i) {
+			if (hart_mask[i]) {
+				ret = sbi_send_ipi(hart_mask[i], i * BITS_PER_LONG);
+				if (ret.error) {
+					ipi_failed = true;
+					report_fail("got %ld when sending ipi to non-retentive suspended harts",
+						    ret.error);
+					break;
+				}
+			}
+		}
+
+		if (!ipi_failed) {
+			for_each_present_cpu(cpu) {
+				if (cpu == me)
+					continue;
+
+				hartid = cpus[cpu].hartid;
+				hart_wait_on_status(hartid, SBI_EXT_HSM_SUSPENDED);
+				hart_wait_on_status(hartid, SBI_EXT_HSM_RESUME_PENDING);
+				ret = sbi_hart_get_status(hartid);
+				report(!ret.error && (ret.value == SBI_EXT_HSM_STARTED),
+				       "hart %ld successfully non-retentive resumed", hartid);
+			}
+
+			for_each_present_cpu(cpu) {
+				if (cpu == me)
+					continue;
+
+				hartid = cpus[cpu].hartid;
+
+				while (!((per_hart_non_retentive_suspend_checks =
+					 READ_ONCE(sbi_hsm_non_retentive_hart_suspend_checks[hartid]))
+					 & SBI_HSM_TEST_DONE))
+					cpu_relax();
+
+				report(per_hart_non_retentive_suspend_checks & SBI_HSM_TEST_SATP,
+				       "satp is zero for test hart %ld", hartid);
+				report(per_hart_non_retentive_suspend_checks & SBI_HSM_TEST_SIE,
+				       "sstatus.SIE is zero for test hart %ld", hartid);
+				report(per_hart_non_retentive_suspend_checks & SBI_HSM_TEST_HARTID_A1,
+				       "a0 and a1 are hartid for test hart %ld", hartid);
+			}
+
+			report_prefix_pop();
+
+			report_prefix_push("hart_stop");
+
+			for_each_present_cpu(cpu) {
+				if (cpu == me)
+					continue;
+
+				hartid = cpus[cpu].hartid;
+				WRITE_ONCE(sbi_hsm_stop_hart[hartid], true);
+			}
+
+			for_each_present_cpu(cpu) {
+				if (cpu == me)
+					continue;
+
+				hartid = cpus[cpu].hartid;
+				hart_wait_on_status(hartid, SBI_EXT_HSM_STARTED);
+				hart_wait_on_status(hartid, SBI_EXT_HSM_STOP_PENDING);
+				ret = sbi_hart_get_status(hartid);
+				report(!ret.error && (ret.value == SBI_EXT_HSM_STOPPED),
+				       "test hart %ld successfully stopped", hartid);
+			}
+		}
+	} else {
+		report_skip("skipping suspension tests since ipi extension is unavailable");
+	}
+
+	report_prefix_popn(2);
+}
+
 int main(int argc, char **argv)
 {
 	if (argc > 1 && !strcmp(argv[1], "-h")) {
@@ -435,6 +816,7 @@ int main(int argc, char **argv)
 	report_prefix_push("sbi");
 	check_base();
 	check_time();
+	check_hsm();
 	check_dbcn();
 
 	return report_summary();
-- 
2.43.0

[kvm-unit-tests PATCH v2 4/4] riscv: sbi: Add tests for HSM extension

[Andrew Jones]

On Mon, Aug 26, 2024 at 01:08:24AM GMT, James Raphael Tiovalen wrote:
> Add some tests for all of the HSM extension functions. These tests
> ensure that the HSM extension functions follow the behavior as described
> in the SBI specification.
> 
> Signed-off-by: James Raphael Tiovalen <jamestiotio@gmail.com>
> ---
>  riscv/Makefile  |   7 +-
>  riscv/sbi-asm.S |  79 ++++++++++
>  riscv/sbi.c     | 382 ++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 465 insertions(+), 3 deletions(-)
>  create mode 100644 riscv/sbi-asm.S
> 
> diff --git a/riscv/Makefile b/riscv/Makefile
> index 179a373d..548041ad 100644
> --- a/riscv/Makefile
> +++ b/riscv/Makefile
> @@ -43,6 +43,7 @@ cflatobjs += lib/riscv/timer.o
>  ifeq ($(ARCH),riscv32)
>  cflatobjs += lib/ldiv32.o
>  endif
> +cflatobjs += riscv/sbi-asm.o
>  
>  ########################################
>  
> @@ -99,7 +100,7 @@ cflatobjs += lib/efi.o
>  .PRECIOUS: %.so
>  
>  %.so: EFI_LDFLAGS += -defsym=EFI_SUBSYSTEM=0xa --no-undefined
> -%.so: %.o $(FLATLIBS) $(SRCDIR)/riscv/efi/elf_riscv64_efi.lds $(cstart.o) %.aux.o
> +%.so: %.o $(FLATLIBS) $(SRCDIR)/riscv/efi/elf_riscv64_efi.lds $(cstart.o) $(sbi-asm.o) %.aux.o

Something left over from the previous version? We don't have $(sbi-asm.o)
anymore.

>  	$(LD) $(EFI_LDFLAGS) -o $@ -T $(SRCDIR)/riscv/efi/elf_riscv64_efi.lds \
>  		$(filter %.o, $^) $(FLATLIBS) $(EFI_LIBS)
>  
> @@ -115,7 +116,7 @@ cflatobjs += lib/efi.o
>  		-O binary $^ $@
>  else
>  %.elf: LDFLAGS += -pie -n -z notext
> -%.elf: %.o $(FLATLIBS) $(SRCDIR)/riscv/flat.lds $(cstart.o) %.aux.o
> +%.elf: %.o $(FLATLIBS) $(SRCDIR)/riscv/flat.lds $(cstart.o) $(sbi-asm.o) %.aux.o

same

>  	$(LD) $(LDFLAGS) -o $@ -T $(SRCDIR)/riscv/flat.lds \
>  		$(filter %.o, $^) $(FLATLIBS)
>  	@chmod a-x $@
> @@ -127,7 +128,7 @@ else
>  endif
>  
>  generated-files = $(asm-offsets)
> -$(tests:.$(exe)=.o) $(cstart.o) $(cflatobjs): $(generated-files)
> +$(tests:.$(exe)=.o) $(cstart.o) $(sbi-asm.o) $(cflatobjs): $(generated-files)

same

>  
>  arch_clean: asm_offsets_clean
>  	$(RM) $(TEST_DIR)/*.{o,flat,elf,so,efi,debug} \
> diff --git a/riscv/sbi-asm.S b/riscv/sbi-asm.S
> new file mode 100644
> index 00000000..f31bc096
> --- /dev/null
> +++ b/riscv/sbi-asm.S
> @@ -0,0 +1,79 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Helper assembly code routines for RISC-V SBI extension tests.
> + *
> + * Copyright (C) 2024, James Raphael Tiovalen <jamestiotio@gmail.com>
> + */
> +#define __ASSEMBLY__
> +#include <config.h>
> +#include <asm/csr.h>
> +#include <asm/page.h>
> +
> +#define SBI_HSM_TEST_DONE       (1 << 0)
> +#define SBI_HSM_TEST_SATP       (1 << 1)
> +#define SBI_HSM_TEST_SIE        (1 << 2)
> +#define SBI_HSM_TEST_HARTID_A1  (1 << 3)
> +
> +.section .text
> +.balign 4
> +.global sbi_hsm_check_hart_start
> +sbi_hsm_check_hart_start:
> +	li	t0, 0

I see this is a faithful copy of the code I suggested in the last review,
but this is a pointless load of zero to t0, it's not used and then gets
overwritten a few lines down.

> +	csrr	t1, CSR_SATP
> +	bnez	t1, 1f
> +	li	t0, SBI_HSM_TEST_SATP
> +1:	csrr	t1, CSR_SSTATUS
> +	andi	t1, t1, SR_SIE
> +	bnez	t1, 2f
> +	ori	t0, t0, SBI_HSM_TEST_SIE
> +2:	bne	a0, a1, 3f
> +	ori	t0, t0, SBI_HSM_TEST_HARTID_A1
> +3:	ori	t0, t0, SBI_HSM_TEST_DONE
> +	la	t1, sbi_hsm_hart_start_checks
> +	add	t1, t1, a0
> +	sb	t0, 0(t1)
> +4:	la	t0, sbi_hsm_stop_hart
> +	add	t0, t0, a0
> +	lb	t0, 0(t0)

And no need to repeat the address construction, just use another register

 	la      t0, sbi_hsm_stop_hart
 	add     t1, t0, a0
 4:	lb	t0, 0(t1) 

> +	pause
> +	beqz	t0, 4b
> +	li	a7, 0x48534d	/* SBI_EXT_HSM */
> +	li	a6, 1		/* SBI_EXT_HSM_HART_STOP */
> +	ecall
> +	j	halt
> +
> +.balign 4
> +.global sbi_hsm_check_non_retentive_suspend
> +sbi_hsm_check_non_retentive_suspend:
> +	li	t0, 0
> +	csrr	t1, CSR_SATP
> +	bnez	t1, 1f
> +	li	t0, SBI_HSM_TEST_SATP
> +1:	csrr	t1, CSR_SSTATUS
> +	andi	t1, t1, SR_SIE
> +	bnez	t1, 2f
> +	ori	t0, t0, SBI_HSM_TEST_SIE
> +2:	bne	a0, a1, 3f
> +	ori	t0, t0, SBI_HSM_TEST_HARTID_A1
> +3:	ori	t0, t0, SBI_HSM_TEST_DONE
> +	la	t1, sbi_hsm_non_retentive_hart_suspend_checks
> +	add	t1, t1, a0
> +	sb	t0, 0(t1)
> +4:	la	t0, sbi_hsm_stop_hart
> +	add	t0, t0, a0
> +	lb	t0, 0(t0)
> +	pause
> +	beqz	t0, 4b
> +	li	a7, 0x48534d	/* SBI_EXT_HSM */
> +	li	a6, 1		/* SBI_EXT_HSM_HART_STOP */
> +	ecall
> +	j	halt

This is identical, except the address to load for the results, to the
function above. Just make a static helper function that puts that
address in a register, e.g. t6, and then call it from both

/* t6 is the address of the results array */
sbi_hsm_check:
	...

.global sbi_hsm_check_hart_start
sbi_hsm_check_non_retentive_suspend:
	la	t6, sbi_hsm_non_retentive_hart_suspend_checks
	j	sbi_hsm_check

.global sbi_hsm_check_non_retentive_suspend
sbi_hsm_check_non_retentive_suspend:
	la	t6, sbi_hsm_non_retentive_hart_suspend_checks
	j	sbi_hsm_check

> +
> +.section .data
> +.balign PAGE_SIZE
> +.global sbi_hsm_hart_start_checks
> +sbi_hsm_hart_start_checks:			.space CONFIG_NR_CPUS
> +.global sbi_hsm_non_retentive_hart_suspend_checks
> +sbi_hsm_non_retentive_hart_suspend_checks:	.space CONFIG_NR_CPUS
> +.global sbi_hsm_stop_hart
> +sbi_hsm_stop_hart:				.space CONFIG_NR_CPUS

I don't think it should be necessary to create these arrays in assembly.
We should be able to make global arrays in C in riscv/sbi.c and still
access them from the assembly as you've done.

CONFIG_NR_CPUS will support all possible cpuids, but hartids have their
own range and the code above is indexing these arrays by hartid. Since
we should be able to define the arrays in C, then we could also either

 1) assert that max_hartid + 1 <= NR_CPUS
 2) dynamically allocate the arrays using max_hartid + 1 for the size
    and then assign global variables the physical addresses of those
    allocated regions to use in the assembly

(1) is probably good enough

> diff --git a/riscv/sbi.c b/riscv/sbi.c
> index 6469304b..25fc2e81 100644
> --- a/riscv/sbi.c
> +++ b/riscv/sbi.c
> @@ -6,6 +6,8 @@
>   */
>  #include <libcflat.h>
>  #include <alloc_page.h>
> +#include <cpumask.h>
> +#include <on-cpus.h>
>  #include <stdlib.h>
>  #include <string.h>
>  #include <limits.h>
> @@ -17,8 +19,10 @@
>  #include <asm/io.h>
>  #include <asm/isa.h>
>  #include <asm/mmu.h>
> +#include <asm/page.h>
>  #include <asm/processor.h>
>  #include <asm/sbi.h>
> +#include <asm/setup.h>
>  #include <asm/smp.h>
>  #include <asm/timer.h>
>  
> @@ -425,6 +429,383 @@ static void check_dbcn(void)
>  	report_prefix_pop();
>  }
>  
> +#define SBI_HSM_TEST_DONE       (1 << 0)
> +#define SBI_HSM_TEST_SATP       (1 << 1)
> +#define SBI_HSM_TEST_SIE        (1 << 2)
> +#define SBI_HSM_TEST_HARTID_A1  (1 << 3)

We should create a header for these devices, riscv/sbi.h, and share them
with the asm file rather than duplicate them.

> +
> +static cpumask_t cpus_alive_after_start;
> +static cpumask_t cpus_alive_after_retentive_suspend;
> +extern void sbi_hsm_check_hart_start(void);
> +extern void sbi_hsm_check_non_retentive_suspend(void);
> +extern unsigned char sbi_hsm_stop_hart[NR_CPUS];
> +extern unsigned char sbi_hsm_hart_start_checks[NR_CPUS];
> +extern unsigned char sbi_hsm_non_retentive_hart_suspend_checks[NR_CPUS];

need blank line here

> +static void on_secondary_cpus_async(void (*func)(void *data), void *data)
> +{
> +	int cpu, me = smp_processor_id();
> +
> +	for_each_present_cpu(cpu) {
> +		if (cpu == me)
> +			continue;
> +		on_cpu_async(cpu, func, data);
> +	}
> +}

Seems on-cpus is missing an API for "all, but ...". How about, as a
separate patch, adding

 void on_cpus_async(cpumask_t *mask, void (*func)(void *data), void *data)

to lib/on-cpus.c

Then here you'd copy the present mask to your own mask and clear 'me' from
it for an 'all present, but me' mask.

> +
> +static bool cpumask_test_secondary_cpus(cpumask_t *mask)
> +{
> +	int cpu, me = smp_processor_id();
> +
> +	for_each_present_cpu(cpu) {
> +		if (cpu == me)
> +			continue;
> +
> +		if (!cpumask_test_cpu(cpu, mask))
> +			return false;
> +	}
> +
> +	return true;
> +}

This is just

  cpumask_weight(mask) == nr_cpus - 1

or

 !cpumask_test_cpu(me, mask) && cpumask_weight(mask) == nr_cpus - 1

if there's concern for 'me' being set in the mask erroneously.

> +
> +static void hart_execute(void *data)
> +{
> +	int me = smp_processor_id();
> +
> +	cpumask_set_cpu(me, &cpus_alive_after_start);
> +}
> +
> +static void hart_retentive_suspend(void *data)
> +{
> +	int me = smp_processor_id();
> +	unsigned long hartid = current_thread_info()->hartid;
> +	struct sbiret ret = sbi_hart_suspend(SBI_EXT_HSM_HART_SUSPEND_RETENTIVE, __pa(NULL), __pa(NULL));

__pa(NULL) is just 0 and I don't think __pa(NULL) tells the reader
anything useful.

> +
> +	if (ret.error)
> +		report_fail("failed to retentive suspend hart %ld", hartid);

Should output ret.error too

> +	else
> +		cpumask_set_cpu(me, &cpus_alive_after_retentive_suspend);

This mask is redundant with the cpu_idle_mask.

> +}
> +
> +static void hart_non_retentive_suspend(void *data)
> +{
> +	unsigned long hartid = current_thread_info()->hartid;
> +

Put a comment here explaining why opaque must be hartid, i.e. for the
a0==a1 test which ensures a0 is hartid and a1 is opaque per the spec.

> +	struct sbiret ret = sbi_hart_suspend(SBI_EXT_HSM_HART_SUSPEND_NON_RETENTIVE,
> +					     virt_to_phys(&sbi_hsm_check_non_retentive_suspend), hartid);
> +
> +	if (ret.error)

Drop the 'if'. Any return from a non-retentive suspend is a failure.

> +		report_fail("failed to non-retentive suspend hart %ld", hartid);

Should output ret.error too

> +}
> +
> +static void hart_wait_on_status(unsigned long hartid, enum sbi_ext_hsm_sid status)
> +{
> +	struct sbiret ret = sbi_hart_get_status(hartid);
> +
> +	while (!ret.error && ret.value == status)
> +		ret = sbi_hart_get_status(hartid);

Let's throw a cpu_relax() in the loop too.

> +
> +	if (ret.error)
> +		report_fail("got %ld while waiting on status %u for hart %lx\n", ret.error, status, hartid);
> +}
> +
> +static void check_hsm(void)
> +{
> +	struct sbiret ret;
> +	unsigned long hartid;
> +	unsigned char per_hart_start_checks, per_hart_non_retentive_suspend_checks;
> +	unsigned long hart_mask[NR_CPUS / BITS_PER_LONG] = {0};

Use a real cpumask. When passing a single ulong to an SBI call you can
simply use cpumask_bits(mask)[]

> +	bool ipi_failed = false;
> +	int cpu, me = smp_processor_id();
> +
> +	report_prefix_push("hsm");
> +
> +	if (!sbi_probe(SBI_EXT_HSM)) {
> +		report_skip("hsm extension not available");
> +		report_prefix_pop();
> +		return;
> +	}
> +
> +	report_prefix_push("hart_get_status");
> +
> +	hartid = current_thread_info()->hartid;
> +	ret = sbi_hart_get_status(hartid);
> +
> +	if (ret.error == SBI_ERR_INVALID_PARAM) {

Any error is a failure, so this should be if (ret.error) and we should
output the error in the fail message.

> +		report_fail("current hartid is invalid");
> +		report_prefix_popn(2);
> +		return;
> +	} else if (ret.value != SBI_EXT_HSM_STARTED) {
> +		report_fail("current hart is not started");

need to output ret.value

> +		report_prefix_popn(2);
> +		return;
> +	}
> +
> +	report_pass("status of current hart is started");
> +
> +	report_prefix_pop();
> +
> +	report_prefix_push("hart_start");
> +
> +	ret = sbi_hart_start(hartid, virt_to_phys(&hart_execute), __pa(NULL));
> +	report(ret.error == SBI_ERR_ALREADY_AVAILABLE, "boot hart is already started");

If we don't get the expected error of 'already started', then the test
could hang here. A test like this is best to do on a secondary, passing
the results back to the primary through a global.

> +
> +	ret = sbi_hart_start(ULONG_MAX, virt_to_phys(&hart_execute), __pa(NULL));
> +	report(ret.error == SBI_ERR_INVALID_PARAM, "invalid hartid check");
> +
> +	if (nr_cpus < 2) {
> +		report_skip("no other cpus to run the remaining hsm tests on");
> +		report_prefix_popn(2);
> +		return;
> +	}
> +
> +	for_each_present_cpu(cpu) {
> +		if (cpu == me)
> +			continue;
> +
> +		hartid = cpus[cpu].hartid;

Need the same why opaque must be hartid comment here as above.

> +		ret = sbi_hart_start(hartid, virt_to_phys(&sbi_hsm_check_hart_start), hartid);
> +

remove this blank line

> +		if (ret.error) {
> +			report_fail("failed to start test hart %ld", hartid);

output ret.error

> +			report_prefix_popn(2);
> +			return;
> +		}
> +	}
> +
> +	for_each_present_cpu(cpu) {
> +		if (cpu == me)
> +			continue;
> +
> +		hartid = cpus[cpu].hartid;
> +
> +		hart_wait_on_status(hartid, SBI_EXT_HSM_STOPPED);
> +		hart_wait_on_status(hartid, SBI_EXT_HSM_START_PENDING);
> +		ret = sbi_hart_get_status(hartid);
> +		report(!ret.error && ret.value == SBI_EXT_HSM_STARTED,
> +		       "test hart with hartid %ld successfully started", hartid);

We should split the reporting to check ret.error and ret.value separately.

> +	}
> +
> +	for_each_present_cpu(cpu) {
> +		if (cpu == me)
> +			continue;
> +
> +		hartid = cpus[cpu].hartid;
> +
> +		while (!((per_hart_start_checks = READ_ONCE(sbi_hsm_hart_start_checks[hartid]))
> +			 & SBI_HSM_TEST_DONE))

This while condition would be a bit less ugly if we dropped
per_hart_start_checks and then just used hartid to index the array in the
three checks below.

> +			cpu_relax();
> +
> +		report(per_hart_start_checks & SBI_HSM_TEST_SATP,
> +		       "satp is zero for test hart %ld", hartid);
> +		report(per_hart_start_checks & SBI_HSM_TEST_SIE,
> +		       "sstatus.SIE is zero for test hart %ld", hartid);
> +		report(per_hart_start_checks & SBI_HSM_TEST_HARTID_A1,
> +		       "a0 and a1 are hartid for test hart %ld", hartid);
> +	}
> +
> +	report_prefix_pop();
> +
> +	report_prefix_push("hart_stop");
> +
> +	for_each_present_cpu(cpu) {
> +		if (cpu == me)
> +			continue;
> +
> +		hartid = cpus[cpu].hartid;
> +		WRITE_ONCE(sbi_hsm_stop_hart[hartid], true);

instead of the loop we could do

 barrier();
 memset(...);
 barrier();

if we don't mind setting the 'me' hart too.

> +	}
> +
> +	for_each_present_cpu(cpu) {
> +		if (cpu == me)
> +			continue;
> +
> +		hartid = cpus[cpu].hartid;
> +		hart_wait_on_status(hartid, SBI_EXT_HSM_STARTED);
> +		hart_wait_on_status(hartid, SBI_EXT_HSM_STOP_PENDING);
> +		ret = sbi_hart_get_status(hartid);
> +		report(!ret.error && (ret.value == SBI_EXT_HSM_STOPPED),

unnecessary (), anyway we should split the check for ret.error and
ret.value and output ret.error/value.  See the DBCN test for how
we do it. For example, the write_byte test has

        report(ret.error == SBI_SUCCESS, "write success (error=%ld)", ret.error);
        report(ret.value == 0, "expected ret.value (%ld)", ret.value);

> +		       "test hart %ld successfully stopped", hartid);
> +	}
> +
> +	/* Reset the stop flags so that we can reuse them after suspension tests */
> +	for_each_present_cpu(cpu) {
> +		if (cpu == me)
> +			continue;
> +
> +		hartid = cpus[cpu].hartid;
> +		WRITE_ONCE(sbi_hsm_stop_hart[hartid], false);

instead of the loop we could do

 barrier();
 memset(...);
 barrier();

since we definitely we don't mind setting the 'me' hart too as it's
getting set to zero.

> +	}
> +
> +	report_prefix_pop();
> +
> +	report_prefix_push("hart_start");
> +
> +	on_secondary_cpus_async(hart_execute, NULL);
> +
> +	for_each_present_cpu(cpu) {
> +		if (cpu == me)
> +			continue;
> +
> +		hartid = cpus[cpu].hartid;
> +		hart_wait_on_status(hartid, SBI_EXT_HSM_STOPPED);
> +		hart_wait_on_status(hartid, SBI_EXT_HSM_START_PENDING);
> +		ret = sbi_hart_get_status(hartid);
> +		report(!ret.error && (ret.value == SBI_EXT_HSM_STARTED),
> +		       "new hart with hartid %ld successfully started", hartid);

split error/value tests

> +	}
> +
> +	while (!cpumask_test_secondary_cpus(&cpu_idle_mask))

while (cpumask_weight(&cpu_idle_mask) != nr_cpus - 1)

> +		cpu_relax();

We could get stuck in this loop forever if the cpus don't come up, so you
may want to set a timeout with timer_start(), then either handler the
failure due to the timeout handler firing or stop the timer, so

 timer_start(long_enough_duration);
 while (cpumask_weight(&cpu_idle_mask) != nr_cpus - 1)
    cpu_relax();
 timer_stop();
 if (timer_fired)
    report_fail(...)

> +
> +	report(cpumask_full(&cpu_online_mask), "all cpus online");
> +	report(cpumask_test_secondary_cpus(&cpus_alive_after_start),
> +	       "all secondary harts successfully executed code after start");

This test is covered by the idle mask check above. hart_execute() could
just be an empty function

> +
> +	report_prefix_pop();
> +
> +	report_prefix_push("hart_suspend");
> +
> +	if (sbi_probe(SBI_EXT_IPI)) {
> +		on_secondary_cpus_async(hart_retentive_suspend, NULL);
> +
> +		for_each_present_cpu(cpu) {
> +			if (cpu == me)
> +				continue;
> +
> +			hartid = cpus[cpu].hartid;
> +			hart_mask[hartid / BITS_PER_LONG] |= 1UL << hartid;
> +			hart_wait_on_status(hartid, SBI_EXT_HSM_STARTED);
> +			hart_wait_on_status(hartid, SBI_EXT_HSM_SUSPEND_PENDING);
> +			ret = sbi_hart_get_status(hartid);
> +			report(!ret.error && (ret.value == SBI_EXT_HSM_SUSPENDED),

split report

> +			       "hart %ld successfully retentive suspended", hartid);
> +		}
> +
> +		for (int i = 0; i < NR_CPUS / BITS_PER_LONG; ++i) {
> +			if (hart_mask[i]) {

using a real mask for hart_mask would allow using

  for_each_cpu(i, &hart_mask)

> +				ret = sbi_send_ipi(hart_mask[i], i * BITS_PER_LONG);

but, instead, let's provide the following (untested) function in lib/riscv/sbi.c

   struct sbiret sbi_send_ipi_cpumask(cpumask_t *mask)
   {
        struct sbiret ret;

	for (int i = 0; i < CPUMASK_NR_LONGS; ++i) {
	    if (cpumask_bits(mask)[i]) {
	       ret = sbi_send_ipi(cpumask_bits(mask)[i], i * BITS_PER_LONG);
	       if (ret.error)
	          break;
	    }
	}

	return ret;
   }

then you can drop your loop and just call the new API.

> +				if (ret.error) {
> +					ipi_failed = true;
> +					report_fail("got %ld when sending ipi to retentive suspended harts",
> +						    ret.error);
> +					break;
> +				}
> +			}
> +		}
> +
> +		if (!ipi_failed) {
> +			for_each_present_cpu(cpu) {
> +				if (cpu == me)
> +					continue;
> +
> +				hartid = cpus[cpu].hartid;
> +				hart_wait_on_status(hartid, SBI_EXT_HSM_SUSPENDED);
> +				hart_wait_on_status(hartid, SBI_EXT_HSM_RESUME_PENDING);
> +				ret = sbi_hart_get_status(hartid);
> +				report(!ret.error && (ret.value == SBI_EXT_HSM_STARTED),

split report

> +				       "hart %ld successfully retentive resumed", hartid);
> +			}
> +
> +			while (!cpumask_test_secondary_cpus(&cpu_idle_mask))
> +				cpu_relax();

Timed wait and use cpumask_weight

> +
> +			report(cpumask_full(&cpu_online_mask), "all cpus online");
> +			report(cpumask_test_secondary_cpus(&cpus_alive_after_retentive_suspend),
> +			       "all secondary harts successfully executed code after retentive suspend");

Already checked with the wait on idle.

> +		}
> +
> +		/* Reset the ipi_failed flag so that we can reuse it for non-retentive suspension tests */
> +		ipi_failed = false;
> +
> +		on_secondary_cpus_async(hart_non_retentive_suspend, NULL);
> +
> +		for_each_present_cpu(cpu) {
> +			if (cpu == me)
> +				continue;
> +
> +			hartid = cpus[cpu].hartid;
> +			hart_wait_on_status(hartid, SBI_EXT_HSM_STARTED);
> +			hart_wait_on_status(hartid, SBI_EXT_HSM_SUSPEND_PENDING);
> +			ret = sbi_hart_get_status(hartid);
> +			report(!ret.error && (ret.value == SBI_EXT_HSM_SUSPENDED),

split errors

> +			       "hart %ld successfully non-retentive suspended", hartid);
> +		}
> +
> +		for (int i = 0; i < NR_CPUS / BITS_PER_LONG; ++i) {
> +			if (hart_mask[i]) {

use real mask

> +				ret = sbi_send_ipi(hart_mask[i], i * BITS_PER_LONG);
> +				if (ret.error) {
> +					ipi_failed = true;
> +					report_fail("got %ld when sending ipi to non-retentive suspended harts",
> +						    ret.error);
> +					break;
> +				}
> +			}
> +		}
> +
> +		if (!ipi_failed) {
> +			for_each_present_cpu(cpu) {
> +				if (cpu == me)
> +					continue;
> +
> +				hartid = cpus[cpu].hartid;
> +				hart_wait_on_status(hartid, SBI_EXT_HSM_SUSPENDED);
> +				hart_wait_on_status(hartid, SBI_EXT_HSM_RESUME_PENDING);
> +				ret = sbi_hart_get_status(hartid);
> +				report(!ret.error && (ret.value == SBI_EXT_HSM_STARTED),

split errors

> +				       "hart %ld successfully non-retentive resumed", hartid);
> +			}
> +
> +			for_each_present_cpu(cpu) {
> +				if (cpu == me)
> +					continue;
> +
> +				hartid = cpus[cpu].hartid;
> +
> +				while (!((per_hart_non_retentive_suspend_checks =
> +					 READ_ONCE(sbi_hsm_non_retentive_hart_suspend_checks[hartid]))
> +					 & SBI_HSM_TEST_DONE))

Could drop the local variable and index the array below.

> +					cpu_relax();
> +
> +				report(per_hart_non_retentive_suspend_checks & SBI_HSM_TEST_SATP,
> +				       "satp is zero for test hart %ld", hartid);
> +				report(per_hart_non_retentive_suspend_checks & SBI_HSM_TEST_SIE,
> +				       "sstatus.SIE is zero for test hart %ld", hartid);
> +				report(per_hart_non_retentive_suspend_checks & SBI_HSM_TEST_HARTID_A1,
> +				       "a0 and a1 are hartid for test hart %ld", hartid);
> +			}
> +
> +			report_prefix_pop();
> +
> +			report_prefix_push("hart_stop");
> +
> +			for_each_present_cpu(cpu) {
> +				if (cpu == me)
> +					continue;
> +
> +				hartid = cpus[cpu].hartid;
> +				WRITE_ONCE(sbi_hsm_stop_hart[hartid], true);

barrier, memset, barrier?

> +			}
> +
> +			for_each_present_cpu(cpu) {
> +				if (cpu == me)
> +					continue;
> +
> +				hartid = cpus[cpu].hartid;
> +				hart_wait_on_status(hartid, SBI_EXT_HSM_STARTED);
> +				hart_wait_on_status(hartid, SBI_EXT_HSM_STOP_PENDING);
> +				ret = sbi_hart_get_status(hartid);
> +				report(!ret.error && (ret.value == SBI_EXT_HSM_STOPPED),

split errors

> +				       "test hart %ld successfully stopped", hartid);
> +			}
> +		}

We can avoid having to add a level of indention to all the code that
depends on SBI_EXT_IPI by either putting it all in a function and then
calling that when IPI is available or by doing

  if (!sbi_probe(SBI_EXT_IPI)) {
     report_skip("skipping suspension tests since ipi extension is unavailable");
     return;
  }

since all these tests are currently the last in this function.

> +	} else {
> +		report_skip("skipping suspension tests since ipi extension is unavailable");
> +	}
> +
> +	report_prefix_popn(2);
> +}
> +
>  int main(int argc, char **argv)
>  {
>  	if (argc > 1 && !strcmp(argv[1], "-h")) {
> @@ -435,6 +816,7 @@ int main(int argc, char **argv)
>  	report_prefix_push("sbi");
>  	check_base();
>  	check_time();
> +	check_hsm();
>  	check_dbcn();
>  
>  	return report_summary();
> -- 
> 2.43.0
>

Thanks,
drew

Re: [kvm-unit-tests PATCH v2 4/4] riscv: sbi: Add tests for HSM extension

[Andrew Jones]

On Mon, Aug 26, 2024 at 01:08:24AM GMT, James Raphael Tiovalen wrote:
> Add some tests for all of the HSM extension functions. These tests
> ensure that the HSM extension functions follow the behavior as described
> in the SBI specification.
> 
> Signed-off-by: James Raphael Tiovalen <jamestiotio@gmail.com>
> ---
>  riscv/Makefile  |   7 +-
>  riscv/sbi-asm.S |  79 ++++++++++
>  riscv/sbi.c     | 382 ++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 465 insertions(+), 3 deletions(-)
>  create mode 100644 riscv/sbi-asm.S
> 
> diff --git a/riscv/Makefile b/riscv/Makefile
> index 179a373d..548041ad 100644
> --- a/riscv/Makefile
> +++ b/riscv/Makefile
> @@ -43,6 +43,7 @@ cflatobjs += lib/riscv/timer.o
>  ifeq ($(ARCH),riscv32)
>  cflatobjs += lib/ldiv32.o
>  endif
> +cflatobjs += riscv/sbi-asm.o
>  
>  ########################################
>  
> @@ -99,7 +100,7 @@ cflatobjs += lib/efi.o
>  .PRECIOUS: %.so
>  
>  %.so: EFI_LDFLAGS += -defsym=EFI_SUBSYSTEM=0xa --no-undefined
> -%.so: %.o $(FLATLIBS) $(SRCDIR)/riscv/efi/elf_riscv64_efi.lds $(cstart.o) %.aux.o
> +%.so: %.o $(FLATLIBS) $(SRCDIR)/riscv/efi/elf_riscv64_efi.lds $(cstart.o) $(sbi-asm.o) %.aux.o

Something left over from the previous version? We don't have $(sbi-asm.o)
anymore.

>  	$(LD) $(EFI_LDFLAGS) -o $@ -T $(SRCDIR)/riscv/efi/elf_riscv64_efi.lds \
>  		$(filter %.o, $^) $(FLATLIBS) $(EFI_LIBS)
>  
> @@ -115,7 +116,7 @@ cflatobjs += lib/efi.o
>  		-O binary $^ $@
>  else
>  %.elf: LDFLAGS += -pie -n -z notext
> -%.elf: %.o $(FLATLIBS) $(SRCDIR)/riscv/flat.lds $(cstart.o) %.aux.o
> +%.elf: %.o $(FLATLIBS) $(SRCDIR)/riscv/flat.lds $(cstart.o) $(sbi-asm.o) %.aux.o

same

>  	$(LD) $(LDFLAGS) -o $@ -T $(SRCDIR)/riscv/flat.lds \
>  		$(filter %.o, $^) $(FLATLIBS)
>  	@chmod a-x $@
> @@ -127,7 +128,7 @@ else
>  endif
>  
>  generated-files = $(asm-offsets)
> -$(tests:.$(exe)=.o) $(cstart.o) $(cflatobjs): $(generated-files)
> +$(tests:.$(exe)=.o) $(cstart.o) $(sbi-asm.o) $(cflatobjs): $(generated-files)

same

>  
>  arch_clean: asm_offsets_clean
>  	$(RM) $(TEST_DIR)/*.{o,flat,elf,so,efi,debug} \
> diff --git a/riscv/sbi-asm.S b/riscv/sbi-asm.S
> new file mode 100644
> index 00000000..f31bc096
> --- /dev/null
> +++ b/riscv/sbi-asm.S
> @@ -0,0 +1,79 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Helper assembly code routines for RISC-V SBI extension tests.
> + *
> + * Copyright (C) 2024, James Raphael Tiovalen <jamestiotio@gmail.com>
> + */
> +#define __ASSEMBLY__
> +#include <config.h>
> +#include <asm/csr.h>
> +#include <asm/page.h>
> +
> +#define SBI_HSM_TEST_DONE       (1 << 0)
> +#define SBI_HSM_TEST_SATP       (1 << 1)
> +#define SBI_HSM_TEST_SIE        (1 << 2)
> +#define SBI_HSM_TEST_HARTID_A1  (1 << 3)
> +
> +.section .text
> +.balign 4
> +.global sbi_hsm_check_hart_start
> +sbi_hsm_check_hart_start:
> +	li	t0, 0

I see this is a faithful copy of the code I suggested in the last review,
but this is a pointless load of zero to t0, it's not used and then gets
overwritten a few lines down.

> +	csrr	t1, CSR_SATP
> +	bnez	t1, 1f
> +	li	t0, SBI_HSM_TEST_SATP
> +1:	csrr	t1, CSR_SSTATUS
> +	andi	t1, t1, SR_SIE
> +	bnez	t1, 2f
> +	ori	t0, t0, SBI_HSM_TEST_SIE
> +2:	bne	a0, a1, 3f
> +	ori	t0, t0, SBI_HSM_TEST_HARTID_A1
> +3:	ori	t0, t0, SBI_HSM_TEST_DONE
> +	la	t1, sbi_hsm_hart_start_checks
> +	add	t1, t1, a0
> +	sb	t0, 0(t1)
> +4:	la	t0, sbi_hsm_stop_hart
> +	add	t0, t0, a0
> +	lb	t0, 0(t0)

And no need to repeat the address construction, just use another register

 	la      t0, sbi_hsm_stop_hart
 	add     t1, t0, a0
 4:	lb	t0, 0(t1) 

> +	pause
> +	beqz	t0, 4b
> +	li	a7, 0x48534d	/* SBI_EXT_HSM */
> +	li	a6, 1		/* SBI_EXT_HSM_HART_STOP */
> +	ecall
> +	j	halt
> +
> +.balign 4
> +.global sbi_hsm_check_non_retentive_suspend
> +sbi_hsm_check_non_retentive_suspend:
> +	li	t0, 0
> +	csrr	t1, CSR_SATP
> +	bnez	t1, 1f
> +	li	t0, SBI_HSM_TEST_SATP
> +1:	csrr	t1, CSR_SSTATUS
> +	andi	t1, t1, SR_SIE
> +	bnez	t1, 2f
> +	ori	t0, t0, SBI_HSM_TEST_SIE
> +2:	bne	a0, a1, 3f
> +	ori	t0, t0, SBI_HSM_TEST_HARTID_A1
> +3:	ori	t0, t0, SBI_HSM_TEST_DONE
> +	la	t1, sbi_hsm_non_retentive_hart_suspend_checks
> +	add	t1, t1, a0
> +	sb	t0, 0(t1)
> +4:	la	t0, sbi_hsm_stop_hart
> +	add	t0, t0, a0
> +	lb	t0, 0(t0)
> +	pause
> +	beqz	t0, 4b
> +	li	a7, 0x48534d	/* SBI_EXT_HSM */
> +	li	a6, 1		/* SBI_EXT_HSM_HART_STOP */
> +	ecall
> +	j	halt

This is identical, except the address to load for the results, to the
function above. Just make a static helper function that puts that
address in a register, e.g. t6, and then call it from both

/* t6 is the address of the results array */
sbi_hsm_check:
	...

.global sbi_hsm_check_hart_start
sbi_hsm_check_non_retentive_suspend:
	la	t6, sbi_hsm_non_retentive_hart_suspend_checks
	j	sbi_hsm_check

.global sbi_hsm_check_non_retentive_suspend
sbi_hsm_check_non_retentive_suspend:
	la	t6, sbi_hsm_non_retentive_hart_suspend_checks
	j	sbi_hsm_check

> +
> +.section .data
> +.balign PAGE_SIZE
> +.global sbi_hsm_hart_start_checks
> +sbi_hsm_hart_start_checks:			.space CONFIG_NR_CPUS
> +.global sbi_hsm_non_retentive_hart_suspend_checks
> +sbi_hsm_non_retentive_hart_suspend_checks:	.space CONFIG_NR_CPUS
> +.global sbi_hsm_stop_hart
> +sbi_hsm_stop_hart:				.space CONFIG_NR_CPUS

I don't think it should be necessary to create these arrays in assembly.
We should be able to make global arrays in C in riscv/sbi.c and still
access them from the assembly as you've done.

CONFIG_NR_CPUS will support all possible cpuids, but hartids have their
own range and the code above is indexing these arrays by hartid. Since
we should be able to define the arrays in C, then we could also either

 1) assert that max_hartid + 1 <= NR_CPUS
 2) dynamically allocate the arrays using max_hartid + 1 for the size
    and then assign global variables the physical addresses of those
    allocated regions to use in the assembly

(1) is probably good enough

> diff --git a/riscv/sbi.c b/riscv/sbi.c
> index 6469304b..25fc2e81 100644
> --- a/riscv/sbi.c
> +++ b/riscv/sbi.c
> @@ -6,6 +6,8 @@
>   */
>  #include <libcflat.h>
>  #include <alloc_page.h>
> +#include <cpumask.h>
> +#include <on-cpus.h>
>  #include <stdlib.h>
>  #include <string.h>
>  #include <limits.h>
> @@ -17,8 +19,10 @@
>  #include <asm/io.h>
>  #include <asm/isa.h>
>  #include <asm/mmu.h>
> +#include <asm/page.h>
>  #include <asm/processor.h>
>  #include <asm/sbi.h>
> +#include <asm/setup.h>
>  #include <asm/smp.h>
>  #include <asm/timer.h>
>  
> @@ -425,6 +429,383 @@ static void check_dbcn(void)
>  	report_prefix_pop();
>  }
>  
> +#define SBI_HSM_TEST_DONE       (1 << 0)
> +#define SBI_HSM_TEST_SATP       (1 << 1)
> +#define SBI_HSM_TEST_SIE        (1 << 2)
> +#define SBI_HSM_TEST_HARTID_A1  (1 << 3)

We should create a header for these devices, riscv/sbi.h, and share them
with the asm file rather than duplicate them.

> +
> +static cpumask_t cpus_alive_after_start;
> +static cpumask_t cpus_alive_after_retentive_suspend;
> +extern void sbi_hsm_check_hart_start(void);
> +extern void sbi_hsm_check_non_retentive_suspend(void);
> +extern unsigned char sbi_hsm_stop_hart[NR_CPUS];
> +extern unsigned char sbi_hsm_hart_start_checks[NR_CPUS];
> +extern unsigned char sbi_hsm_non_retentive_hart_suspend_checks[NR_CPUS];

need blank line here

> +static void on_secondary_cpus_async(void (*func)(void *data), void *data)
> +{
> +	int cpu, me = smp_processor_id();
> +
> +	for_each_present_cpu(cpu) {
> +		if (cpu == me)
> +			continue;
> +		on_cpu_async(cpu, func, data);
> +	}
> +}

Seems on-cpus is missing an API for "all, but ...". How about, as a
separate patch, adding

 void on_cpus_async(cpumask_t *mask, void (*func)(void *data), void *data)

to lib/on-cpus.c

Then here you'd copy the present mask to your own mask and clear 'me' from
it for an 'all present, but me' mask.

> +
> +static bool cpumask_test_secondary_cpus(cpumask_t *mask)
> +{
> +	int cpu, me = smp_processor_id();
> +
> +	for_each_present_cpu(cpu) {
> +		if (cpu == me)
> +			continue;
> +
> +		if (!cpumask_test_cpu(cpu, mask))
> +			return false;
> +	}
> +
> +	return true;
> +}

This is just

  cpumask_weight(mask) == nr_cpus - 1

or

 !cpumask_test_cpu(me, mask) && cpumask_weight(mask) == nr_cpus - 1

if there's concern for 'me' being set in the mask erroneously.

> +
> +static void hart_execute(void *data)
> +{
> +	int me = smp_processor_id();
> +
> +	cpumask_set_cpu(me, &cpus_alive_after_start);
> +}
> +
> +static void hart_retentive_suspend(void *data)
> +{
> +	int me = smp_processor_id();
> +	unsigned long hartid = current_thread_info()->hartid;
> +	struct sbiret ret = sbi_hart_suspend(SBI_EXT_HSM_HART_SUSPEND_RETENTIVE, __pa(NULL), __pa(NULL));

__pa(NULL) is just 0 and I don't think __pa(NULL) tells the reader
anything useful.

> +
> +	if (ret.error)
> +		report_fail("failed to retentive suspend hart %ld", hartid);

Should output ret.error too

> +	else
> +		cpumask_set_cpu(me, &cpus_alive_after_retentive_suspend);

This mask is redundant with the cpu_idle_mask.

> +}
> +
> +static void hart_non_retentive_suspend(void *data)
> +{
> +	unsigned long hartid = current_thread_info()->hartid;
> +

Put a comment here explaining why opaque must be hartid, i.e. for the
a0==a1 test which ensures a0 is hartid and a1 is opaque per the spec.

> +	struct sbiret ret = sbi_hart_suspend(SBI_EXT_HSM_HART_SUSPEND_NON_RETENTIVE,
> +					     virt_to_phys(&sbi_hsm_check_non_retentive_suspend), hartid);
> +
> +	if (ret.error)

Drop the 'if'. Any return from a non-retentive suspend is a failure.

> +		report_fail("failed to non-retentive suspend hart %ld", hartid);

Should output ret.error too

> +}
> +
> +static void hart_wait_on_status(unsigned long hartid, enum sbi_ext_hsm_sid status)
> +{
> +	struct sbiret ret = sbi_hart_get_status(hartid);
> +
> +	while (!ret.error && ret.value == status)
> +		ret = sbi_hart_get_status(hartid);

Let's throw a cpu_relax() in the loop too.

> +
> +	if (ret.error)
> +		report_fail("got %ld while waiting on status %u for hart %lx\n", ret.error, status, hartid);
> +}
> +
> +static void check_hsm(void)
> +{
> +	struct sbiret ret;
> +	unsigned long hartid;
> +	unsigned char per_hart_start_checks, per_hart_non_retentive_suspend_checks;
> +	unsigned long hart_mask[NR_CPUS / BITS_PER_LONG] = {0};

Use a real cpumask. When passing a single ulong to an SBI call you can
simply use cpumask_bits(mask)[]

> +	bool ipi_failed = false;
> +	int cpu, me = smp_processor_id();
> +
> +	report_prefix_push("hsm");
> +
> +	if (!sbi_probe(SBI_EXT_HSM)) {
> +		report_skip("hsm extension not available");
> +		report_prefix_pop();
> +		return;
> +	}
> +
> +	report_prefix_push("hart_get_status");
> +
> +	hartid = current_thread_info()->hartid;
> +	ret = sbi_hart_get_status(hartid);
> +
> +	if (ret.error == SBI_ERR_INVALID_PARAM) {

Any error is a failure, so this should be if (ret.error) and we should
output the error in the fail message.

> +		report_fail("current hartid is invalid");
> +		report_prefix_popn(2);
> +		return;
> +	} else if (ret.value != SBI_EXT_HSM_STARTED) {
> +		report_fail("current hart is not started");

need to output ret.value

> +		report_prefix_popn(2);
> +		return;
> +	}
> +
> +	report_pass("status of current hart is started");
> +
> +	report_prefix_pop();
> +
> +	report_prefix_push("hart_start");
> +
> +	ret = sbi_hart_start(hartid, virt_to_phys(&hart_execute), __pa(NULL));
> +	report(ret.error == SBI_ERR_ALREADY_AVAILABLE, "boot hart is already started");

If we don't get the expected error of 'already started', then the test
could hang here. A test like this is best to do on a secondary, passing
the results back to the primary through a global.

> +
> +	ret = sbi_hart_start(ULONG_MAX, virt_to_phys(&hart_execute), __pa(NULL));
> +	report(ret.error == SBI_ERR_INVALID_PARAM, "invalid hartid check");
> +
> +	if (nr_cpus < 2) {
> +		report_skip("no other cpus to run the remaining hsm tests on");
> +		report_prefix_popn(2);
> +		return;
> +	}
> +
> +	for_each_present_cpu(cpu) {
> +		if (cpu == me)
> +			continue;
> +
> +		hartid = cpus[cpu].hartid;

Need the same why opaque must be hartid comment here as above.

> +		ret = sbi_hart_start(hartid, virt_to_phys(&sbi_hsm_check_hart_start), hartid);
> +

remove this blank line

> +		if (ret.error) {
> +			report_fail("failed to start test hart %ld", hartid);

output ret.error

> +			report_prefix_popn(2);
> +			return;
> +		}
> +	}
> +
> +	for_each_present_cpu(cpu) {
> +		if (cpu == me)
> +			continue;
> +
> +		hartid = cpus[cpu].hartid;
> +
> +		hart_wait_on_status(hartid, SBI_EXT_HSM_STOPPED);
> +		hart_wait_on_status(hartid, SBI_EXT_HSM_START_PENDING);
> +		ret = sbi_hart_get_status(hartid);
> +		report(!ret.error && ret.value == SBI_EXT_HSM_STARTED,
> +		       "test hart with hartid %ld successfully started", hartid);

We should split the reporting to check ret.error and ret.value separately.

> +	}
> +
> +	for_each_present_cpu(cpu) {
> +		if (cpu == me)
> +			continue;
> +
> +		hartid = cpus[cpu].hartid;
> +
> +		while (!((per_hart_start_checks = READ_ONCE(sbi_hsm_hart_start_checks[hartid]))
> +			 & SBI_HSM_TEST_DONE))

This while condition would be a bit less ugly if we dropped
per_hart_start_checks and then just used hartid to index the array in the
three checks below.

> +			cpu_relax();
> +
> +		report(per_hart_start_checks & SBI_HSM_TEST_SATP,
> +		       "satp is zero for test hart %ld", hartid);
> +		report(per_hart_start_checks & SBI_HSM_TEST_SIE,
> +		       "sstatus.SIE is zero for test hart %ld", hartid);
> +		report(per_hart_start_checks & SBI_HSM_TEST_HARTID_A1,
> +		       "a0 and a1 are hartid for test hart %ld", hartid);
> +	}
> +
> +	report_prefix_pop();
> +
> +	report_prefix_push("hart_stop");
> +
> +	for_each_present_cpu(cpu) {
> +		if (cpu == me)
> +			continue;
> +
> +		hartid = cpus[cpu].hartid;
> +		WRITE_ONCE(sbi_hsm_stop_hart[hartid], true);

instead of the loop we could do

 barrier();
 memset(...);
 barrier();

if we don't mind setting the 'me' hart too.

> +	}
> +
> +	for_each_present_cpu(cpu) {
> +		if (cpu == me)
> +			continue;
> +
> +		hartid = cpus[cpu].hartid;
> +		hart_wait_on_status(hartid, SBI_EXT_HSM_STARTED);
> +		hart_wait_on_status(hartid, SBI_EXT_HSM_STOP_PENDING);
> +		ret = sbi_hart_get_status(hartid);
> +		report(!ret.error && (ret.value == SBI_EXT_HSM_STOPPED),

unnecessary (), anyway we should split the check for ret.error and
ret.value and output ret.error/value.  See the DBCN test for how
we do it. For example, the write_byte test has

        report(ret.error == SBI_SUCCESS, "write success (error=%ld)", ret.error);
        report(ret.value == 0, "expected ret.value (%ld)", ret.value);

> +		       "test hart %ld successfully stopped", hartid);
> +	}
> +
> +	/* Reset the stop flags so that we can reuse them after suspension tests */
> +	for_each_present_cpu(cpu) {
> +		if (cpu == me)
> +			continue;
> +
> +		hartid = cpus[cpu].hartid;
> +		WRITE_ONCE(sbi_hsm_stop_hart[hartid], false);

instead of the loop we could do

 barrier();
 memset(...);
 barrier();

since we definitely we don't mind setting the 'me' hart too as it's
getting set to zero.

> +	}
> +
> +	report_prefix_pop();
> +
> +	report_prefix_push("hart_start");
> +
> +	on_secondary_cpus_async(hart_execute, NULL);
> +
> +	for_each_present_cpu(cpu) {
> +		if (cpu == me)
> +			continue;
> +
> +		hartid = cpus[cpu].hartid;
> +		hart_wait_on_status(hartid, SBI_EXT_HSM_STOPPED);
> +		hart_wait_on_status(hartid, SBI_EXT_HSM_START_PENDING);
> +		ret = sbi_hart_get_status(hartid);
> +		report(!ret.error && (ret.value == SBI_EXT_HSM_STARTED),
> +		       "new hart with hartid %ld successfully started", hartid);

split error/value tests

> +	}
> +
> +	while (!cpumask_test_secondary_cpus(&cpu_idle_mask))

while (cpumask_weight(&cpu_idle_mask) != nr_cpus - 1)

> +		cpu_relax();

We could get stuck in this loop forever if the cpus don't come up, so you
may want to set a timeout with timer_start(), then either handler the
failure due to the timeout handler firing or stop the timer, so

 timer_start(long_enough_duration);
 while (cpumask_weight(&cpu_idle_mask) != nr_cpus - 1)
    cpu_relax();
 timer_stop();
 if (timer_fired)
    report_fail(...)

> +
> +	report(cpumask_full(&cpu_online_mask), "all cpus online");
> +	report(cpumask_test_secondary_cpus(&cpus_alive_after_start),
> +	       "all secondary harts successfully executed code after start");

This test is covered by the idle mask check above. hart_execute() could
just be an empty function

> +
> +	report_prefix_pop();
> +
> +	report_prefix_push("hart_suspend");
> +
> +	if (sbi_probe(SBI_EXT_IPI)) {
> +		on_secondary_cpus_async(hart_retentive_suspend, NULL);
> +
> +		for_each_present_cpu(cpu) {
> +			if (cpu == me)
> +				continue;
> +
> +			hartid = cpus[cpu].hartid;
> +			hart_mask[hartid / BITS_PER_LONG] |= 1UL << hartid;
> +			hart_wait_on_status(hartid, SBI_EXT_HSM_STARTED);
> +			hart_wait_on_status(hartid, SBI_EXT_HSM_SUSPEND_PENDING);
> +			ret = sbi_hart_get_status(hartid);
> +			report(!ret.error && (ret.value == SBI_EXT_HSM_SUSPENDED),

split report

> +			       "hart %ld successfully retentive suspended", hartid);
> +		}
> +
> +		for (int i = 0; i < NR_CPUS / BITS_PER_LONG; ++i) {
> +			if (hart_mask[i]) {

using a real mask for hart_mask would allow using

  for_each_cpu(i, &hart_mask)

> +				ret = sbi_send_ipi(hart_mask[i], i * BITS_PER_LONG);

but, instead, let's provide the following (untested) function in lib/riscv/sbi.c

   struct sbiret sbi_send_ipi_cpumask(cpumask_t *mask)
   {
        struct sbiret ret;

	for (int i = 0; i < CPUMASK_NR_LONGS; ++i) {
	    if (cpumask_bits(mask)[i]) {
	       ret = sbi_send_ipi(cpumask_bits(mask)[i], i * BITS_PER_LONG);
	       if (ret.error)
	          break;
	    }
	}

	return ret;
   }

then you can drop your loop and just call the new API.

> +				if (ret.error) {
> +					ipi_failed = true;
> +					report_fail("got %ld when sending ipi to retentive suspended harts",
> +						    ret.error);
> +					break;
> +				}
> +			}
> +		}
> +
> +		if (!ipi_failed) {
> +			for_each_present_cpu(cpu) {
> +				if (cpu == me)
> +					continue;
> +
> +				hartid = cpus[cpu].hartid;
> +				hart_wait_on_status(hartid, SBI_EXT_HSM_SUSPENDED);
> +				hart_wait_on_status(hartid, SBI_EXT_HSM_RESUME_PENDING);
> +				ret = sbi_hart_get_status(hartid);
> +				report(!ret.error && (ret.value == SBI_EXT_HSM_STARTED),

split report

> +				       "hart %ld successfully retentive resumed", hartid);
> +			}
> +
> +			while (!cpumask_test_secondary_cpus(&cpu_idle_mask))
> +				cpu_relax();

Timed wait and use cpumask_weight

> +
> +			report(cpumask_full(&cpu_online_mask), "all cpus online");
> +			report(cpumask_test_secondary_cpus(&cpus_alive_after_retentive_suspend),
> +			       "all secondary harts successfully executed code after retentive suspend");

Already checked with the wait on idle.

> +		}
> +
> +		/* Reset the ipi_failed flag so that we can reuse it for non-retentive suspension tests */
> +		ipi_failed = false;
> +
> +		on_secondary_cpus_async(hart_non_retentive_suspend, NULL);
> +
> +		for_each_present_cpu(cpu) {
> +			if (cpu == me)
> +				continue;
> +
> +			hartid = cpus[cpu].hartid;
> +			hart_wait_on_status(hartid, SBI_EXT_HSM_STARTED);
> +			hart_wait_on_status(hartid, SBI_EXT_HSM_SUSPEND_PENDING);
> +			ret = sbi_hart_get_status(hartid);
> +			report(!ret.error && (ret.value == SBI_EXT_HSM_SUSPENDED),

split errors

> +			       "hart %ld successfully non-retentive suspended", hartid);
> +		}
> +
> +		for (int i = 0; i < NR_CPUS / BITS_PER_LONG; ++i) {
> +			if (hart_mask[i]) {

use real mask

> +				ret = sbi_send_ipi(hart_mask[i], i * BITS_PER_LONG);
> +				if (ret.error) {
> +					ipi_failed = true;
> +					report_fail("got %ld when sending ipi to non-retentive suspended harts",
> +						    ret.error);
> +					break;
> +				}
> +			}
> +		}
> +
> +		if (!ipi_failed) {
> +			for_each_present_cpu(cpu) {
> +				if (cpu == me)
> +					continue;
> +
> +				hartid = cpus[cpu].hartid;
> +				hart_wait_on_status(hartid, SBI_EXT_HSM_SUSPENDED);
> +				hart_wait_on_status(hartid, SBI_EXT_HSM_RESUME_PENDING);
> +				ret = sbi_hart_get_status(hartid);
> +				report(!ret.error && (ret.value == SBI_EXT_HSM_STARTED),

split errors

> +				       "hart %ld successfully non-retentive resumed", hartid);
> +			}
> +
> +			for_each_present_cpu(cpu) {
> +				if (cpu == me)
> +					continue;
> +
> +				hartid = cpus[cpu].hartid;
> +
> +				while (!((per_hart_non_retentive_suspend_checks =
> +					 READ_ONCE(sbi_hsm_non_retentive_hart_suspend_checks[hartid]))
> +					 & SBI_HSM_TEST_DONE))

Could drop the local variable and index the array below.

> +					cpu_relax();
> +
> +				report(per_hart_non_retentive_suspend_checks & SBI_HSM_TEST_SATP,
> +				       "satp is zero for test hart %ld", hartid);
> +				report(per_hart_non_retentive_suspend_checks & SBI_HSM_TEST_SIE,
> +				       "sstatus.SIE is zero for test hart %ld", hartid);
> +				report(per_hart_non_retentive_suspend_checks & SBI_HSM_TEST_HARTID_A1,
> +				       "a0 and a1 are hartid for test hart %ld", hartid);
> +			}
> +
> +			report_prefix_pop();
> +
> +			report_prefix_push("hart_stop");
> +
> +			for_each_present_cpu(cpu) {
> +				if (cpu == me)
> +					continue;
> +
> +				hartid = cpus[cpu].hartid;
> +				WRITE_ONCE(sbi_hsm_stop_hart[hartid], true);

barrier, memset, barrier?

> +			}
> +
> +			for_each_present_cpu(cpu) {
> +				if (cpu == me)
> +					continue;
> +
> +				hartid = cpus[cpu].hartid;
> +				hart_wait_on_status(hartid, SBI_EXT_HSM_STARTED);
> +				hart_wait_on_status(hartid, SBI_EXT_HSM_STOP_PENDING);
> +				ret = sbi_hart_get_status(hartid);
> +				report(!ret.error && (ret.value == SBI_EXT_HSM_STOPPED),

split errors

> +				       "test hart %ld successfully stopped", hartid);
> +			}
> +		}

We can avoid having to add a level of indention to all the code that
depends on SBI_EXT_IPI by either putting it all in a function and then
calling that when IPI is available or by doing

  if (!sbi_probe(SBI_EXT_IPI)) {
     report_skip("skipping suspension tests since ipi extension is unavailable");
     return;
  }

since all these tests are currently the last in this function.

> +	} else {
> +		report_skip("skipping suspension tests since ipi extension is unavailable");
> +	}
> +
> +	report_prefix_popn(2);
> +}
> +
>  int main(int argc, char **argv)
>  {
>  	if (argc > 1 && !strcmp(argv[1], "-h")) {
> @@ -435,6 +816,7 @@ int main(int argc, char **argv)
>  	report_prefix_push("sbi");
>  	check_base();
>  	check_time();
> +	check_hsm();
>  	check_dbcn();
>  
>  	return report_summary();
> -- 
> 2.43.0
>

Thanks,
drew

[kvm-unit-tests PATCH v2 4/4] riscv: sbi: Add tests for HSM extension

[Andrew Jones]

On Thu, Aug 29, 2024 at 03:30:26PM GMT, Andrew Jones wrote:
> On Mon, Aug 26, 2024 at 01:08:24AM GMT, James Raphael Tiovalen wrote:
...
> > +.section .data
> > +.balign PAGE_SIZE
> > +.global sbi_hsm_hart_start_checks
> > +sbi_hsm_hart_start_checks:			.space CONFIG_NR_CPUS
> > +.global sbi_hsm_non_retentive_hart_suspend_checks
> > +sbi_hsm_non_retentive_hart_suspend_checks:	.space CONFIG_NR_CPUS
> > +.global sbi_hsm_stop_hart
> > +sbi_hsm_stop_hart:				.space CONFIG_NR_CPUS
> 
> I don't think it should be necessary to create these arrays in assembly.
> We should be able to make global arrays in C in riscv/sbi.c and still
> access them from the assembly as you've done.
> 
> CONFIG_NR_CPUS will support all possible cpuids, but hartids have their
> own range and the code above is indexing these arrays by hartid. Since
> we should be able to define the arrays in C, then we could also either
> 
>  1) assert that max_hartid + 1 <= NR_CPUS
>  2) dynamically allocate the arrays using max_hartid + 1 for the size
>     and then assign global variables the physical addresses of those
>     allocated regions to use in the assembly
> 
> (1) is probably good enough

Actually we have to do (1) unless we want to open a big can of worms
because we're currently shoehorning hartids into cpumasks, but cpumasks
are based on NR_CPUS for size. To do it right, we should have hartmasks,
but they may be very large and/or sparse.

> 
> Seems on-cpus is missing an API for "all, but ...". How about, as a
> separate patch, adding
> 
>  void on_cpus_async(cpumask_t *mask, void (*func)(void *data), void *data)
> 
> to lib/on-cpus.c
> 
> Then here you'd copy the present mask to your own mask and clear 'me' from
> it for an 'all present, but me' mask.

I'll write a patch for on_cpus_async() now.

> but, instead, let's provide the following (untested) function in lib/riscv/sbi.c
> 
>    struct sbiret sbi_send_ipi_cpumask(cpumask_t *mask)
>    {
>         struct sbiret ret;
> 
> 	for (int i = 0; i < CPUMASK_NR_LONGS; ++i) {
> 	    if (cpumask_bits(mask)[i]) {
> 	       ret = sbi_send_ipi(cpumask_bits(mask)[i], i * BITS_PER_LONG);
> 	       if (ret.error)
> 	          break;
> 	    }
> 	}
> 
> 	return ret;
>    }
>

I'll write a patch adding this now too.

Thanks,
drew

Re: [kvm-unit-tests PATCH v2 4/4] riscv: sbi: Add tests for HSM extension

[Andrew Jones]

On Thu, Aug 29, 2024 at 03:30:26PM GMT, Andrew Jones wrote:
> On Mon, Aug 26, 2024 at 01:08:24AM GMT, James Raphael Tiovalen wrote:
...
> > +.section .data
> > +.balign PAGE_SIZE
> > +.global sbi_hsm_hart_start_checks
> > +sbi_hsm_hart_start_checks:			.space CONFIG_NR_CPUS
> > +.global sbi_hsm_non_retentive_hart_suspend_checks
> > +sbi_hsm_non_retentive_hart_suspend_checks:	.space CONFIG_NR_CPUS
> > +.global sbi_hsm_stop_hart
> > +sbi_hsm_stop_hart:				.space CONFIG_NR_CPUS
> 
> I don't think it should be necessary to create these arrays in assembly.
> We should be able to make global arrays in C in riscv/sbi.c and still
> access them from the assembly as you've done.
> 
> CONFIG_NR_CPUS will support all possible cpuids, but hartids have their
> own range and the code above is indexing these arrays by hartid. Since
> we should be able to define the arrays in C, then we could also either
> 
>  1) assert that max_hartid + 1 <= NR_CPUS
>  2) dynamically allocate the arrays using max_hartid + 1 for the size
>     and then assign global variables the physical addresses of those
>     allocated regions to use in the assembly
> 
> (1) is probably good enough

Actually we have to do (1) unless we want to open a big can of worms
because we're currently shoehorning hartids into cpumasks, but cpumasks
are based on NR_CPUS for size. To do it right, we should have hartmasks,
but they may be very large and/or sparse.

> 
> Seems on-cpus is missing an API for "all, but ...". How about, as a
> separate patch, adding
> 
>  void on_cpus_async(cpumask_t *mask, void (*func)(void *data), void *data)
> 
> to lib/on-cpus.c
> 
> Then here you'd copy the present mask to your own mask and clear 'me' from
> it for an 'all present, but me' mask.

I'll write a patch for on_cpus_async() now.

> but, instead, let's provide the following (untested) function in lib/riscv/sbi.c
> 
>    struct sbiret sbi_send_ipi_cpumask(cpumask_t *mask)
>    {
>         struct sbiret ret;
> 
> 	for (int i = 0; i < CPUMASK_NR_LONGS; ++i) {
> 	    if (cpumask_bits(mask)[i]) {
> 	       ret = sbi_send_ipi(cpumask_bits(mask)[i], i * BITS_PER_LONG);
> 	       if (ret.error)
> 	          break;
> 	    }
> 	}
> 
> 	return ret;
>    }
>

I'll write a patch adding this now too.

Thanks,
drew