[PATCH v3 0/4] SBIUnit: cover OpenSBI with tests

[PATCH v3 0/4] SBIUnit: cover OpenSBI with tests

[Ivan Orlov]

It is good when the code is covered with tests. Tests help us to keep
the code clean and avoid regressions. Also, a good test is always a nice
documentation for the code it covers.

This patch series introduces SBIUnit - the set of macros and functions
which simplify the unit test development for OpenSBI and automate tests
execution and evaluation.

This thing is mainly inspired by the KUnit framework from the Linux
Kernel, where the similar unit-test development tooling have been used
successfully for a pretty long time now. SBIUnit uses the same test
structure: multiple test cases are grouped together into the test
suites. SBIUnit also tries to reproduce the KUnit tests API while
allowing for some simplifications.

Another difference between the KUnit and SBIUnit is the location where
the tests are "stored". KUnit creates the ELF section in the kernel and
stores the pointers to all of the test suites there. SBIUnit takes
advantage of the 'carray' functionality of OpenSBI, keeping the pointers
to test suites in the auto-generated ".c" file. ELF section approach
could not be applied to SBIUnit, because OpenSBI could be used as a
static library when linking firmware. Use of a dedicated ELF section
would mean that all firmware linked with OpenSBI would need to have
this ELF section as well.

V1 -> V2:
- Add a cover letter
- Elaborate more on the differences between SBIUnit and KUnit
- Add a new patch adding a new entry to the 'clear' makefile target in
order to clear carray-generated files as well.

V2 -> V3:
- Drop the patch which modifies carray behavior (it will be included
into a separate patch series)
- (Patch-specific changes are described in the following patches)

Ivan Orlov (4):
  docs: Add documentation about tests and SBIUnit
  lib: Add SBIUnit testing macros and functions
  lib: tests: Add a test for sbi_bitmap
  lib: tests: Add sbi_console test

 docs/writing_tests.md         | 133 ++++++++++++++++++++++++++++++++++
 include/sbi/sbi_unit_test.h   |  71 ++++++++++++++++++
 lib/sbi/Kconfig               |   4 +
 lib/sbi/objects.mk            |   6 ++
 lib/sbi/sbi_bitmap_test.c     | 102 ++++++++++++++++++++++++++
 lib/sbi/sbi_console.c         |   4 +
 lib/sbi/sbi_console_test.c    | 101 ++++++++++++++++++++++++++
 lib/sbi/sbi_init.c            |   3 +
 lib/sbi/sbi_unit_test.c       |  43 +++++++++++
 lib/sbi/sbi_unit_tests.carray |   3 +
 10 files changed, 470 insertions(+)
 create mode 100644 docs/writing_tests.md
 create mode 100644 include/sbi/sbi_unit_test.h
 create mode 100644 lib/sbi/sbi_bitmap_test.c
 create mode 100644 lib/sbi/sbi_console_test.c
 create mode 100644 lib/sbi/sbi_unit_test.c
 create mode 100644 lib/sbi/sbi_unit_tests.carray

-- 
2.34.1

[PATCH v3 1/4] docs: Add documentation about tests and SBIUnit

[Ivan Orlov]

This patch contains the documentation for SBIUnit. It describes:

- What is SBIUnit
- Simple test writing scenario
- How we can cover static functions
- How we can "mock" structures in order to test the functions which
operate on them
- SBIUnit API Reference

Signed-off-by: Ivan Orlov <ivan.orlov0322@gmail.com>
---
V1 -> V2:
- Use more appropriate language
- Update the documentation in accordance with SBIUnit API and
structure changes
- Remove the redundant documentation for every SBIUNIT_* macro.
- Make the documentation follow the 80-characters rule.
V2 -> V3:
- Fix a few typos
- Provide the expected console output as it is, without following 80-chars
rule
- Update the example test source: use SBIUNIT_END_CASE macro instead of
"{}"
- Add the paragraph about the manual removal of the 'build/' directory
in order to regenerate carray-related files. Currently, that's the only
reliable way to regenerate them if carray Makefile variable changes, and
it is going to be fixed in the future.

 docs/writing_tests.md | 133 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 133 insertions(+)
 create mode 100644 docs/writing_tests.md

diff --git a/docs/writing_tests.md b/docs/writing_tests.md
new file mode 100644
index 0000000..a1f11e5
--- /dev/null
+++ b/docs/writing_tests.md
@@ -0,0 +1,133 @@
+Writing tests for OpenSBI
+=========================
+
+SBIUnit
+-------
+SBIUnit is a set of macros and functions which simplify the test development and
+automate the test execution and evaluation. All of the SBIUnit definitions are
+in the `include/sbi/sbi_unit_test.h` header file, and implementations are
+available in `lib/sbi/sbi_unit_test.c`.
+
+Simple SBIUnit test
+-------------------
+
+For instance, we would like to test the following function from
+`lib/sbi/sbi_string.c`:
+
+```c
+size_t sbi_strlen(const char *str)
+{
+	unsigned long ret = 0;
+
+	while (*str != '\0') {
+		ret++;
+		str++;
+	}
+
+	return ret;
+}
+```
+
+which calculates the string length.
+
+Create the file `lib/sbi/sbi_string_test.c` with the following content:
+
+```c
+#include <sbi/sbi_unit_test.h>
+#include <sbi/sbi_string.h>
+
+static void strlen_test(struct sbiunit_test_case *test)
+{
+	SBIUNIT_EXPECT_EQ(test, sbi_strlen("Hello"), 5);
+	SBIUNIT_EXPECT_EQ(test, sbi_strlen("Hell\0o"), 4);
+}
+
+static struct sbiunit_test_case string_test_cases[] = {
+	SBIUNIT_TEST_CASE(strlen_test),
+	SBIUNIT_END_CASE,
+};
+
+SBIUNIT_TEST_SUITE(string_test_suite, string_test_cases);
+```
+
+Then, add the corresponding Makefile entries to `lib/sbi/objects.mk`:
+```lang-makefile
+...
+libsbi-objs-$(CONFIG_SBIUNIT) += sbi_string_test.o
+carray-sbi_unit_tests-$(CONFIG_SBIUNIT) += string_test_suite
+```
+
+If you compiled OpenSBI with CONFIG_SBIUNIT enabled before, you may need to
+manually remove the build folder in order to regenerate the carray files:
+`rm -rf build/`.
+
+Recompile OpenSBI with CONFIG_SBIUNIT option enabled and run it in the QEMU.
+You will see something like this:
+```
+# make PLATFORM=generic run
+...
+# Running SBIUNIT tests #
+...
+## Running test suite: string_test_suite
+[PASSED] strlen_test
+1 PASSED / 0 FAILED / 1 TOTAL
+```
+
+Now let's try to change this test in the way that it will fail:
+
+```c
+- SBIUNIT_EXPECT_EQ(test, sbi_strlen("Hello"), 5);
++ SBIUNIT_EXPECT_EQ(test, sbi_strlen("Hello"), 100);
+```
+
+`make all` and `make run` it again:
+```
+...
+# Running SBIUNIT tests #
+...
+## Running test suite: string_test_suite
+[SBIUnit] [.../opensbi/lib/sbi/sbi_string_test.c:6]: strlen_test: Condition "(sbi_strlen("Hello")) == (100)" expected to be true!
+[FAILED] strlen_test
+0 PASSED / 1 FAILED / 1 TOTAL
+```
+Covering the static functions / using the static definitions
+------------------------------------------------------------
+
+SBIUnit also allows you to test static functions. In order to do so, simply
+include your test source in the file you would like to test. Complementing the
+example above, just add this to the
+`lib/sbi/sbi_string.c` file:
+
+```c
+#ifdef CONFIG_SBIUNIT
+#include "sbi_string_test.c"
+#endif
+```
+
+In this case you should only add a new carray entry pointing to the test suite
+to `lib/sbi/objects.mk`:
+```lang-makefile
+...
+carray-sbi_unit_tests-$(CONFIG_SBIUNIT) += string_test_suite
+```
+
+You don't have to compile the `sbi_string_test.o` separately, because the
+test code will be included into the `sbi_string` object file.
+
+See example in `lib/sbi/sbi_console_test.c`, where statically declared
+`console_dev` variable is used to mock the `sbi_console_device` structure.
+
+"Mocking" the structures
+------------------------
+See the example of structure "mocking" in the `lib/sbi/sbi_console_test.c`,
+where the sbi_console_device structure was mocked to be used in various
+console-related functions in order to test them.
+
+API Reference
+-------------
+All of the `SBIUNIT_EXPECT_*` macros will cause a test case to fail if the
+corresponding conditions are not met, however, the execution of a particular
+test case will not be stopped.
+
+All of the `SBIUNIT_ASSERT_*` macros will cause a test case to fail and stop
+immediately, triggering a panic.
-- 
2.34.1

[PATCH v3 2/4] lib: Add SBIUnit testing macros and functions

[Ivan Orlov]

This patch introduces all of the SBIUnit macros and functions which
can be used during the test development process. Also, it defines
the 'run_all_tests' function, which is being called during the
'init_coldboot' right after printing the boot hart information.

Also, add the CONFIG_SBIUNIT Kconfig entry in order to be able to
turn the tests on and off. When the CONFIG_SBIUNIT is disabled,
the tests and all related code is excluded completely on the
compilation stage.

Signed-off-by: Ivan Orlov <ivan.orlov0322@gmail.com>
---
V1 -> V2:
- Rename the sources to have 'test' in the names: 'sbi_unit.c' =>
'sbi_unit_test.c', 'sbi_unit.h' => 'sbi_unit_test.h'.
- Rewrite using the carray functionality instead of placing the
pointers to the test suites into the source explicitly.
- Add 'ifdef' into the sbi_unit_test header file so we could
avoid writing a lot of 'ifdef' in other files.
- Get rid of unused symbols and macros
- Change the behaviour of SBIUNIT_ASSERT_* functions: now they trigger
sbi_panic if the assertion fails.
- Add a 'SBIUnit' prefix to the SBIUNIT_INFO macro message
- Rename the sbiunit_test_case structure fields, and use true/false
instead of 1/0
- Change the logic of the test fail detection: now we have the 'failed'
sbiunit_test_case structure field instead of the 'result'. 'failed'
field gets set to 'true' if an assertion or an expectaion fails.
- Fix codestyle issues
- Add 'len' parameter to SBIUNIT_*_STREQ
V2 -> V3:
- Remove redundant 'onerr' field from 'sbiunit_test_case' struct
- Remove setting of the 'failed' field for the test case struct in case
of assert (because panic() is called after that)
- Add 'SBIUNIT_END_CASE' macro, which should be placed at the end of a
test cases list
- Remove redundant 'enabled' field of the test case, as currently we
can't enable/disable tests separately; Now we iterate the test cases
until we face test_func == NULL

 include/sbi/sbi_unit_test.h   | 71 +++++++++++++++++++++++++++++++++++
 lib/sbi/Kconfig               |  4 ++
 lib/sbi/objects.mk            |  2 +
 lib/sbi/sbi_init.c            |  3 ++
 lib/sbi/sbi_unit_test.c       | 43 +++++++++++++++++++++
 lib/sbi/sbi_unit_tests.carray |  3 ++
 6 files changed, 126 insertions(+)
 create mode 100644 include/sbi/sbi_unit_test.h
 create mode 100644 lib/sbi/sbi_unit_test.c
 create mode 100644 lib/sbi/sbi_unit_tests.carray

