Change in coreboot[master]: tests: Add lib/cbmem_console-test test case

[Name of user not set (Code Review)]

Name of user not set #1003143 has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/47642 )

Change subject: tests: Add lib/cbmem_console-test test case
......................................................................

tests: Add lib/cbmem_console-test test case

Add test case executed twice, once for ROMSTAGE and once RAMSTAGE.
Each test is named and visible in cmocka output with stage in its name.

Signed-off-by: Jakub Czapiga 
Change-Id: I464eee61f538188427bec730d2e004c7b76cca67
---
M tests/lib/Makefile.inc
A tests/lib/cbmem_console-test.c
2 files changed, 114 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/42/47642/1

diff --git a/tests/lib/Makefile.inc b/tests/lib/Makefile.inc
index 219f7e7..c498289 100644
--- a/tests/lib/Makefile.inc
+++ b/tests/lib/Makefile.inc
@@ -6,6 +6,8 @@
 tests-y += imd-test
 tests-y += timestamp-test
 tests-y += edid-test
+tests-y += cbmem_console-romstage-test
+tests-y += cbmem_console-ramstage-test

string-test-srcs += tests/lib/string-test.c
 string-test-srcs += src/lib/string.c
@@ -30,3 +32,10 @@
 edid-test-srcs += src/lib/edid.c
 edid-test-srcs += tests/stubs/console.c

+cbmem_console-romstage-test-stage := romstage
+cbmem_console-romstage-test-srcs += tests/lib/cbmem_console-test.c
+cbmem_console-romstage-test-srcs += tests/stubs/console.c
+
+cbmem_console-ramstage-test-stage := ramstage
+cbmem_console-ramstage-test-srcs += tests/lib/cbmem_console-test.c
+cbmem_console-ramstage-test-srcs += tests/stubs/console.c
\ No newline at end of file
diff --git a/tests/lib/cbmem_console-test.c b/tests/lib/cbmem_console-test.c
new file mode 100644
index 0000000..831f963
--- /dev/null
+++ b/tests/lib/cbmem_console-test.c
@@ -0,0 +1,105 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+#include "../lib/cbmem_console.c"
+
+#include <inttypes.h>
+#include <tests/test.h>
+
+
+#if ENV_ROMSTAGE_OR_BEFORE
+	/* Weak references have to be here, so TEST_REGION macro will work properly */
+	__weak extern u8 _preram_cbmem_console[];
+	__weak extern u8 _epreram_cbmem_console[];
+
+	#define PRERAM_CBMEM_CONSOLE_SIZE (1 * KiB)
+	TEST_REGION(preram_cbmem_console, PRERAM_CBMEM_CONSOLE_SIZE);
+#endif
+
+/* Disable init hooks. This test does not need them. */
+void cbmem_run_init_hooks(int is_recovery)
+{
+	(void)is_recovery;
+}
+
+int setup_cbmemc(void **state)
+{
+	cbmemc_init();
+	return 0;
+}
+
+int teardown_cbmemc(void **state)
+{
+	current_console->size = 0;
+	return 0;
+}
+
+void test_cbmemc_init(void **state)
+{
+	cbmemc_init();
+
+	/* Check if console structure was created. */
+	assert_non_null(current_console);
+}
+
+void test_cbmemc_tx_byte(void **state)
+{
+	int i;
+	u32 cursor;
+	const unsigned char data[] = "Random testing string\n"
+				"`1234567890-=~!@#$%^&*()_+\n"
+				"abcdefghijklmnopqrstuvwxyz\n"
+				"ABCDEFGHIJKLMNOPQRSTUVWXYZ\n";
+
+	for (i = 0; i < ARRAY_SIZE(data); ++i)
+		cbmemc_tx_byte(data[i]);
+
+	cursor = current_console->cursor & CURSOR_MASK;
+
+	assert_int_equal(ARRAY_SIZE(data), cursor);
+
+	/* Check if all characters were added correctly. */
+	assert_string_equal(data, current_console->body);
+}
+
+void test_cbmemc_tx_byte_overflow(void **state)
+{
+	int i;
+	u32 cursor;
+	u32 flags;
+	const uint32_t console_size = current_console->size;
+	const unsigned char data[] = "Another random string\n"
+				"abcdefghijklmnopqrstuvwxyz\n"
+				"ABCDEFGHIJKLMNOPQRSTUVWXYZ\n"
+				"`1234567890-=~!@#$%^&*()_+\n";
+
+	for (i = 0; i < console_size + ARRAY_SIZE(data); ++i)
+		cbmemc_tx_byte(data[i % ARRAY_SIZE(data)]);
+
+	cursor = current_console->cursor & CURSOR_MASK;
+	flags = current_console->cursor & ~CURSOR_MASK;
+
+	/* Check if there was a overflow in buffer */
+	assert_int_equal(OVERFLOW, flags & OVERFLOW);
+
+	/* Check if cursor got back to the beginning of a buffer */
+	assert_int_equal(ARRAY_SIZE(data), cursor);
+}
+
+int main(void)
+{
+#if ENV_ROMSTAGE_OR_BEFORE
+	const char *test_name = "cbmem_console-test-romstage";
+#else
+	const char *test_name = "cbmem_console-test-ramstage";
+#endif
+
+	const struct CMUnitTest tests[] = {
+		cmocka_unit_test_teardown(test_cbmemc_init, teardown_cbmemc),
+		cmocka_unit_test_setup_teardown(test_cbmemc_tx_byte,
+						setup_cbmemc, teardown_cbmemc),
+		cmocka_unit_test_setup_teardown(test_cbmemc_tx_byte_overflow,
+						setup_cbmemc, teardown_cbmemc),
+	};
+
+	return cmocka_run_group_tests_name(test_name, tests, NULL, NULL);
+}