diff --git a/include/sbi/sbi_unit_test.h b/include/sbi/sbi_unit_test.h
new file mode 100644
index 0000000..c63d900
--- /dev/null
+++ b/include/sbi/sbi_unit_test.h
@@ -0,0 +1,71 @@
+/*
+ * SPDX-License-Identifier: BSD-2-Clause
+ *
+ * Author: Ivan Orlov <ivan.orlov0322@gmail.com>
+ */
+#ifdef CONFIG_SBIUNIT
+#ifndef __SBI_UNIT_H__
+#define __SBI_UNIT_H__
+
+#include <sbi/sbi_types.h>
+#include <sbi/sbi_console.h>
+#include <sbi/sbi_string.h>
+
+struct sbiunit_test_case {
+	const char *name;
+	bool failed;
+	void (*test_func)(struct sbiunit_test_case *test);
+};
+
+struct sbiunit_test_suite {
+	const char *name;
+	struct sbiunit_test_case *cases;
+};
+
+#define SBIUNIT_TEST_CASE(func)		\
+	{				\
+		.name = #func,		\
+		.failed = false,	\
+		.test_func = (func)	\
+	}
+
+#define SBIUNIT_END_CASE { }
+
+#define SBIUNIT_TEST_SUITE(suite_name, cases_arr)		\
+	struct sbiunit_test_suite suite_name = {		\
+		.name = #suite_name,				\
+		.cases = cases_arr				\
+	}
+
+#define _sbiunit_msg(test, msg) "[SBIUnit] [%s:%d]: %s: %s", __FILE__,	\
+				__LINE__, test->name, msg
+
+#define SBIUNIT_INFO(test, msg) sbi_printf(_sbiunit_msg(test, msg))
+#define SBIUNIT_PANIC(test, msg) sbi_panic(_sbiunit_msg(test, msg))
+
+#define SBIUNIT_EXPECT(test, cond) do {							\
+	if (!(cond)) {									\
+		test->failed = true;							\
+		SBIUNIT_INFO(test, "Condition \"" #cond "\" expected to be true!\n");	\
+	}										\
+} while (0)
+
+#define SBIUNIT_ASSERT(test, cond) do {						\
+	if (!(cond))								\
+		SBIUNIT_PANIC(test, "Condition \"" #cond "\" must be true!\n");	\
+} while (0)
+
+#define SBIUNIT_EXPECT_EQ(test, a, b) SBIUNIT_EXPECT(test, (a) == (b))
+#define SBIUNIT_ASSERT_EQ(test, a, b) SBIUNIT_ASSERT(test, (a) == (b))
+#define SBIUNIT_EXPECT_NE(test, a, b) SBIUNIT_EXPECT(test, (a) != (b))
+#define SBIUNIT_ASSERT_NE(test, a, b) SBIUNIT_ASSERT(test, (a) != (b))
+#define SBIUNIT_EXPECT_MEMEQ(test, a, b, len) SBIUNIT_EXPECT(test, !sbi_memcmp(a, b, len))
+#define SBIUNIT_ASSERT_MEMEQ(test, a, b, len) SBIUNIT_ASSERT(test, !sbi_memcmp(a, b, len))
+#define SBIUNIT_EXPECT_STREQ(test, a, b, len) SBIUNIT_EXPECT(test, !sbi_strncmp(a, b, len))
+#define SBIUNIT_ASSERT_STREQ(test, a, b, len) SBIUNIT_ASSERT(test, !sbi_strncmp(a, b, len))
+
+void run_all_tests(void);
+#endif
+#else
+#define run_all_tests()
+#endif
diff --git a/lib/sbi/Kconfig b/lib/sbi/Kconfig
index 81dd2db..e3038ee 100644
--- a/lib/sbi/Kconfig
+++ b/lib/sbi/Kconfig
@@ -50,4 +50,8 @@ config SBI_ECALL_DBTR
 	bool "Debug Trigger Extension"
 	default y
 
+config SBIUNIT
+	bool "Enable SBIUNIT tests"
+	default n
+
 endmenu
diff --git a/lib/sbi/objects.mk b/lib/sbi/objects.mk
index 0a50e95..08959f1 100644
--- a/lib/sbi/objects.mk
+++ b/lib/sbi/objects.mk
@@ -11,6 +11,8 @@ libsbi-objs-y += riscv_asm.o
 libsbi-objs-y += riscv_atomic.o
 libsbi-objs-y += riscv_hardfp.o
 libsbi-objs-y += riscv_locks.o
+libsbi-objs-$(CONFIG_SBIUNIT) += sbi_unit_test.o
+libsbi-objs-$(CONFIG_SBIUNIT) += sbi_unit_tests.o
 
 libsbi-objs-y += sbi_ecall.o
 libsbi-objs-y += sbi_ecall_exts.o
diff --git a/lib/sbi/sbi_init.c b/lib/sbi/sbi_init.c
index 804b01c..796cccc 100644
--- a/lib/sbi/sbi_init.c
+++ b/lib/sbi/sbi_init.c
@@ -29,6 +29,7 @@
 #include <sbi/sbi_timer.h>
 #include <sbi/sbi_tlb.h>
 #include <sbi/sbi_version.h>
+#include <sbi/sbi_unit_test.h>
 
 #define BANNER                                              \
 	"   ____                    _____ ____ _____\n"     \
@@ -398,6 +399,8 @@ static void __noreturn init_coldboot(struct sbi_scratch *scratch, u32 hartid)
 
 	sbi_boot_print_hart(scratch, hartid);
 
+	run_all_tests();
+
 	/*
 	 * Configure PMP at last because if SMEPMP is detected,
 	 * M-mode access to the S/U space will be rescinded.
diff --git a/lib/sbi/sbi_unit_test.c b/lib/sbi/sbi_unit_test.c
new file mode 100644
index 0000000..1987838
--- /dev/null
+++ b/lib/sbi/sbi_unit_test.c
@@ -0,0 +1,43 @@
+/*
+ * SPDX-License-Identifier: BSD-2-Clause
+ *
+ * Author: Ivan Orlov <ivan.orlov0322@gmail.com>
+ */
+#include <sbi/sbi_unit_test.h>
+#include <sbi/sbi_types.h>
+#include <sbi/sbi_console.h>
+
+extern struct sbiunit_test_suite *sbi_unit_tests[];
+extern unsigned long sbi_unit_tests_size;
+
+static void run_test_suite(struct sbiunit_test_suite *suite)
+{
+	struct sbiunit_test_case *s_case;
+	u32 count_pass = 0, count_fail = 0;
+
+	sbi_printf("## Running test suite: %s\n", suite->name);
+
+	s_case = suite->cases;
+	while (s_case->test_func) {
+		s_case->test_func(s_case);
+		if (s_case->failed)
+			count_fail++;
+		else
+			count_pass++;
+		sbi_printf("[%s] %s\n", s_case->failed ? "FAILED" : "PASSED",
+			   s_case->name);
+		s_case++;
+	}
+	sbi_printf("%u PASSED / %u FAILED / %u TOTAL\n", count_pass, count_fail,
+		   count_pass + count_fail);
+}
+
+void run_all_tests(void)
+{
+	u32 i;
+
+	sbi_printf("\n# Running SBIUNIT tests #\n");
+
+	for (i = 0; i < sbi_unit_tests_size; i++)
+		run_test_suite(sbi_unit_tests[i]);
+}
diff --git a/lib/sbi/sbi_unit_tests.carray b/lib/sbi/sbi_unit_tests.carray
new file mode 100644
index 0000000..8d6069b
--- /dev/null
+++ b/lib/sbi/sbi_unit_tests.carray
@@ -0,0 +1,3 @@
+HEADER: sbi/sbi_unit_test.h
+TYPE: struct sbiunit_test_suite
+NAME: sbi_unit_tests
-- 
2.34.1

[PATCH v3 3/4] lib: tests: Add a test for sbi_bitmap

[Ivan Orlov]

Add test suite covering all of the functions from lib/sbi/sbi_bitmap.c:
__bitmap_and, __bitmap_or and __bitmap_xor.

Signed-off-by: Ivan Orlov <ivan.orlov0322@gmail.com>
---
V1 -> V2:
- Extract the part of the commit description to the cover letter
- Update to use the carray functionality instead of placing the pointer
to the test suite directly into the sources
- Fix codestyle issues
- Use the references to the data_* arrays when defining the expected
test results, instead of repeating the same numbers again and again
V2 -> V3:
- Use the SBIUNIT_END_CASE macro in the test cases list instead of '{}'
- Remove redundant 'include' statement

 lib/sbi/objects.mk        |   3 ++
 lib/sbi/sbi_bitmap_test.c | 102 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 105 insertions(+)
 create mode 100644 lib/sbi/sbi_bitmap_test.c

diff --git a/lib/sbi/objects.mk b/lib/sbi/objects.mk
index 08959f1..b4c273f 100644
--- a/lib/sbi/objects.mk
+++ b/lib/sbi/objects.mk
@@ -14,6 +14,9 @@ libsbi-objs-y += riscv_locks.o
 libsbi-objs-$(CONFIG_SBIUNIT) += sbi_unit_test.o
 libsbi-objs-$(CONFIG_SBIUNIT) += sbi_unit_tests.o
 
+libsbi-objs-$(CONFIG_SBIUNIT) += sbi_bitmap_test.o
+carray-sbi_unit_tests-$(CONFIG_SBIUNIT) += bitmap_test_suite
+
 libsbi-objs-y += sbi_ecall.o
 libsbi-objs-y += sbi_ecall_exts.o
 
diff --git a/lib/sbi/sbi_bitmap_test.c b/lib/sbi/sbi_bitmap_test.c
new file mode 100644
index 0000000..2563934
--- /dev/null
+++ b/lib/sbi/sbi_bitmap_test.c
@@ -0,0 +1,102 @@
+/*
+ * SPDX-License-Identifier: BSD-2-Clause
+ *
+ * Author: Ivan Orlov <ivan.orlov0322@gmail.com>
+ */
+#include <sbi/sbi_bitmap.h>
+#include <sbi/sbi_unit_test.h>
+
+#define DATA_SIZE sizeof(data_zero)
+#define DATA_BIT_SIZE (DATA_SIZE * 8)
+
+static u64 data_a[] = { 0xDEADBEEF, 0x00BAB10C, 0x1BADB002, 0xABADBABE };
+static u64 data_b[] = { 0xC00010FF, 0x00BAB10C, 0xBAAAAAAD, 0xBADDCAFE };
+static u64 data_zero[] = { 0, 0, 0, 0 };
+
+static void bitmap_and_test(struct sbiunit_test_case *test)
+{
+	u64 res[DATA_SIZE];
+	u64 a_and_b[] = { data_a[0] & data_b[0], data_a[1] & data_b[1],
+			  data_a[2] & data_b[2], data_a[3] & data_b[3] };
+
+	__bitmap_and(res, data_a, data_b, DATA_BIT_SIZE);
+	SBIUNIT_EXPECT_MEMEQ(test, res, a_and_b, DATA_SIZE);
+
+	/* a & a = a */
+	__bitmap_and(res, data_a, data_a, DATA_BIT_SIZE);
+	SBIUNIT_ASSERT_MEMEQ(test, res, data_a, DATA_SIZE);
+
+	/* a & 0 = 0 */
+	__bitmap_and(res, data_a, data_zero, DATA_BIT_SIZE);
+	SBIUNIT_EXPECT_MEMEQ(test, res, data_zero, DATA_SIZE);
+
+	/* 0 & 0 = 0 */
+	__bitmap_and(res, data_zero, data_zero, DATA_BIT_SIZE);
+	SBIUNIT_EXPECT_MEMEQ(test, res, data_zero, DATA_SIZE);
+
+	sbi_memcpy(res, data_zero, DATA_SIZE);
+	/* Cover zero 'bits' argument */
+	__bitmap_and(res, data_a, data_b, 0);
+	SBIUNIT_EXPECT_MEMEQ(test, res, data_zero, DATA_SIZE);
+}
+
+static void bitmap_or_test(struct sbiunit_test_case *test)
+{
+	u64 res[DATA_SIZE];
+	u64 a_or_b[] = { data_a[0] | data_b[0], data_a[1] | data_b[1],
+			 data_a[2] | data_b[2], data_a[3] | data_b[3] };
+
+	__bitmap_or(res, data_a, data_b, DATA_BIT_SIZE);
+	SBIUNIT_EXPECT_MEMEQ(test, res, a_or_b, DATA_SIZE);
+
+	/* a | a = a */
+	__bitmap_or(res, data_a, data_a, DATA_BIT_SIZE);
+	SBIUNIT_EXPECT_MEMEQ(test, res, data_a, DATA_SIZE);
+
+	/* a | 0 = a */
+	__bitmap_or(res, data_a, data_zero, DATA_BIT_SIZE);
+	SBIUNIT_EXPECT_MEMEQ(test, res, data_a, DATA_SIZE);
+
+	/* 0 | 0 = 0 */
+	__bitmap_or(res, data_zero, data_zero, DATA_BIT_SIZE);
+	SBIUNIT_EXPECT_MEMEQ(test, res, data_zero, DATA_SIZE);
+
+	sbi_memcpy(res, data_zero, DATA_SIZE);
+	__bitmap_or(res, data_a, data_b, 0);
+	SBIUNIT_EXPECT_MEMEQ(test, res, data_zero, DATA_SIZE);
+}
+
+static void bitmap_xor_test(struct sbiunit_test_case *test)
+{
+	u64 res[DATA_SIZE];
+	u64 a_xor_b[] = { data_a[0] ^ data_b[0], data_a[1] ^ data_b[1],
+			  data_a[2] ^ data_b[2], data_a[3] ^ data_b[3] };
+
+	__bitmap_xor(res, data_a, data_b, DATA_BIT_SIZE);
+	SBIUNIT_EXPECT_MEMEQ(test, res, a_xor_b, DATA_SIZE);
+
+	/* a ^ 0 = a */
+	__bitmap_xor(res, data_a, data_zero, DATA_BIT_SIZE);
+	SBIUNIT_EXPECT_MEMEQ(test, res, data_a, DATA_SIZE);
+
+	/* a ^ a = 0 */
+	__bitmap_xor(res, data_a, data_a, DATA_BIT_SIZE);
+	SBIUNIT_EXPECT_MEMEQ(test, res, data_zero, DATA_SIZE);
+
+	/* 0 ^ 0 = 0 */
+	__bitmap_xor(res, data_zero, data_zero, DATA_BIT_SIZE);
+	SBIUNIT_EXPECT_MEMEQ(test, res, data_zero, DATA_SIZE);
+
+	sbi_memcpy(res, data_zero, DATA_SIZE);
+	__bitmap_xor(res, data_a, data_b, 0);
+	SBIUNIT_EXPECT_MEMEQ(test, res, data_zero, DATA_SIZE);
+}
+
+static struct sbiunit_test_case bitmap_test_cases[] = {
+	SBIUNIT_TEST_CASE(bitmap_and_test),
+	SBIUNIT_TEST_CASE(bitmap_or_test),
+	SBIUNIT_TEST_CASE(bitmap_xor_test),
+	SBIUNIT_END_CASE,
+};
+
+SBIUNIT_TEST_SUITE(bitmap_test_suite, bitmap_test_cases);
-- 
2.34.1

[PATCH v3 4/4] lib: tests: Add sbi_console test

[Ivan Orlov]

Add the test suite covering some of the functions from
lib/sbi/sbi_console.c: putc, puts and printf. The test covers a variety
of format specifiers for printf and different strings and characters for
putc and puts.

In order to do that, the test "mocks" the sbi_console_device structure
by setting the 'console_dev' variable to the virtual console.

Signed-off-by: Ivan Orlov <ivan.orlov0322@gmail.com>
---
V1 -> V2:
- Rewrite using the carray functionality
- Replace CONSOLE_DO and CONSOLE_DO_RET macros with two inline
functions: one of them "mocks" the default console device, and
the second one restores the old console device.
- Fix codestyle issues (comments, etc.)
- Remove incorrect 'puts' test
- Use updated SBIUNIT_ASSERT_STREQ API
V2 -> V3:
- Remove unused include statement
- Rename "new_dev" => "test_console_dev"
- Use SBIUNIT_END_CASE macro in the test cases list instead of '{}'

 lib/sbi/objects.mk         |   1 +
 lib/sbi/sbi_console.c      |   4 ++
 lib/sbi/sbi_console_test.c | 101 +++++++++++++++++++++++++++++++++++++
 3 files changed, 106 insertions(+)
 create mode 100644 lib/sbi/sbi_console_test.c

diff --git a/lib/sbi/objects.mk b/lib/sbi/objects.mk
index b4c273f..9d065fa 100644
--- a/lib/sbi/objects.mk
+++ b/lib/sbi/objects.mk
@@ -16,6 +16,7 @@ libsbi-objs-$(CONFIG_SBIUNIT) += sbi_unit_tests.o
 
 libsbi-objs-$(CONFIG_SBIUNIT) += sbi_bitmap_test.o
 carray-sbi_unit_tests-$(CONFIG_SBIUNIT) += bitmap_test_suite
+carray-sbi_unit_tests-$(CONFIG_SBIUNIT) += console_test_suite
 
 libsbi-objs-y += sbi_ecall.o
 libsbi-objs-y += sbi_ecall_exts.o
diff --git a/lib/sbi/sbi_console.c b/lib/sbi/sbi_console.c
index ab09a5c..d1229d0 100644
--- a/lib/sbi/sbi_console.c
+++ b/lib/sbi/sbi_console.c
@@ -488,3 +488,7 @@ int sbi_console_init(struct sbi_scratch *scratch)
 
 	return rc;
 }
+
+#ifdef CONFIG_SBIUNIT
+#include "sbi_console_test.c"
+#endif
diff --git a/lib/sbi/sbi_console_test.c b/lib/sbi/sbi_console_test.c
new file mode 100644
index 0000000..734a68c
--- /dev/null
+++ b/lib/sbi/sbi_console_test.c
@@ -0,0 +1,101 @@
+/*
+ * SPDX-License-Identifier: BSD-2-Clause
+ *
+ * Author: Ivan Orlov <ivan.orlov0322@gmail.com>
+ */
+#include <sbi/sbi_unit_test.h>
+
+#define TEST_CONSOLE_BUF_LEN 1024
+
+static const struct sbi_console_device *old_dev;
+static char test_console_buf[TEST_CONSOLE_BUF_LEN];
+static u32 test_console_buf_pos;
+
+static void test_console_putc(char c)
+{
+	test_console_buf[test_console_buf_pos] = c;
+	test_console_buf_pos = (test_console_buf_pos + 1) % TEST_CONSOLE_BUF_LEN;
+}
+
+static void clear_test_console_buf(void)
+{
+	test_console_buf_pos = 0;
+	test_console_buf[0] = '\0';
+}
+
+static const struct sbi_console_device test_console_dev = {
+	.name = "Test console device",
+	.console_putc = test_console_putc,
+};
+
+/* Mock the console device */
+static inline void test_console_begin(const struct sbi_console_device *device)
+{
+	old_dev = console_dev;
+	console_dev = device;
+}
+
+static inline void test_console_end(void)
+{
+	console_dev = old_dev;
+}
+
+static void putc_test(struct sbiunit_test_case *test)
+{
+	clear_test_console_buf();
+
+	test_console_begin(&test_console_dev);
+	sbi_putc('a');
+	test_console_end();
+	SBIUNIT_ASSERT_EQ(test, test_console_buf[0], 'a');
+}
+
+#define PUTS_TEST(test, expected, str) do {			\
+	clear_test_console_buf();				\
+	test_console_begin(&test_console_dev);			\
+	sbi_puts(str);						\
+	test_console_end();					\
+	SBIUNIT_ASSERT_STREQ(test, test_console_buf, expected,	\
+			     sbi_strlen(expected));		\
+} while (0)
+
+static void puts_test(struct sbiunit_test_case *test)
+{
+	PUTS_TEST(test, "Hello, OpenSBI!", "Hello, OpenSBI!");
+	PUTS_TEST(test, "Hello,\r\nOpenSBI!", "Hello,\nOpenSBI!");
+}
+
+#define PRINTF_TEST(test, expected, format, ...) do {		\
+	clear_test_console_buf();				\
+	test_console_begin(&test_console_dev);			\
+	size_t __res = sbi_printf(format, ##__VA_ARGS__);	\
+	test_console_end();					\
+	SBIUNIT_ASSERT_EQ(test, __res, sbi_strlen(expected));	\
+	SBIUNIT_ASSERT_STREQ(test, test_console_buf, expected,	\
+			     sbi_strlen(expected));		\
+} while (0)
+
+static void printf_test(struct sbiunit_test_case *test)
+{
+	PRINTF_TEST(test, "Hello", "Hello");
+	PRINTF_TEST(test, "3 5 7", "%d %d %d", 3, 5, 7);
+	PRINTF_TEST(test, "Hello", "%s", "Hello");
+	PRINTF_TEST(test, "-1", "%d", -1);
+	PRINTF_TEST(test, "FF", "%X", 255);
+	PRINTF_TEST(test, "ff", "%x", 255);
+	PRINTF_TEST(test, "A", "%c", 'A');
+	PRINTF_TEST(test, "1fe", "%p", (void *)0x1fe);
+	PRINTF_TEST(test, "4294967295", "%u", 4294967295U);
+	PRINTF_TEST(test, "-2147483647", "%ld", -2147483647l);
+	PRINTF_TEST(test, "-9223372036854775807", "%lld", -9223372036854775807LL);
+	PRINTF_TEST(test, "18446744073709551615", "%llu", 18446744073709551615ULL);
+}
+
+static struct sbiunit_test_case console_test_cases[] = {
+	SBIUNIT_TEST_CASE(putc_test),
+	SBIUNIT_TEST_CASE(puts_test),
+	SBIUNIT_TEST_CASE(printf_test),
+	SBIUNIT_END_CASE,
+};
+
+SBIUNIT_TEST_SUITE(console_test_suite, console_test_cases);
-- 
2.34.1