[Paul Fagerburg (Code Review)]

Paul Fagerburg has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47642 )

Change subject: tests: Add lib/cbmem_console-test test case
......................................................................

Patch Set 1: Code-Review+1

(2 comments)

https://review.coreboot.org/c/coreboot/+/47642/1/tests/lib/cbmem_console-tes... 
File tests/lib/cbmem_console-test.c:

https://review.coreboot.org/c/coreboot/+/47642/1/tests/lib/cbmem_console-tes... 
PS1, Line 10: 	/* Weak references have to be here, so TEST_REGION macro will work properly */
            : 	__weak extern u8 _preram_cbmem_console[];
            : 	__weak extern u8 _epreram_cbmem_console[];
            : 
            : 	#define PRERAM_CBMEM_CONSOLE_SIZE (1 * KiB)
            : 	TEST_REGION(preram_cbmem_console, PRERAM_CBMEM_CONSOLE_SIZE);
style nit: these don't need to be indented inside a #if block

https://review.coreboot.org/c/coreboot/+/47642/1/tests/lib/cbmem_console-tes... 
PS1, Line 85: 	assert_int_equal(ARRAY_SIZE(data), cursor);
Should you check the contents to verify the correct behavior on overflow (keep oldest data or keep newest data)?

[Name of user not set (Code Review)]

Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Paul Fagerburg, Jan Dabros,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/47642