[PATCH v3 1/4] docs: Add documentation about tests and SBIUnit

[Andrew Jones]

On Fri, Mar 01, 2024 at 04:00:42PM +0000, Ivan Orlov wrote:
> This patch contains the documentation for SBIUnit. It describes:
> 
> - What is SBIUnit
> - Simple test writing scenario
> - How we can cover static functions
> - How we can "mock" structures in order to test the functions which
> operate on them
> - SBIUnit API Reference
> 
> Signed-off-by: Ivan Orlov <ivan.orlov0322@gmail.com>
> ---
> V1 -> V2:
> - Use more appropriate language
> - Update the documentation in accordance with SBIUnit API and
> structure changes
> - Remove the redundant documentation for every SBIUNIT_* macro.
> - Make the documentation follow the 80-characters rule.
> V2 -> V3:
> - Fix a few typos
> - Provide the expected console output as it is, without following 80-chars
> rule
> - Update the example test source: use SBIUNIT_END_CASE macro instead of
> "{}"
> - Add the paragraph about the manual removal of the 'build/' directory
> in order to regenerate carray-related files. Currently, that's the only
> reliable way to regenerate them if carray Makefile variable changes, and
> it is going to be fixed in the future.
> 
>  docs/writing_tests.md | 133 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 133 insertions(+)
>  create mode 100644 docs/writing_tests.md
> 
> diff --git a/docs/writing_tests.md b/docs/writing_tests.md
> new file mode 100644
> index 0000000..a1f11e5
> --- /dev/null
> +++ b/docs/writing_tests.md
> @@ -0,0 +1,133 @@
> +Writing tests for OpenSBI
> +=========================
> +
> +SBIUnit
> +-------
> +SBIUnit is a set of macros and functions which simplify the test development and
> +automate the test execution and evaluation. All of the SBIUnit definitions are
> +in the `include/sbi/sbi_unit_test.h` header file, and implementations are
> +available in `lib/sbi/sbi_unit_test.c`.
> +
> +Simple SBIUnit test
> +-------------------
> +
> +For instance, we would like to test the following function from
> +`lib/sbi/sbi_string.c`:
> +
> +```c
> +size_t sbi_strlen(const char *str)
> +{
> +	unsigned long ret = 0;
> +
> +	while (*str != '\0') {
> +		ret++;
> +		str++;
> +	}
> +
> +	return ret;
> +}
> +```
> +
> +which calculates the string length.
> +
> +Create the file `lib/sbi/sbi_string_test.c` with the following content:
> +
> +```c
> +#include <sbi/sbi_unit_test.h>
> +#include <sbi/sbi_string.h>
> +
> +static void strlen_test(struct sbiunit_test_case *test)
> +{
> +	SBIUNIT_EXPECT_EQ(test, sbi_strlen("Hello"), 5);
> +	SBIUNIT_EXPECT_EQ(test, sbi_strlen("Hell\0o"), 4);
> +}
> +
> +static struct sbiunit_test_case string_test_cases[] = {
> +	SBIUNIT_TEST_CASE(strlen_test),
> +	SBIUNIT_END_CASE,
> +};
> +
> +SBIUNIT_TEST_SUITE(string_test_suite, string_test_cases);
> +```
> +
> +Then, add the corresponding Makefile entries to `lib/sbi/objects.mk`:
> +```lang-makefile
> +...
> +libsbi-objs-$(CONFIG_SBIUNIT) += sbi_string_test.o
> +carray-sbi_unit_tests-$(CONFIG_SBIUNIT) += string_test_suite
> +```
> +
> +If you compiled OpenSBI with CONFIG_SBIUNIT enabled before, you may need to
> +manually remove the build folder in order to regenerate the carray files:
> +`rm -rf build/`.
> +
> +Recompile OpenSBI with CONFIG_SBIUNIT option enabled and run it in the QEMU.

s/CONFIG_SBIUNIT/the CONFIG_SBIUNIT/
s/the QEMU/QEMU/

> +You will see something like this:
> +```
> +# make PLATFORM=generic run
> +...
> +# Running SBIUNIT tests #
> +...
> +## Running test suite: string_test_suite
> +[PASSED] strlen_test
> +1 PASSED / 0 FAILED / 1 TOTAL
> +```
> +
> +Now let's try to change this test in the way that it will fail:
> +
> +```c
> +- SBIUNIT_EXPECT_EQ(test, sbi_strlen("Hello"), 5);
> ++ SBIUNIT_EXPECT_EQ(test, sbi_strlen("Hello"), 100);
> +```
> +
> +`make all` and `make run` it again:
> +```
> +...
> +# Running SBIUNIT tests #
> +...
> +## Running test suite: string_test_suite
> +[SBIUnit] [.../opensbi/lib/sbi/sbi_string_test.c:6]: strlen_test: Condition "(sbi_strlen("Hello")) == (100)" expected to be true!
> +[FAILED] strlen_test
> +0 PASSED / 1 FAILED / 1 TOTAL
> +```
> +Covering the static functions / using the static definitions
> +------------------------------------------------------------
> +
> +SBIUnit also allows you to test static functions. In order to do so, simply
> +include your test source in the file you would like to test. Complementing the
> +example above, just add this to the
> +`lib/sbi/sbi_string.c` file:
> +
> +```c
> +#ifdef CONFIG_SBIUNIT
> +#include "sbi_string_test.c"
> +#endif
> +```
> +
> +In this case you should only add a new carray entry pointing to the test suite
> +to `lib/sbi/objects.mk`:
> +```lang-makefile
> +...
> +carray-sbi_unit_tests-$(CONFIG_SBIUNIT) += string_test_suite
> +```
> +
> +You don't have to compile the `sbi_string_test.o` separately, because the
> +test code will be included into the `sbi_string` object file.
> +
> +See example in `lib/sbi/sbi_console_test.c`, where statically declared
> +`console_dev` variable is used to mock the `sbi_console_device` structure.
> +
> +"Mocking" the structures
> +------------------------
> +See the example of structure "mocking" in the `lib/sbi/sbi_console_test.c`,
> +where the sbi_console_device structure was mocked to be used in various
> +console-related functions in order to test them.
> +
> +API Reference
> +-------------
> +All of the `SBIUNIT_EXPECT_*` macros will cause a test case to fail if the
> +corresponding conditions are not met, however, the execution of a particular
> +test case will not be stopped.
> +
> +All of the `SBIUNIT_ASSERT_*` macros will cause a test case to fail and stop
> +immediately, triggering a panic.
> -- 
> 2.34.1
>

Other than the 'the' removal/addition,

Reviewed-by: Andrew Jones <ajones@ventanamicro.com>

Thanks,
drew

[PATCH v3 2/4] lib: Add SBIUnit testing macros and functions

[Andrew Jones]