to look at the new patch set (#2).

Change subject: tests: Add lib/cbmem_console-test test case
......................................................................

tests: Add lib/cbmem_console-test test case

Add test case executed twice, once for ROMSTAGE and once RAMSTAGE.
Each test is named and visible in cmocka output with stage in its name.

Signed-off-by: Jakub Czapiga 
Change-Id: I464eee61f538188427bec730d2e004c7b76cca67
---
M tests/lib/Makefile.inc
A tests/lib/cbmem_console-test.c
2 files changed, 145 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/42/47642/2

[Name of user not set (Code Review)]

Name of user not set #1003143 has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47642 )

Change subject: tests: Add lib/cbmem_console-test test case
......................................................................

Patch Set 2:

(2 comments)

https://review.coreboot.org/c/coreboot/+/47642/1/tests/lib/cbmem_console-tes... 
File tests/lib/cbmem_console-test.c:

https://review.coreboot.org/c/coreboot/+/47642/1/tests/lib/cbmem_console-tes... 
PS1, Line 10: 	/* Weak references have to be here, so TEST_REGION macro will work properly */
            : 	__weak extern u8 _preram_cbmem_console[];
            : 	__weak extern u8 _epreram_cbmem_console[];
            : 
            : 	#define PRERAM_CBMEM_CONSOLE_SIZE (1 * KiB)
            : 	TEST_REGION(preram_cbmem_console, PRERAM_CBMEM_CONSOLE_SIZE);
> style nit: these don't need to be indented inside a #if block

Done

https://review.coreboot.org/c/coreboot/+/47642/1/tests/lib/cbmem_console-tes... 
PS1, Line 85: 	assert_int_equal(ARRAY_SIZE(data), cursor);
> Should you check the contents to verify the correct behavior on overflow (keep oldest data or keep n […]

Done

[Paul Fagerburg (Code Review)]

Paul Fagerburg has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47642 )

Change subject: tests: Add lib/cbmem_console-test test case
......................................................................

Patch Set 2: Code-Review+2

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/47642 )

Change subject: tests: Add lib/cbmem_console-test test case
......................................................................

tests: Add lib/cbmem_console-test test case

Add test case executed twice, once for ROMSTAGE and once RAMSTAGE.
Each test is named and visible in cmocka output with stage in its name.

Signed-off-by: Jakub Czapiga 
Change-Id: I464eee61f538188427bec730d2e004c7b76cca67
Reviewed-on: https://review.coreboot.org/c/coreboot/+/47642
Tested-by: build bot (Jenkins) 
Reviewed-by: Paul Fagerburg 
---
M tests/lib/Makefile.inc
A tests/lib/cbmem_console-test.c
2 files changed, 145 insertions(+), 0 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Paul Fagerburg: Looks good to me, approved

diff --git a/tests/lib/Makefile.inc b/tests/lib/Makefile.inc
index 219f7e7..c498289 100644
--- a/tests/lib/Makefile.inc
+++ b/tests/lib/Makefile.inc
@@ -6,6 +6,8 @@
 tests-y += imd-test
 tests-y += timestamp-test
 tests-y += edid-test
+tests-y += cbmem_console-romstage-test
+tests-y += cbmem_console-ramstage-test

string-test-srcs += tests/lib/string-test.c
 string-test-srcs += src/lib/string.c
@@ -30,3 +32,10 @@
 edid-test-srcs += src/lib/edid.c
 edid-test-srcs += tests/stubs/console.c