On Fri, Mar 01, 2024 at 04:00:43PM +0000, Ivan Orlov wrote:
> This patch introduces all of the SBIUnit macros and functions which
> can be used during the test development process. Also, it defines
> the 'run_all_tests' function, which is being called during the
> 'init_coldboot' right after printing the boot hart information.
> 
> Also, add the CONFIG_SBIUNIT Kconfig entry in order to be able to
> turn the tests on and off. When the CONFIG_SBIUNIT is disabled,
> the tests and all related code is excluded completely on the
> compilation stage.
> 
> Signed-off-by: Ivan Orlov <ivan.orlov0322@gmail.com>
> ---
> V1 -> V2:
> - Rename the sources to have 'test' in the names: 'sbi_unit.c' =>
> 'sbi_unit_test.c', 'sbi_unit.h' => 'sbi_unit_test.h'.
> - Rewrite using the carray functionality instead of placing the
> pointers to the test suites into the source explicitly.
> - Add 'ifdef' into the sbi_unit_test header file so we could
> avoid writing a lot of 'ifdef' in other files.
> - Get rid of unused symbols and macros
> - Change the behaviour of SBIUNIT_ASSERT_* functions: now they trigger
> sbi_panic if the assertion fails.
> - Add a 'SBIUnit' prefix to the SBIUNIT_INFO macro message
> - Rename the sbiunit_test_case structure fields, and use true/false
> instead of 1/0
> - Change the logic of the test fail detection: now we have the 'failed'
> sbiunit_test_case structure field instead of the 'result'. 'failed'
> field gets set to 'true' if an assertion or an expectaion fails.
> - Fix codestyle issues
> - Add 'len' parameter to SBIUNIT_*_STREQ
> V2 -> V3:
> - Remove redundant 'onerr' field from 'sbiunit_test_case' struct
> - Remove setting of the 'failed' field for the test case struct in case
> of assert (because panic() is called after that)
> - Add 'SBIUNIT_END_CASE' macro, which should be placed at the end of a
> test cases list
> - Remove redundant 'enabled' field of the test case, as currently we
> can't enable/disable tests separately; Now we iterate the test cases
> until we face test_func == NULL
> 
>  include/sbi/sbi_unit_test.h   | 71 +++++++++++++++++++++++++++++++++++
>  lib/sbi/Kconfig               |  4 ++
>  lib/sbi/objects.mk            |  2 +
>  lib/sbi/sbi_init.c            |  3 ++
>  lib/sbi/sbi_unit_test.c       | 43 +++++++++++++++++++++
>  lib/sbi/sbi_unit_tests.carray |  3 ++
>  6 files changed, 126 insertions(+)
>  create mode 100644 include/sbi/sbi_unit_test.h
>  create mode 100644 lib/sbi/sbi_unit_test.c
>  create mode 100644 lib/sbi/sbi_unit_tests.carray
> 
> diff --git a/include/sbi/sbi_unit_test.h b/include/sbi/sbi_unit_test.h
> new file mode 100644
> index 0000000..c63d900
> --- /dev/null
> +++ b/include/sbi/sbi_unit_test.h
> @@ -0,0 +1,71 @@
> +/*
> + * SPDX-License-Identifier: BSD-2-Clause
> + *
> + * Author: Ivan Orlov <ivan.orlov0322@gmail.com>
> + */
> +#ifdef CONFIG_SBIUNIT
> +#ifndef __SBI_UNIT_H__
> +#define __SBI_UNIT_H__
> +
> +#include <sbi/sbi_types.h>
> +#include <sbi/sbi_console.h>
> +#include <sbi/sbi_string.h>
> +
> +struct sbiunit_test_case {
> +	const char *name;
> +	bool failed;
> +	void (*test_func)(struct sbiunit_test_case *test);
> +};
> +
> +struct sbiunit_test_suite {
> +	const char *name;
> +	struct sbiunit_test_case *cases;
> +};
> +
> +#define SBIUNIT_TEST_CASE(func)		\
> +	{				\
> +		.name = #func,		\
> +		.failed = false,	\
> +		.test_func = (func)	\
> +	}
> +
> +#define SBIUNIT_END_CASE { }
> +
> +#define SBIUNIT_TEST_SUITE(suite_name, cases_arr)		\
> +	struct sbiunit_test_suite suite_name = {		\
> +		.name = #suite_name,				\
> +		.cases = cases_arr				\
> +	}
> +
> +#define _sbiunit_msg(test, msg) "[SBIUnit] [%s:%d]: %s: %s", __FILE__,	\
> +				__LINE__, test->name, msg
> +
> +#define SBIUNIT_INFO(test, msg) sbi_printf(_sbiunit_msg(test, msg))
> +#define SBIUNIT_PANIC(test, msg) sbi_panic(_sbiunit_msg(test, msg))
> +
> +#define SBIUNIT_EXPECT(test, cond) do {							\
> +	if (!(cond)) {									\
> +		test->failed = true;							\
> +		SBIUNIT_INFO(test, "Condition \"" #cond "\" expected to be true!\n");	\
> +	}										\
> +} while (0)
> +
> +#define SBIUNIT_ASSERT(test, cond) do {						\
> +	if (!(cond))								\
> +		SBIUNIT_PANIC(test, "Condition \"" #cond "\" must be true!\n");	\
> +} while (0)
> +
> +#define SBIUNIT_EXPECT_EQ(test, a, b) SBIUNIT_EXPECT(test, (a) == (b))
> +#define SBIUNIT_ASSERT_EQ(test, a, b) SBIUNIT_ASSERT(test, (a) == (b))
> +#define SBIUNIT_EXPECT_NE(test, a, b) SBIUNIT_EXPECT(test, (a) != (b))
> +#define SBIUNIT_ASSERT_NE(test, a, b) SBIUNIT_ASSERT(test, (a) != (b))
> +#define SBIUNIT_EXPECT_MEMEQ(test, a, b, len) SBIUNIT_EXPECT(test, !sbi_memcmp(a, b, len))
> +#define SBIUNIT_ASSERT_MEMEQ(test, a, b, len) SBIUNIT_ASSERT(test, !sbi_memcmp(a, b, len))
> +#define SBIUNIT_EXPECT_STREQ(test, a, b, len) SBIUNIT_EXPECT(test, !sbi_strncmp(a, b, len))
> +#define SBIUNIT_ASSERT_STREQ(test, a, b, len) SBIUNIT_ASSERT(test, !sbi_strncmp(a, b, len))
> +
> +void run_all_tests(void);
> +#endif
> +#else
> +#define run_all_tests()
> +#endif
> diff --git a/lib/sbi/Kconfig b/lib/sbi/Kconfig
> index 81dd2db..e3038ee 100644
> --- a/lib/sbi/Kconfig
> +++ b/lib/sbi/Kconfig
> @@ -50,4 +50,8 @@ config SBI_ECALL_DBTR
>  	bool "Debug Trigger Extension"
>  	default y
>  
> +config SBIUNIT
> +	bool "Enable SBIUNIT tests"
> +	default n
> +
>  endmenu
> diff --git a/lib/sbi/objects.mk b/lib/sbi/objects.mk
> index 0a50e95..08959f1 100644
> --- a/lib/sbi/objects.mk
> +++ b/lib/sbi/objects.mk
> @@ -11,6 +11,8 @@ libsbi-objs-y += riscv_asm.o
>  libsbi-objs-y += riscv_atomic.o
>  libsbi-objs-y += riscv_hardfp.o
>  libsbi-objs-y += riscv_locks.o
> +libsbi-objs-$(CONFIG_SBIUNIT) += sbi_unit_test.o
> +libsbi-objs-$(CONFIG_SBIUNIT) += sbi_unit_tests.o
>  
>  libsbi-objs-y += sbi_ecall.o
>  libsbi-objs-y += sbi_ecall_exts.o
> diff --git a/lib/sbi/sbi_init.c b/lib/sbi/sbi_init.c
> index 804b01c..796cccc 100644
> --- a/lib/sbi/sbi_init.c
> +++ b/lib/sbi/sbi_init.c
> @@ -29,6 +29,7 @@
>  #include <sbi/sbi_timer.h>
>  #include <sbi/sbi_tlb.h>
>  #include <sbi/sbi_version.h>
> +#include <sbi/sbi_unit_test.h>
>  
>  #define BANNER                                              \
>  	"   ____                    _____ ____ _____\n"     \
> @@ -398,6 +399,8 @@ static void __noreturn init_coldboot(struct sbi_scratch *scratch, u32 hartid)
>  
>  	sbi_boot_print_hart(scratch, hartid);
>  
> +	run_all_tests();
> +
>  	/*
>  	 * Configure PMP at last because if SMEPMP is detected,
>  	 * M-mode access to the S/U space will be rescinded.
> diff --git a/lib/sbi/sbi_unit_test.c b/lib/sbi/sbi_unit_test.c
> new file mode 100644
> index 0000000..1987838
> --- /dev/null
> +++ b/lib/sbi/sbi_unit_test.c
> @@ -0,0 +1,43 @@
> +/*
> + * SPDX-License-Identifier: BSD-2-Clause
> + *
> + * Author: Ivan Orlov <ivan.orlov0322@gmail.com>
> + */
> +#include <sbi/sbi_unit_test.h>
> +#include <sbi/sbi_types.h>
> +#include <sbi/sbi_console.h>
> +
> +extern struct sbiunit_test_suite *sbi_unit_tests[];
> +extern unsigned long sbi_unit_tests_size;
> +
> +static void run_test_suite(struct sbiunit_test_suite *suite)
> +{
> +	struct sbiunit_test_case *s_case;
> +	u32 count_pass = 0, count_fail = 0;
> +
> +	sbi_printf("## Running test suite: %s\n", suite->name);
> +
> +	s_case = suite->cases;
> +	while (s_case->test_func) {
> +		s_case->test_func(s_case);
> +		if (s_case->failed)
> +			count_fail++;
> +		else
> +			count_pass++;
> +		sbi_printf("[%s] %s\n", s_case->failed ? "FAILED" : "PASSED",
> +			   s_case->name);
> +		s_case++;
> +	}
> +	sbi_printf("%u PASSED / %u FAILED / %u TOTAL\n", count_pass, count_fail,
> +		   count_pass + count_fail);
> +}
> +
> +void run_all_tests(void)
> +{
> +	u32 i;
> +
> +	sbi_printf("\n# Running SBIUNIT tests #\n");
> +
> +	for (i = 0; i < sbi_unit_tests_size; i++)
> +		run_test_suite(sbi_unit_tests[i]);
> +}
> diff --git a/lib/sbi/sbi_unit_tests.carray b/lib/sbi/sbi_unit_tests.carray
> new file mode 100644
> index 0000000..8d6069b
> --- /dev/null
> +++ b/lib/sbi/sbi_unit_tests.carray
> @@ -0,0 +1,3 @@
> +HEADER: sbi/sbi_unit_test.h
> +TYPE: struct sbiunit_test_suite
> +NAME: sbi_unit_tests
> -- 
> 2.34.1
>

Reviewed-by: Andrew Jones <ajones@ventanamicro.com>

[PATCH v3 3/4] lib: tests: Add a test for sbi_bitmap

[Andrew Jones]