+cbmem_console-romstage-test-stage := romstage
+cbmem_console-romstage-test-srcs += tests/lib/cbmem_console-test.c
+cbmem_console-romstage-test-srcs += tests/stubs/console.c
+
+cbmem_console-ramstage-test-stage := ramstage
+cbmem_console-ramstage-test-srcs += tests/lib/cbmem_console-test.c
+cbmem_console-ramstage-test-srcs += tests/stubs/console.c
\ No newline at end of file
diff --git a/tests/lib/cbmem_console-test.c b/tests/lib/cbmem_console-test.c
new file mode 100644
index 0000000..604ca7a
--- /dev/null
+++ b/tests/lib/cbmem_console-test.c
@@ -0,0 +1,136 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+#include "../lib/cbmem_console.c"
+
+#include <inttypes.h>
+#include <stdlib.h>
+#include <string.h>
+#include <tests/test.h>
+
+
+#if ENV_ROMSTAGE_OR_BEFORE
+/* Weak references have to be here, so TEST_REGION macro will work properly */
+__weak extern u8 _preram_cbmem_console[];
+__weak extern u8 _epreram_cbmem_console[];
+
+#define PRERAM_CBMEM_CONSOLE_SIZE (1 * KiB)
+TEST_REGION(preram_cbmem_console, PRERAM_CBMEM_CONSOLE_SIZE);
+#endif
+
+/* Disable init hooks. This test does not need them. */
+void cbmem_run_init_hooks(int is_recovery)
+{
+	(void)is_recovery;
+}
+
+int setup_cbmemc(void **state)
+{
+	cbmemc_init();
+	return 0;
+}
+
+int teardown_cbmemc(void **state)
+{
+	current_console->size = 0;
+	return 0;
+}
+
+void test_cbmemc_init(void **state)
+{
+	cbmemc_init();
+
+	/* Check if console structure was created. */
+	assert_non_null(current_console);
+}
+
+void test_cbmemc_tx_byte(void **state)
+{
+	int i;
+	u32 cursor;
+	const unsigned char data[] = "Random testing string\n"
+				"`1234567890-=~!@#$%^&*()_+\n"
+				"abcdefghijklmnopqrstuvwxyz\n"
+				"ABCDEFGHIJKLMNOPQRSTUVWXYZ\n";
+
+	for (i = 0; i < ARRAY_SIZE(data); ++i)
+		cbmemc_tx_byte(data[i]);
+
+	cursor = current_console->cursor & CURSOR_MASK;
+
+	assert_int_equal(ARRAY_SIZE(data), cursor);
+
+	/* Check if all characters were added correctly. */
+	assert_memory_equal(data, current_console->body, ARRAY_SIZE(data));
+}
+
+void test_cbmemc_tx_byte_overflow(void **state)
+{
+	int i;
+	u32 cursor;
+	u32 flags;
+	const uint32_t console_size = current_console->size;
+	const unsigned char data[] = "Another random string\n"
+				"abcdefghijklmnopqrstuvwxyz\n"
+				"ABCDEFGHIJKLMNOPQRSTUVWXYZ\n"
+				"`1234567890-=~!@#$%^&*()_+\n";
+	const int data_size = ARRAY_SIZE(data) - 1;
+	const int data_stream_length = console_size + data_size;
+	const int overflow_bytes = data_stream_length % console_size;
+	unsigned char *check_buffer =
+			(unsigned char *)malloc(sizeof(unsigned char) * console_size);
+
+	/* Fill console buffer */
+	for (i = 0; i < console_size; ++i)
+		cbmemc_tx_byte(data[i % data_size]);
+
+	/* Store buffer for checking */
+	memcpy(check_buffer, current_console->body, console_size);
+
+	assert_memory_equal(current_console->body, data, overflow_bytes);
+
+	/* Add more data to console buffer to override older bytes */
+	for (; i < data_stream_length; ++i)
+		cbmemc_tx_byte(data[i % data_size]);
+
+	cursor = current_console->cursor & CURSOR_MASK;
+	flags = current_console->cursor & ~CURSOR_MASK;
+
+	/* Check if there was a overflow in buffer */
+	assert_int_equal(OVERFLOW, flags & OVERFLOW);
+
+	/* Check if cursor got back to the beginning of a buffer */
+	assert_int_equal(data_size, cursor);
+
+	/* Check if overflow buffer was overwritten */
+	assert_memory_not_equal(current_console->body,
+			data,
+			overflow_bytes);
+
+	/* Check if rest of the buffer contents, that should not be overridden,
+	 * is the same.
+	 */
+	assert_memory_equal(&current_console->body[overflow_bytes],
+			check_buffer + overflow_bytes,
+			console_size - overflow_bytes);
+
+	free(check_buffer);
+}
+
+int main(void)
+{
+#if ENV_ROMSTAGE_OR_BEFORE
+	const char *test_name = "cbmem_console-test-romstage";
+#else
+	const char *test_name = "cbmem_console-test-ramstage";
+#endif
+
+	const struct CMUnitTest tests[] = {
+		cmocka_unit_test_teardown(test_cbmemc_init, teardown_cbmemc),
+		cmocka_unit_test_setup_teardown(test_cbmemc_tx_byte,
+						setup_cbmemc, teardown_cbmemc),
+		cmocka_unit_test_setup_teardown(test_cbmemc_tx_byte_overflow,
+						setup_cbmemc, teardown_cbmemc),
+	};
+
+	return cmocka_run_group_tests_name(test_name, tests, NULL, NULL);
+}