On Fri, Mar 01, 2024 at 04:00:44PM +0000, Ivan Orlov wrote:
> Add test suite covering all of the functions from lib/sbi/sbi_bitmap.c:
> __bitmap_and, __bitmap_or and __bitmap_xor.
> 
> Signed-off-by: Ivan Orlov <ivan.orlov0322@gmail.com>
> ---
> V1 -> V2:
> - Extract the part of the commit description to the cover letter
> - Update to use the carray functionality instead of placing the pointer
> to the test suite directly into the sources
> - Fix codestyle issues
> - Use the references to the data_* arrays when defining the expected
> test results, instead of repeating the same numbers again and again
> V2 -> V3:
> - Use the SBIUNIT_END_CASE macro in the test cases list instead of '{}'
> - Remove redundant 'include' statement
> 
>  lib/sbi/objects.mk        |   3 ++
>  lib/sbi/sbi_bitmap_test.c | 102 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 105 insertions(+)
>  create mode 100644 lib/sbi/sbi_bitmap_test.c
> 
> diff --git a/lib/sbi/objects.mk b/lib/sbi/objects.mk
> index 08959f1..b4c273f 100644
> --- a/lib/sbi/objects.mk
> +++ b/lib/sbi/objects.mk
> @@ -14,6 +14,9 @@ libsbi-objs-y += riscv_locks.o
>  libsbi-objs-$(CONFIG_SBIUNIT) += sbi_unit_test.o
>  libsbi-objs-$(CONFIG_SBIUNIT) += sbi_unit_tests.o
>  
> +libsbi-objs-$(CONFIG_SBIUNIT) += sbi_bitmap_test.o
> +carray-sbi_unit_tests-$(CONFIG_SBIUNIT) += bitmap_test_suite
> +
>  libsbi-objs-y += sbi_ecall.o
>  libsbi-objs-y += sbi_ecall_exts.o
>  
> diff --git a/lib/sbi/sbi_bitmap_test.c b/lib/sbi/sbi_bitmap_test.c
> new file mode 100644
> index 0000000..2563934
> --- /dev/null
> +++ b/lib/sbi/sbi_bitmap_test.c
> @@ -0,0 +1,102 @@
> +/*
> + * SPDX-License-Identifier: BSD-2-Clause
> + *
> + * Author: Ivan Orlov <ivan.orlov0322@gmail.com>
> + */
> +#include <sbi/sbi_bitmap.h>
> +#include <sbi/sbi_unit_test.h>
> +
> +#define DATA_SIZE sizeof(data_zero)
> +#define DATA_BIT_SIZE (DATA_SIZE * 8)
> +
> +static u64 data_a[] = { 0xDEADBEEF, 0x00BAB10C, 0x1BADB002, 0xABADBABE };
> +static u64 data_b[] = { 0xC00010FF, 0x00BAB10C, 0xBAAAAAAD, 0xBADDCAFE };
> +static u64 data_zero[] = { 0, 0, 0, 0 };
> +
> +static void bitmap_and_test(struct sbiunit_test_case *test)
> +{
> +	u64 res[DATA_SIZE];
> +	u64 a_and_b[] = { data_a[0] & data_b[0], data_a[1] & data_b[1],
> +			  data_a[2] & data_b[2], data_a[3] & data_b[3] };
> +
> +	__bitmap_and(res, data_a, data_b, DATA_BIT_SIZE);
> +	SBIUNIT_EXPECT_MEMEQ(test, res, a_and_b, DATA_SIZE);
> +
> +	/* a & a = a */
> +	__bitmap_and(res, data_a, data_a, DATA_BIT_SIZE);
> +	SBIUNIT_ASSERT_MEMEQ(test, res, data_a, DATA_SIZE);
> +
> +	/* a & 0 = 0 */
> +	__bitmap_and(res, data_a, data_zero, DATA_BIT_SIZE);
> +	SBIUNIT_EXPECT_MEMEQ(test, res, data_zero, DATA_SIZE);
> +
> +	/* 0 & 0 = 0 */
> +	__bitmap_and(res, data_zero, data_zero, DATA_BIT_SIZE);
> +	SBIUNIT_EXPECT_MEMEQ(test, res, data_zero, DATA_SIZE);
> +
> +	sbi_memcpy(res, data_zero, DATA_SIZE);
> +	/* Cover zero 'bits' argument */
> +	__bitmap_and(res, data_a, data_b, 0);
> +	SBIUNIT_EXPECT_MEMEQ(test, res, data_zero, DATA_SIZE);
> +}
> +
> +static void bitmap_or_test(struct sbiunit_test_case *test)
> +{
> +	u64 res[DATA_SIZE];
> +	u64 a_or_b[] = { data_a[0] | data_b[0], data_a[1] | data_b[1],
> +			 data_a[2] | data_b[2], data_a[3] | data_b[3] };
> +
> +	__bitmap_or(res, data_a, data_b, DATA_BIT_SIZE);
> +	SBIUNIT_EXPECT_MEMEQ(test, res, a_or_b, DATA_SIZE);
> +
> +	/* a | a = a */
> +	__bitmap_or(res, data_a, data_a, DATA_BIT_SIZE);
> +	SBIUNIT_EXPECT_MEMEQ(test, res, data_a, DATA_SIZE);
> +
> +	/* a | 0 = a */
> +	__bitmap_or(res, data_a, data_zero, DATA_BIT_SIZE);
> +	SBIUNIT_EXPECT_MEMEQ(test, res, data_a, DATA_SIZE);
> +
> +	/* 0 | 0 = 0 */
> +	__bitmap_or(res, data_zero, data_zero, DATA_BIT_SIZE);
> +	SBIUNIT_EXPECT_MEMEQ(test, res, data_zero, DATA_SIZE);
> +
> +	sbi_memcpy(res, data_zero, DATA_SIZE);
> +	__bitmap_or(res, data_a, data_b, 0);
> +	SBIUNIT_EXPECT_MEMEQ(test, res, data_zero, DATA_SIZE);
> +}
> +
> +static void bitmap_xor_test(struct sbiunit_test_case *test)
> +{
> +	u64 res[DATA_SIZE];
> +	u64 a_xor_b[] = { data_a[0] ^ data_b[0], data_a[1] ^ data_b[1],
> +			  data_a[2] ^ data_b[2], data_a[3] ^ data_b[3] };
> +
> +	__bitmap_xor(res, data_a, data_b, DATA_BIT_SIZE);
> +	SBIUNIT_EXPECT_MEMEQ(test, res, a_xor_b, DATA_SIZE);
> +
> +	/* a ^ 0 = a */
> +	__bitmap_xor(res, data_a, data_zero, DATA_BIT_SIZE);
> +	SBIUNIT_EXPECT_MEMEQ(test, res, data_a, DATA_SIZE);
> +
> +	/* a ^ a = 0 */
> +	__bitmap_xor(res, data_a, data_a, DATA_BIT_SIZE);
> +	SBIUNIT_EXPECT_MEMEQ(test, res, data_zero, DATA_SIZE);
> +
> +	/* 0 ^ 0 = 0 */
> +	__bitmap_xor(res, data_zero, data_zero, DATA_BIT_SIZE);
> +	SBIUNIT_EXPECT_MEMEQ(test, res, data_zero, DATA_SIZE);
> +
> +	sbi_memcpy(res, data_zero, DATA_SIZE);
> +	__bitmap_xor(res, data_a, data_b, 0);
> +	SBIUNIT_EXPECT_MEMEQ(test, res, data_zero, DATA_SIZE);
> +}
> +
> +static struct sbiunit_test_case bitmap_test_cases[] = {
> +	SBIUNIT_TEST_CASE(bitmap_and_test),
> +	SBIUNIT_TEST_CASE(bitmap_or_test),
> +	SBIUNIT_TEST_CASE(bitmap_xor_test),
> +	SBIUNIT_END_CASE,
> +};
> +
> +SBIUNIT_TEST_SUITE(bitmap_test_suite, bitmap_test_cases);
> -- 
> 2.34.1
>

Reviewed-by: Andrew Jones <ajones@ventanamicro.com>

[PATCH v3 4/4] lib: tests: Add sbi_console test

[Andrew Jones]

On Fri, Mar 01, 2024 at 04:00:45PM +0000, Ivan Orlov wrote:
> Add the test suite covering some of the functions from
> lib/sbi/sbi_console.c: putc, puts and printf. The test covers a variety
> of format specifiers for printf and different strings and characters for
> putc and puts.
> 
> In order to do that, the test "mocks" the sbi_console_device structure
> by setting the 'console_dev' variable to the virtual console.
> 
> Signed-off-by: Ivan Orlov <ivan.orlov0322@gmail.com>
> ---
> V1 -> V2:
> - Rewrite using the carray functionality
> - Replace CONSOLE_DO and CONSOLE_DO_RET macros with two inline
> functions: one of them "mocks" the default console device, and
> the second one restores the old console device.
> - Fix codestyle issues (comments, etc.)
> - Remove incorrect 'puts' test
> - Use updated SBIUNIT_ASSERT_STREQ API
> V2 -> V3:
> - Remove unused include statement
> - Rename "new_dev" => "test_console_dev"
> - Use SBIUNIT_END_CASE macro in the test cases list instead of '{}'
> 
>  lib/sbi/objects.mk         |   1 +
>  lib/sbi/sbi_console.c      |   4 ++
>  lib/sbi/sbi_console_test.c | 101 +++++++++++++++++++++++++++++++++++++
>  3 files changed, 106 insertions(+)
>  create mode 100644 lib/sbi/sbi_console_test.c
> 
> diff --git a/lib/sbi/objects.mk b/lib/sbi/objects.mk
> index b4c273f..9d065fa 100644
> --- a/lib/sbi/objects.mk
> +++ b/lib/sbi/objects.mk
> @@ -16,6 +16,7 @@ libsbi-objs-$(CONFIG_SBIUNIT) += sbi_unit_tests.o
>  
>  libsbi-objs-$(CONFIG_SBIUNIT) += sbi_bitmap_test.o
>  carray-sbi_unit_tests-$(CONFIG_SBIUNIT) += bitmap_test_suite
> +carray-sbi_unit_tests-$(CONFIG_SBIUNIT) += console_test_suite
>  
>  libsbi-objs-y += sbi_ecall.o
>  libsbi-objs-y += sbi_ecall_exts.o
> diff --git a/lib/sbi/sbi_console.c b/lib/sbi/sbi_console.c
> index ab09a5c..d1229d0 100644
> --- a/lib/sbi/sbi_console.c
> +++ b/lib/sbi/sbi_console.c
> @@ -488,3 +488,7 @@ int sbi_console_init(struct sbi_scratch *scratch)
>  
>  	return rc;
>  }
> +
> +#ifdef CONFIG_SBIUNIT
> +#include "sbi_console_test.c"
> +#endif
> diff --git a/lib/sbi/sbi_console_test.c b/lib/sbi/sbi_console_test.c
> new file mode 100644
> index 0000000..734a68c
> --- /dev/null
> +++ b/lib/sbi/sbi_console_test.c
> @@ -0,0 +1,101 @@
> +/*
> + * SPDX-License-Identifier: BSD-2-Clause
> + *
> + * Author: Ivan Orlov <ivan.orlov0322@gmail.com>
> + */
> +#include <sbi/sbi_unit_test.h>
> +
> +#define TEST_CONSOLE_BUF_LEN 1024
> +
> +static const struct sbi_console_device *old_dev;
> +static char test_console_buf[TEST_CONSOLE_BUF_LEN];
> +static u32 test_console_buf_pos;
> +
> +static void test_console_putc(char c)
> +{
> +	test_console_buf[test_console_buf_pos] = c;
> +	test_console_buf_pos = (test_console_buf_pos + 1) % TEST_CONSOLE_BUF_LEN;
> +}
> +
> +static void clear_test_console_buf(void)
> +{
> +	test_console_buf_pos = 0;
> +	test_console_buf[0] = '\0';
> +}
> +
> +static const struct sbi_console_device test_console_dev = {
> +	.name = "Test console device",
> +	.console_putc = test_console_putc,
> +};
> +
> +/* Mock the console device */
> +static inline void test_console_begin(const struct sbi_console_device *device)
> +{
> +	old_dev = console_dev;
> +	console_dev = device;
> +}
> +
> +static inline void test_console_end(void)
> +{
> +	console_dev = old_dev;
> +}
> +
> +static void putc_test(struct sbiunit_test_case *test)
> +{
> +	clear_test_console_buf();
> +

stray blank line

> +	test_console_begin(&test_console_dev);
> +	sbi_putc('a');
> +	test_console_end();
> +	SBIUNIT_ASSERT_EQ(test, test_console_buf[0], 'a');
> +}
> +
> +#define PUTS_TEST(test, expected, str) do {			\
> +	clear_test_console_buf();				\
> +	test_console_begin(&test_console_dev);			\
> +	sbi_puts(str);						\
> +	test_console_end();					\
> +	SBIUNIT_ASSERT_STREQ(test, test_console_buf, expected,	\
> +			     sbi_strlen(expected));		\
> +} while (0)
> +
> +static void puts_test(struct sbiunit_test_case *test)
> +{
> +	PUTS_TEST(test, "Hello, OpenSBI!", "Hello, OpenSBI!");
> +	PUTS_TEST(test, "Hello,\r\nOpenSBI!", "Hello,\nOpenSBI!");
> +}
> +
> +#define PRINTF_TEST(test, expected, format, ...) do {		\
> +	clear_test_console_buf();				\
> +	test_console_begin(&test_console_dev);			\
> +	size_t __res = sbi_printf(format, ##__VA_ARGS__);	\
> +	test_console_end();					\
> +	SBIUNIT_ASSERT_EQ(test, __res, sbi_strlen(expected));	\
> +	SBIUNIT_ASSERT_STREQ(test, test_console_buf, expected,	\
> +			     sbi_strlen(expected));		\
> +} while (0)
> +
> +static void printf_test(struct sbiunit_test_case *test)
> +{
> +	PRINTF_TEST(test, "Hello", "Hello");
> +	PRINTF_TEST(test, "3 5 7", "%d %d %d", 3, 5, 7);
> +	PRINTF_TEST(test, "Hello", "%s", "Hello");
> +	PRINTF_TEST(test, "-1", "%d", -1);
> +	PRINTF_TEST(test, "FF", "%X", 255);
> +	PRINTF_TEST(test, "ff", "%x", 255);
> +	PRINTF_TEST(test, "A", "%c", 'A');
> +	PRINTF_TEST(test, "1fe", "%p", (void *)0x1fe);
> +	PRINTF_TEST(test, "4294967295", "%u", 4294967295U);
> +	PRINTF_TEST(test, "-2147483647", "%ld", -2147483647l);
> +	PRINTF_TEST(test, "-9223372036854775807", "%lld", -9223372036854775807LL);
> +	PRINTF_TEST(test, "18446744073709551615", "%llu", 18446744073709551615ULL);
> +}
> +
> +static struct sbiunit_test_case console_test_cases[] = {
> +	SBIUNIT_TEST_CASE(putc_test),
> +	SBIUNIT_TEST_CASE(puts_test),
> +	SBIUNIT_TEST_CASE(printf_test),
> +	SBIUNIT_END_CASE,
> +};
> +
> +SBIUNIT_TEST_SUITE(console_test_suite, console_test_cases);
> -- 
> 2.34.1
>

Will there ever be a chance that more than one hart will run these tests
simultaneously? If so, we need to take a lock in PUTS_TEST and
PRINTF_TEST.

Thanks,
drew

[PATCH v3 4/4] lib: tests: Add sbi_console test

[Ivan Orlov]

On 3/4/24 13:44, Andrew Jones wrote:
> On Fri, Mar 01, 2024 at 04:00:45PM +0000, Ivan Orlov wrote:
>> Add the test suite covering some of the functions from
>> lib/sbi/sbi_console.c: putc, puts and printf. The test covers a variety
>> of format specifiers for printf and different strings and characters for
>> putc and puts.
>>
>> In order to do that, the test "mocks" the sbi_console_device structure
>> by setting the 'console_dev' variable to the virtual console.
>>
>> Signed-off-by: Ivan Orlov <ivan.orlov0322@gmail.com>
>> ---
>> V1 -> V2:
>> - Rewrite using the carray functionality
>> - Replace CONSOLE_DO and CONSOLE_DO_RET macros with two inline
>> functions: one of them "mocks" the default console device, and
>> the second one restores the old console device.
>> - Fix codestyle issues (comments, etc.)
>> - Remove incorrect 'puts' test
>> - Use updated SBIUNIT_ASSERT_STREQ API
>> V2 -> V3:
>> - Remove unused include statement
>> - Rename "new_dev" => "test_console_dev"
>> - Use SBIUNIT_END_CASE macro in the test cases list instead of '{}'
>>
>>   lib/sbi/objects.mk         |   1 +
>>   lib/sbi/sbi_console.c      |   4 ++
>>   lib/sbi/sbi_console_test.c | 101 +++++++++++++++++++++++++++++++++++++
>>   3 files changed, 106 insertions(+)
>>   create mode 100644 lib/sbi/sbi_console_test.c
>>
>> diff --git a/lib/sbi/objects.mk b/lib/sbi/objects.mk
>> index b4c273f..9d065fa 100644
>> --- a/lib/sbi/objects.mk
>> +++ b/lib/sbi/objects.mk
>> @@ -16,6 +16,7 @@ libsbi-objs-$(CONFIG_SBIUNIT) += sbi_unit_tests.o
>>   
>>   libsbi-objs-$(CONFIG_SBIUNIT) += sbi_bitmap_test.o
>>   carray-sbi_unit_tests-$(CONFIG_SBIUNIT) += bitmap_test_suite
>> +carray-sbi_unit_tests-$(CONFIG_SBIUNIT) += console_test_suite
>>   
>>   libsbi-objs-y += sbi_ecall.o
>>   libsbi-objs-y += sbi_ecall_exts.o
>> diff --git a/lib/sbi/sbi_console.c b/lib/sbi/sbi_console.c
>> index ab09a5c..d1229d0 100644
>> --- a/lib/sbi/sbi_console.c
>> +++ b/lib/sbi/sbi_console.c
>> @@ -488,3 +488,7 @@ int sbi_console_init(struct sbi_scratch *scratch)
>>   
>>   	return rc;
>>   }
>> +
>> +#ifdef CONFIG_SBIUNIT
>> +#include "sbi_console_test.c"
>> +#endif
>> diff --git a/lib/sbi/sbi_console_test.c b/lib/sbi/sbi_console_test.c
>> new file mode 100644
>> index 0000000..734a68c
>> --- /dev/null
>> +++ b/lib/sbi/sbi_console_test.c
>> @@ -0,0 +1,101 @@
>> +/*
>> + * SPDX-License-Identifier: BSD-2-Clause
>> + *
>> + * Author: Ivan Orlov <ivan.orlov0322@gmail.com>
>> + */
>> +#include <sbi/sbi_unit_test.h>
>> +
>> +#define TEST_CONSOLE_BUF_LEN 1024
>> +
>> +static const struct sbi_console_device *old_dev;
>> +static char test_console_buf[TEST_CONSOLE_BUF_LEN];
>> +static u32 test_console_buf_pos;
>> +
>> +static void test_console_putc(char c)
>> +{
>> +	test_console_buf[test_console_buf_pos] = c;
>> +	test_console_buf_pos = (test_console_buf_pos + 1) % TEST_CONSOLE_BUF_LEN;
>> +}
>> +
>> +static void clear_test_console_buf(void)
>> +{
>> +	test_console_buf_pos = 0;
>> +	test_console_buf[0] = '\0';
>> +}
>> +
>> +static const struct sbi_console_device test_console_dev = {
>> +	.name = "Test console device",
>> +	.console_putc = test_console_putc,
>> +};
>> +
>> +/* Mock the console device */
>> +static inline void test_console_begin(const struct sbi_console_device *device)
>> +{
>> +	old_dev = console_dev;
>> +	console_dev = device;
>> +}
>> +
>> +static inline void test_console_end(void)
>> +{
>> +	console_dev = old_dev;
>> +}
>> +
>> +static void putc_test(struct sbiunit_test_case *test)
>> +{
>> +	clear_test_console_buf();
>> +
> 
> stray blank line
> 
>> +	test_console_begin(&test_console_dev);
>> +	sbi_putc('a');
>> +	test_console_end();
>> +	SBIUNIT_ASSERT_EQ(test, test_console_buf[0], 'a');
>> +}
>> +
>> +#define PUTS_TEST(test, expected, str) do {			\
>> +	clear_test_console_buf();				\
>> +	test_console_begin(&test_console_dev);			\
>> +	sbi_puts(str);						\
>> +	test_console_end();					\
>> +	SBIUNIT_ASSERT_STREQ(test, test_console_buf, expected,	\
>> +			     sbi_strlen(expected));		\
>> +} while (0)
>> +
>> +static void puts_test(struct sbiunit_test_case *test)
>> +{
>> +	PUTS_TEST(test, "Hello, OpenSBI!", "Hello, OpenSBI!");
>> +	PUTS_TEST(test, "Hello,\r\nOpenSBI!", "Hello,\nOpenSBI!");
>> +}
>> +
>> +#define PRINTF_TEST(test, expected, format, ...) do {		\
>> +	clear_test_console_buf();				\
>> +	test_console_begin(&test_console_dev);			\
>> +	size_t __res = sbi_printf(format, ##__VA_ARGS__);	\
>> +	test_console_end();					\
>> +	SBIUNIT_ASSERT_EQ(test, __res, sbi_strlen(expected));	\
>> +	SBIUNIT_ASSERT_STREQ(test, test_console_buf, expected,	\
>> +			     sbi_strlen(expected));		\
>> +} while (0)
>> +
>> +static void printf_test(struct sbiunit_test_case *test)
>> +{
>> +	PRINTF_TEST(test, "Hello", "Hello");
>> +	PRINTF_TEST(test, "3 5 7", "%d %d %d", 3, 5, 7);
>> +	PRINTF_TEST(test, "Hello", "%s", "Hello");
>> +	PRINTF_TEST(test, "-1", "%d", -1);
>> +	PRINTF_TEST(test, "FF", "%X", 255);
>> +	PRINTF_TEST(test, "ff", "%x", 255);
>> +	PRINTF_TEST(test, "A", "%c", 'A');
>> +	PRINTF_TEST(test, "1fe", "%p", (void *)0x1fe);
>> +	PRINTF_TEST(test, "4294967295", "%u", 4294967295U);
>> +	PRINTF_TEST(test, "-2147483647", "%ld", -2147483647l);
>> +	PRINTF_TEST(test, "-9223372036854775807", "%lld", -9223372036854775807LL);
>> +	PRINTF_TEST(test, "18446744073709551615", "%llu", 18446744073709551615ULL);
>> +}
>> +
>> +static struct sbiunit_test_case console_test_cases[] = {
>> +	SBIUNIT_TEST_CASE(putc_test),
>> +	SBIUNIT_TEST_CASE(puts_test),
>> +	SBIUNIT_TEST_CASE(printf_test),
>> +	SBIUNIT_END_CASE,
>> +};
>> +
>> +SBIUNIT_TEST_SUITE(console_test_suite, console_test_cases);
>> -- 
>> 2.34.1
>>
> 
> Will there ever be a chance that more than one hart will run these tests
> simultaneously? If so, we need to take a lock in PUTS_TEST and
> PRINTF_TEST.
> 

Hi Andrew,

We call 'run_all_tests' function in 'init_coldboot', which runs on the 
coldboot hart only. If I understand it correctly (maybe I'm wrong), 
there could be only one coldboot hart in the system, so there is no 
chance that we will run tests on two harts simultaneously.

How do you think, should the reason why we don't use locking in the 
tests be documented somewhere? Or maybe we should use the locking 
anyway, just to make it more robust and avoid errors if we decide to run 
tests on multiple cores in the future? (but it is pretty hard for me to 
imagine why we should do that)

-- 
Kind regards,
Ivan Orlov

[PATCH v3 4/4] lib: tests: Add sbi_console test

[Andrew Jones]

On Mon, Mar 04, 2024 at 03:46:01PM +0000, Ivan Orlov wrote:
> On 3/4/24 13:44, Andrew Jones wrote:
> > On Fri, Mar 01, 2024 at 04:00:45PM +0000, Ivan Orlov wrote:
> > > Add the test suite covering some of the functions from
> > > lib/sbi/sbi_console.c: putc, puts and printf. The test covers a variety
> > > of format specifiers for printf and different strings and characters for
> > > putc and puts.
> > > 
> > > In order to do that, the test "mocks" the sbi_console_device structure
> > > by setting the 'console_dev' variable to the virtual console.
> > > 
> > > Signed-off-by: Ivan Orlov <ivan.orlov0322@gmail.com>
> > > ---
> > > V1 -> V2:
> > > - Rewrite using the carray functionality
> > > - Replace CONSOLE_DO and CONSOLE_DO_RET macros with two inline
> > > functions: one of them "mocks" the default console device, and
> > > the second one restores the old console device.
> > > - Fix codestyle issues (comments, etc.)
> > > - Remove incorrect 'puts' test
> > > - Use updated SBIUNIT_ASSERT_STREQ API
> > > V2 -> V3:
> > > - Remove unused include statement
> > > - Rename "new_dev" => "test_console_dev"
> > > - Use SBIUNIT_END_CASE macro in the test cases list instead of '{}'
> > > 
> > >   lib/sbi/objects.mk         |   1 +
> > >   lib/sbi/sbi_console.c      |   4 ++
> > >   lib/sbi/sbi_console_test.c | 101 +++++++++++++++++++++++++++++++++++++
> > >   3 files changed, 106 insertions(+)
> > >   create mode 100644 lib/sbi/sbi_console_test.c
> > > 
> > > diff --git a/lib/sbi/objects.mk b/lib/sbi/objects.mk
> > > index b4c273f..9d065fa 100644
> > > --- a/lib/sbi/objects.mk
> > > +++ b/lib/sbi/objects.mk
> > > @@ -16,6 +16,7 @@ libsbi-objs-$(CONFIG_SBIUNIT) += sbi_unit_tests.o
> > >   libsbi-objs-$(CONFIG_SBIUNIT) += sbi_bitmap_test.o
> > >   carray-sbi_unit_tests-$(CONFIG_SBIUNIT) += bitmap_test_suite
> > > +carray-sbi_unit_tests-$(CONFIG_SBIUNIT) += console_test_suite
> > >   libsbi-objs-y += sbi_ecall.o
> > >   libsbi-objs-y += sbi_ecall_exts.o
> > > diff --git a/lib/sbi/sbi_console.c b/lib/sbi/sbi_console.c
> > > index ab09a5c..d1229d0 100644
> > > --- a/lib/sbi/sbi_console.c
> > > +++ b/lib/sbi/sbi_console.c
> > > @@ -488,3 +488,7 @@ int sbi_console_init(struct sbi_scratch *scratch)
> > >   	return rc;
> > >   }
> > > +
> > > +#ifdef CONFIG_SBIUNIT
> > > +#include "sbi_console_test.c"
> > > +#endif
> > > diff --git a/lib/sbi/sbi_console_test.c b/lib/sbi/sbi_console_test.c
> > > new file mode 100644
> > > index 0000000..734a68c
> > > --- /dev/null
> > > +++ b/lib/sbi/sbi_console_test.c
> > > @@ -0,0 +1,101 @@
> > > +/*
> > > + * SPDX-License-Identifier: BSD-2-Clause
> > > + *
> > > + * Author: Ivan Orlov <ivan.orlov0322@gmail.com>
> > > + */
> > > +#include <sbi/sbi_unit_test.h>
> > > +
> > > +#define TEST_CONSOLE_BUF_LEN 1024
> > > +
> > > +static const struct sbi_console_device *old_dev;
> > > +static char test_console_buf[TEST_CONSOLE_BUF_LEN];
> > > +static u32 test_console_buf_pos;
> > > +
> > > +static void test_console_putc(char c)
> > > +{
> > > +	test_console_buf[test_console_buf_pos] = c;
> > > +	test_console_buf_pos = (test_console_buf_pos + 1) % TEST_CONSOLE_BUF_LEN;
> > > +}
> > > +
> > > +static void clear_test_console_buf(void)
> > > +{
> > > +	test_console_buf_pos = 0;
> > > +	test_console_buf[0] = '\0';
> > > +}
> > > +
> > > +static const struct sbi_console_device test_console_dev = {
> > > +	.name = "Test console device",
> > > +	.console_putc = test_console_putc,
> > > +};
> > > +
> > > +/* Mock the console device */
> > > +static inline void test_console_begin(const struct sbi_console_device *device)
> > > +{
> > > +	old_dev = console_dev;
> > > +	console_dev = device;
> > > +}
> > > +
> > > +static inline void test_console_end(void)
> > > +{
> > > +	console_dev = old_dev;
> > > +}
> > > +
> > > +static void putc_test(struct sbiunit_test_case *test)
> > > +{
> > > +	clear_test_console_buf();
> > > +
> > 
> > stray blank line
> > 
> > > +	test_console_begin(&test_console_dev);
> > > +	sbi_putc('a');
> > > +	test_console_end();
> > > +	SBIUNIT_ASSERT_EQ(test, test_console_buf[0], 'a');
> > > +}
> > > +
> > > +#define PUTS_TEST(test, expected, str) do {			\
> > > +	clear_test_console_buf();				\
> > > +	test_console_begin(&test_console_dev);			\
> > > +	sbi_puts(str);						\
> > > +	test_console_end();					\
> > > +	SBIUNIT_ASSERT_STREQ(test, test_console_buf, expected,	\
> > > +			     sbi_strlen(expected));		\
> > > +} while (0)
> > > +
> > > +static void puts_test(struct sbiunit_test_case *test)
> > > +{
> > > +	PUTS_TEST(test, "Hello, OpenSBI!", "Hello, OpenSBI!");
> > > +	PUTS_TEST(test, "Hello,\r\nOpenSBI!", "Hello,\nOpenSBI!");
> > > +}
> > > +
> > > +#define PRINTF_TEST(test, expected, format, ...) do {		\
> > > +	clear_test_console_buf();				\
> > > +	test_console_begin(&test_console_dev);			\
> > > +	size_t __res = sbi_printf(format, ##__VA_ARGS__);	\
> > > +	test_console_end();					\
> > > +	SBIUNIT_ASSERT_EQ(test, __res, sbi_strlen(expected));	\
> > > +	SBIUNIT_ASSERT_STREQ(test, test_console_buf, expected,	\
> > > +			     sbi_strlen(expected));		\
> > > +} while (0)
> > > +
> > > +static void printf_test(struct sbiunit_test_case *test)
> > > +{
> > > +	PRINTF_TEST(test, "Hello", "Hello");
> > > +	PRINTF_TEST(test, "3 5 7", "%d %d %d", 3, 5, 7);
> > > +	PRINTF_TEST(test, "Hello", "%s", "Hello");
> > > +	PRINTF_TEST(test, "-1", "%d", -1);
> > > +	PRINTF_TEST(test, "FF", "%X", 255);
> > > +	PRINTF_TEST(test, "ff", "%x", 255);
> > > +	PRINTF_TEST(test, "A", "%c", 'A');
> > > +	PRINTF_TEST(test, "1fe", "%p", (void *)0x1fe);
> > > +	PRINTF_TEST(test, "4294967295", "%u", 4294967295U);
> > > +	PRINTF_TEST(test, "-2147483647", "%ld", -2147483647l);
> > > +	PRINTF_TEST(test, "-9223372036854775807", "%lld", -9223372036854775807LL);
> > > +	PRINTF_TEST(test, "18446744073709551615", "%llu", 18446744073709551615ULL);
> > > +}
> > > +
> > > +static struct sbiunit_test_case console_test_cases[] = {
> > > +	SBIUNIT_TEST_CASE(putc_test),
> > > +	SBIUNIT_TEST_CASE(puts_test),
> > > +	SBIUNIT_TEST_CASE(printf_test),
> > > +	SBIUNIT_END_CASE,
> > > +};
> > > +
> > > +SBIUNIT_TEST_SUITE(console_test_suite, console_test_cases);
> > > -- 
> > > 2.34.1
> > > 
> > 
> > Will there ever be a chance that more than one hart will run these tests
> > simultaneously? If so, we need to take a lock in PUTS_TEST and
> > PRINTF_TEST.
> > 
> 
> Hi Andrew,
> 
> We call 'run_all_tests' function in 'init_coldboot', which runs on the
> coldboot hart only. If I understand it correctly (maybe I'm wrong), there
> could be only one coldboot hart in the system, so there is no chance that we
> will run tests on two harts simultaneously.
> 
> How do you think, should the reason why we don't use locking in the tests be
> documented somewhere? Or maybe we should use the locking anyway, just to
> make it more robust and avoid errors if we decide to run tests on multiple
> cores in the future? (but it is pretty hard for me to imagine why we should
> do that)
>

I'm not sure what's best, since I don't have a good feel for how this test
framework will evolve. I'd vote for adding a lock now, since it won't
hurt.

Thanks,
drew

[PATCH v3 4/4] lib: tests: Add sbi_console test

[Ivan Orlov]

On 3/4/24 16:33, Andrew Jones wrote:
> 
> I'm not sure what's best, since I don't have a good feel for how this test
> framework will evolve. I'd vote for adding a lock now, since it won't
> hurt.
> 

Alright, I'll add a spinlock in V4. Thanks!

-- 
Kind regards,
Ivan Orlov