s390x: factor out running of tests into common code

I want to use this in another file, so instead of replicating, factoring it out feels like the right thing to do.

Let's directly provide it for all architectures, they just have to implement get_time_ms() in asm/time.h to use it.

Signed-off-by David Hildenbrand <[email protected]>

---

The next step would be allowing to add custom parameters to a test. (defining them similar to the test cases).

Any other requests/nice to have? Does this make sense?

 lib/s390x/asm/time.h |  24 ++++++++++++
 lib/testrun.c        |  89 ++++++++++++++++++++++++++++++++++++++++++
 lib/testrun.h        |  41 ++++++++++++++++++++
 s390x/Makefile       |   1 +
 s390x/intercept.c    | 106 ++++++++-------------------------------------------
 5 files changed, 170 insertions(+), 91 deletions(-)
 create mode 100644 lib/s390x/asm/time.h
 create mode 100644 lib/testrun.c
 create mode 100644 lib/testrun.h

lib/s390x/asm/time.h new file +24 -0

new file mode 100644
index 0000000..856facd
@@ -0,0 +1,24 @@
+/*
+ * Copyright (c) 2017 Red Hat Inc
+ *
+ * Authors:
+ *  Thomas Huth <[email protected]>
+ *  David Hildenbrand <[email protected]>
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU Library General Public License version 2.
+ */
+#ifndef __ASMS390X_TIME_H
+#define __ASMS390X_TIME_H
+
+static inline uint64_t get_clock_ms(void)
+{
+	uint64_t clk;
+
+	asm volatile(" stck %0 " : : "Q"(clk) : "memory");
+
+	/* Bit 51 is incrememented each microsecond */
+	return (clk >> (63 - 51)) / 1000;
+}
+
+#endif

lib/testrun.c new file +89 -0

new file mode 100644
index 0000000..6034767
@@ -0,0 +1,89 @@
+/*
+ * Utility functions for running/configuring a set of tests
+ *
+ * Copyright (c) 2017 Red Hat Inc
+ *
+ * Authors:
+ *  Thomas Huth <[email protected]>
+ *  David Hildenbrand <[email protected]>
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU Library General Public License version 2.
+ */
+
+#include <testrun.h>
+#include <asm/time.h>
+
+void parse_test_args(const int argc, const char **argv, struct test *test)
+{
+	bool run_all = true;
+	int i, ti;
+
+	test->iterations = 0;
+	test->time_to_run = 0;
+
+	for (i = 1; i < argc; i++) {
+		if (!strcmp("-i", argv[i])) {
+			i++;
+			if (i >= argc)
+				report_abort("-i needs a parameter");
+			test->iterations = atol(argv[i]);
+		} else if (!strcmp("-t", argv[i])) {
+			i++;
+			if (i >= argc)
+				report_abort("-t needs a parameter");
+			test->time_to_run = atol(argv[i]);
+		} else for (ti = 0; test->test_cases[ti].name != NULL; ti++) {
+			if (!strcmp(test->test_cases[ti].name, argv[i])) {
+				run_all = false;
+				test->test_cases[ti].run_it = true;
+				break;
+			} else if (test->test_cases[ti + 1].name == NULL) {
+				report_abort("Unsupported parameter '%s'",
+					     argv[i]);
+			}
+		}
+	}
+
+	if (test->iterations == 0 && test->time_to_run == 0)
+		test->iterations = 1;
+
+	if ((test->iterations > 1 || test->time_to_run) &&
+	    !test->multirun_supported)
+		report_abort("Test does not support multiruns.");
+
+	if (run_all) {
+		for (ti = 0; test->test_cases[ti].name != NULL; ti++)
+			test->test_cases[ti].run_it = true;
+	}
+}
+
+int run_test(struct test *test)
+{
+	unsigned long iterations = test->iterations;
+	unsigned long time_to_run = test->time_to_run;
+	uint64_t startclk;
+	int ti;
+
+	report_prefix_push(test->name);
+	startclk = get_clock_ms();
+	for (;;) {
+		for (ti = 0; test->test_cases[ti].name != NULL; ti++) {
+			report_prefix_push(test->test_cases[ti].name);
+			if (test->test_cases[ti].run_it)
+				test->test_cases[ti].func();
+			report_prefix_pop();
+		}
+		if (iterations) {
+			iterations -= 1;
+			if (iterations == 0)
+				break;
+		}
+		if (time_to_run) {
+			if (get_clock_ms() - startclk > time_to_run)
+				break;
+		}
+	}
+	report_prefix_pop();
+	return report_summary();
+}

lib/testrun.h new file +41 -0

new file mode 100644
index 0000000..8f9a2d7
@@ -0,0 +1,41 @@
+/*
+ * Utility functions for running/configuring a set of test cases
+ *
+ * Copyright (c) 2017 Red Hat Inc
+ *
+ * Authors:
+ *  Thomas Huth <[email protected]>
+ *  David Hildenbrand <[email protected]>
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU Library General Public License version 2.
+ */
+#ifndef __TESTRUN_H
+#define __TESTRUN_H
+
+#include <libcflat.h>
+
+struct test_case {
+	const char *name;
+	void (*func)(void);
+	/* initialized by parse_test_args() */
+	bool run_it;
+};
+
+#define DEFINE_TEST_CASE(_name, _func) { _name, _func, false }
+#define DEFINE_TEST_CASE_END_OF_LIST() { }
+
+struct test {
+	/* to be initialized by the test */
+	const char *name;
+	bool multirun_supported;
+	struct test_case *test_cases;
+	/* initialized by parse_test_args() */
+	unsigned long iterations;
+	unsigned long time_to_run;
+};
+
+void parse_test_args(int argc, const char **argv, struct test *test);
+int run_test(struct test *test);
+
+#endif /* __TESTRUN_H */

s390x/Makefile +1 -0

index bc099da..e174895 100644
@@ -21,6 +21,7 @@ include $(SRCDIR)/scripts/asm-offsets.mak
 
 cflatobjs += lib/util.o
 cflatobjs += lib/alloc.o
+cflatobjs += lib/testrun.o
 cflatobjs += lib/s390x/io.o
 cflatobjs += lib/s390x/stack.o
 cflatobjs += lib/s390x/sclp-ascii.o

s390x/intercept.c +15 -91

index 59e5fca..15e50f6 100644
@@ -10,15 +10,13 @@
  * under the terms of the GNU Library General Public License version 2.
  */
 #include <libcflat.h>
+#include <testrun.h>
 #include <asm/asm-offsets.h>
 #include <asm/interrupt.h>
 #include <asm/page.h>
 
 static uint8_t pagebuf[PAGE_SIZE * 2] __attribute__((aligned(PAGE_SIZE * 2)));
 
-static unsigned long nr_iterations;
-static unsigned long time_to_run;
-
 /* Test the STORE PREFIX instruction */
 static void test_stpx(void)
 {

@@ -159,95 +157,21 @@ static void test_testblock(void)
 	check_pgm_int_code(PGM_INT_CODE_ADDRESSING);
 }
 
-static uint64_t get_clock_ms(void)
-{
-	uint64_t clk;
-
-	asm volatile(" stck %0 " : : "Q"(clk) : "memory");
-
-	/* Bit 51 is incrememented each microsecond */
-	return (clk >> (63 - 51)) / 1000;
-}
-
-struct {
-	const char *name;
-	void (*func)(void);
-	bool run_it;
-} tests[] = {
-	{ "stpx", test_stpx, false },
-	{ "spx", test_spx, false },
-	{ "stap", test_stap, false },
-	{ "stidp", test_stidp, false },
-	{ "testblock", test_testblock, false },
-	{ NULL, NULL, false }
-};
-
-static void parse_intercept_test_args(int argc, char **argv)
-{
-	int i, ti;
-	bool run_all = true;
-
-	for (i = 1; i < argc; i++) {
-		if (!strcmp("-i", argv[i])) {
-			i++;
-			if (i >= argc)
-				report_abort("-i needs a parameter");
-			nr_iterations = atol(argv[i]);
-		} else if (!strcmp("-t", argv[i])) {
-			i++;
-			if (i >= argc)
-				report_abort("-t needs a parameter");
-			time_to_run = atol(argv[i]);
-		} else for (ti = 0; tests[ti].name != NULL; ti++) {
-			if (!strcmp(tests[ti].name, argv[i])) {
-				run_all = false;
-				tests[ti].run_it = true;
-				break;
-			} else if (tests[ti + 1].name == NULL) {
-				report_abort("Unsupported parameter '%s'",
-					     argv[i]);
-			}
-		}
-	}
-
-	if (run_all) {
-		for (ti = 0; tests[ti].name != NULL; ti++)
-			tests[ti].run_it = true;
+static struct test test = {
+	.name = "intercept",
+	.multirun_supported = true,
+	.test_cases = (struct test_case[]) {
+		DEFINE_TEST_CASE("stpx", test_stpx),
+		DEFINE_TEST_CASE("spx", test_spx),
+		DEFINE_TEST_CASE("stap", test_stap),
+		DEFINE_TEST_CASE("stidp", test_stidp),
+		DEFINE_TEST_CASE("testblock", test_testblock),
+		DEFINE_TEST_CASE_END_OF_LIST()
 	}
-}
+};
 
-int main(int argc, char **argv)
+int main(int argc, const char **argv)
 {
-	uint64_t startclk;
-	int ti;
-
-	parse_intercept_test_args(argc, argv);
-
-	if (nr_iterations == 0 && time_to_run == 0)
-		nr_iterations = 1;
-
-	report_prefix_push("intercept");
-
-	startclk = get_clock_ms();
-	for (;;) {
-		for (ti = 0; tests[ti].name != NULL; ti++) {
-			report_prefix_push(tests[ti].name);
-			if (tests[ti].run_it)
-				tests[ti].func();
-			report_prefix_pop();
-		}
-		if (nr_iterations) {
-			nr_iterations -= 1;
-			if (nr_iterations == 0)
-				break;
-		}
-		if (time_to_run) {
-			if (get_clock_ms() - startclk > time_to_run)
-				break;
-		}
-	}
-
-	report_prefix_pop();
-
-	return report_summary();
+	parse_test_args(argc, argv, &test);
+	return run_test(&test);
 }

Quoted 9 lines · David Hildenbrand

I want to use this in another file, so instead of replicating, factoring it out feels like the right thing to do.

Let's directly provide it for all architectures, they just have to implement get_time_ms() in asm/time.h to use it.

Signed-off-by David Hildenbrand <[email protected]>

---

Quoted 4 lines

The next step would be allowing to add custom parameters to a test. (defining them similar to the test cases).

Any other requests/nice to have? Does this make sense?

It does, but we have at least this, vmx.c (v1 and v2) and vmexit.c, with not exactly overlapping usecases and command-line syntax. Would it make

sense to have an API that is not home-grown, e.g. a subset of GTest? The command-line arguments could be globs (like GTest's "-p").

Paolo

Quoted 10 lines · Paolo Bonzini

Quoted 9 lines · David Hildenbrand

I want to use this in another file, so instead of replicating, factoring it out feels like the right thing to do.

Let's directly provide it for all architectures, they just have to implement get_time_ms() in asm/time.h to use it.

Signed-off-by David Hildenbrand <[email protected]>

---

Quoted 7 lines

Quoted 4 lines

The next step would be allowing to add custom parameters to a test. (defining them similar to the test cases).

Any other requests/nice to have? Does this make sense?

It does, but we have at least this, vmx.c (v1 and v2) and vmexit.c, with not exactly overlapping usecases and command-line syntax. Would it make

Yes, the ultimate goal would be to combine all into one. I only had a look at some ARM tests yet. Will have a look at these.

Quoted 2 lines

sense to have an API that is not home-grown, e.g. a subset of GTest? The command-line arguments could be globs (like GTest's "-p").

I'll have a look. Any experts around?

Quoted 19 lines · David Hildenbrand

Quoted 10 lines · Paolo Bonzini

Quoted 9 lines · David Hildenbrand

I want to use this in another file, so instead of replicating, factoring it out feels like the right thing to do.

Let's directly provide it for all architectures, they just have to implement get_time_ms() in asm/time.h to use it.

Signed-off-by David Hildenbrand <[email protected]>

---

Quoted 7 lines

Quoted 4 lines

The next step would be allowing to add custom parameters to a test. (defining them similar to the test cases).

Any other requests/nice to have? Does this make sense?

It does, but we have at least this, vmx.c (v1 and v2) and vmexit.c, with not exactly overlapping usecases and command-line syntax. Would it make

Quoted 7 lines

Yes, the ultimate goal would be to combine all into one. I only had a look at some ARM tests yet. Will have a look at these.

Quoted 2 lines

sense to have an API that is not home-grown, e.g. a subset of GTest? The command-line arguments could be globs (like GTest's "-p").

I'll have a look. Any experts around?

Take a look at https://developer.gnome.org/glib/stable/glib-Testing.html.

But... one obstacle is probably the lack of malloc in x86, so this would be a prerequisite. Andrew first proposed it last year, and there was some discussion on the design and how to abstract giving more memory to malloc. You can find the result here:
https://patchwork.kernel.org/patch/9409843/ -- it's a longish thread, but it should be fun to implement. :)

The idea is to have four allocator APIs:

- "phys" (phys_alloc) provides contiguous physical addresses and is used in early allocations or for tests that do not enable the MMU. It need not be able to free anything. It also provides an arch-dependent interface to register the available memory.

- "page" (alloc_page) is initialized after the MMU is started. It picks whatever memory wasn't allocated by phys, and slices it in pages. Like "phys", it provides contiguous physical addresses.

- "virt" (alloc_vpages) is also initialized after the MMU is started, and it allocates virtual address space. It also provides an arch-dependent interface to install PTEs. It provides contiguous virtual addresses.

- "malloc" is the API we all love :) and it uses a pluggable allocator underneath, called "morecore" in the thread, with the prototype

    void *(*morecore)(size_t size, size_t align_min);

In the beginning "morecore" simply uses "phys", later it is switched (when the MMU is started) to use both "page" and "virt". That is it takes not necessarily consecutive physical memory at page granularity from "page", it assigning consecutive virtual address (from "virt") to it, and if needed it also slices pages into smaller allocations.

Of course each could be arbitrarily complicated, but it need not be.

"page" could be a buddy system, but really in practice it need not even support reusing freed pages, except for 1-page allocations. This makes it enough to use a simple free list of pages, plus a separately-tracked huge block of consecutive free physical addresses at the top.

Likewise, "malloc" could be Doug Lea's malloc, but really in practice it will just be a small veneer over morecore, with a dummy free. Like the testing thing, the important part is avoiding under-engineering.

Quoting from the thread, you can choose separately at each layer whether to implement freeing or not. "phys", "virt" and "morecore" do not need it. See in particular the Nov. 3, 2016, 5:34 p.m. message for a plan to transform Drew's v2 series
(https://www.spinics.net/lists/kvm/msg139685.html) into the design envisioned above.

Paolo

Quoted 75 lines · Paolo Bonzini

Quoted 19 lines · David Hildenbrand

Quoted 10 lines · Paolo Bonzini

Quoted 9 lines · David Hildenbrand

I want to use this in another file, so instead of replicating, factoring it out feels like the right thing to do.

Let's directly provide it for all architectures, they just have to implement get_time_ms() in asm/time.h to use it.

Signed-off-by David Hildenbrand <[email protected]>

---

Quoted 7 lines

Quoted 4 lines

The next step would be allowing to add custom parameters to a test. (defining them similar to the test cases).

Any other requests/nice to have? Does this make sense?

It does, but we have at least this, vmx.c (v1 and v2) and vmexit.c, with not exactly overlapping usecases and command-line syntax. Would it make

Quoted 7 lines

Yes, the ultimate goal would be to combine all into one. I only had a look at some ARM tests yet. Will have a look at these.

Quoted 2 lines

sense to have an API that is not home-grown, e.g. a subset of GTest? The command-line arguments could be globs (like GTest's "-p").

I'll have a look. Any experts around?

Take a look at https://developer.gnome.org/glib/stable/glib-Testing.html.

But... one obstacle is probably the lack of malloc in x86, so this would be a prerequisite. Andrew first proposed it last year, and there was some discussion on the design and how to abstract giving more memory to malloc. You can find the result here:
https://patchwork.kernel.org/patch/9409843/ -- it's a longish thread, but it should be fun to implement. :)

The idea is to have four allocator APIs:

- "phys" (phys_alloc) provides contiguous physical addresses and is used in early allocations or for tests that do not enable the MMU. It need not be able to free anything. It also provides an arch-dependent interface to register the available memory.

- "page" (alloc_page) is initialized after the MMU is started. It picks whatever memory wasn't allocated by phys, and slices it in pages. Like "phys", it provides contiguous physical addresses.

- "virt" (alloc_vpages) is also initialized after the MMU is started, and it allocates virtual address space. It also provides an arch-dependent interface to install PTEs. It provides contiguous virtual addresses.

- "malloc" is the API we all love :) and it uses a pluggable allocator underneath, called "morecore" in the thread, with the prototype

    void *(*morecore)(size_t size, size_t align_min);

In the beginning "morecore" simply uses "phys", later it is switched (when the MMU is started) to use both "page" and "virt". That is it takes not necessarily consecutive physical memory at page granularity from "page", it assigning consecutive virtual address (from "virt") to it, and if needed it also slices pages into smaller allocations.

Of course each could be arbitrarily complicated, but it need not be.

"page" could be a buddy system, but really in practice it need not even support reusing freed pages, except for 1-page allocations. This makes it enough to use a simple free list of pages, plus a separately-tracked huge block of consecutive free physical addresses at the top.

Likewise, "malloc" could be Doug Lea's malloc, but really in practice it will just be a small veneer over morecore, with a dummy free. Like the testing thing, the important part is avoiding under-engineering.

Quoted 6 lines

Quoting from the thread, you can choose separately at each layer whether to implement freeing or not. "phys", "virt" and "morecore" do not need it. See in particular the Nov. 3, 2016, 5:34 p.m. message for a plan to transform Drew's v2 series
(https://www.spinics.net/lists/kvm/msg139685.html) into the design envisioned above.

Holy cow, that sounds like a lot of work, especially as malloc is also not expected to work on s390x - guess because of which architecture I started to factor out ;)

phys_alloc_init() yields only arm and powerpc.

... so while having GTest would be the optimal solution, I wonder if it is worth the trouble right now. (sure, malloc for x86 and s390x would also be nice, but already getting that implemented sounds like it could take quite a while).

Hmm.... probably have too look into the details of the malloc discussion...

Quoted 94 lines · David Hildenbrand

Quoted 75 lines · Paolo Bonzini

Quoted 19 lines · David Hildenbrand

Quoted 10 lines · Paolo Bonzini

Quoted 9 lines · David Hildenbrand

I want to use this in another file, so instead of replicating, factoring it out feels like the right thing to do.

Let's directly provide it for all architectures, they just have to implement get_time_ms() in asm/time.h to use it.

Signed-off-by David Hildenbrand <[email protected]>

---

Quoted 7 lines

Quoted 4 lines

The next step would be allowing to add custom parameters to a test. (defining them similar to the test cases).

Any other requests/nice to have? Does this make sense?

It does, but we have at least this, vmx.c (v1 and v2) and vmexit.c, with not exactly overlapping usecases and command-line syntax. Would it make

Quoted 7 lines

Yes, the ultimate goal would be to combine all into one. I only had a look at some ARM tests yet. Will have a look at these.

Quoted 2 lines

sense to have an API that is not home-grown, e.g. a subset of GTest? The command-line arguments could be globs (like GTest's "-p").

I'll have a look. Any experts around?

Take a look at https://developer.gnome.org/glib/stable/glib-Testing.html.

But... one obstacle is probably the lack of malloc in x86, so this would be a prerequisite. Andrew first proposed it last year, and there was some discussion on the design and how to abstract giving more memory to malloc. You can find the result here:
https://patchwork.kernel.org/patch/9409843/ -- it's a longish thread, but it should be fun to implement. :)

The idea is to have four allocator APIs:

- "phys" (phys_alloc) provides contiguous physical addresses and is used in early allocations or for tests that do not enable the MMU. It need not be able to free anything. It also provides an arch-dependent interface to register the available memory.

- "page" (alloc_page) is initialized after the MMU is started. It picks whatever memory wasn't allocated by phys, and slices it in pages. Like "phys", it provides contiguous physical addresses.

- "virt" (alloc_vpages) is also initialized after the MMU is started, and it allocates virtual address space. It also provides an arch-dependent interface to install PTEs. It provides contiguous virtual addresses.

- "malloc" is the API we all love :) and it uses a pluggable allocator underneath, called "morecore" in the thread, with the prototype

    void *(*morecore)(size_t size, size_t align_min);

In the beginning "morecore" simply uses "phys", later it is switched (when the MMU is started) to use both "page" and "virt". That is it takes not necessarily consecutive physical memory at page granularity from "page", it assigning consecutive virtual address (from "virt") to it, and if needed it also slices pages into smaller allocations.

Of course each could be arbitrarily complicated, but it need not be.

"page" could be a buddy system, but really in practice it need not even support reusing freed pages, except for 1-page allocations. This makes it enough to use a simple free list of pages, plus a separately-tracked huge block of consecutive free physical addresses at the top.

Likewise, "malloc" could be Doug Lea's malloc, but really in practice it will just be a small veneer over morecore, with a dummy free. Like the testing thing, the important part is avoiding under-engineering.

Quoted 6 lines

Quoting from the thread, you can choose separately at each layer whether to implement freeing or not. "phys", "virt" and "morecore" do not need it. See in particular the Nov. 3, 2016, 5:34 p.m. message for a plan to transform Drew's v2 series
(https://www.spinics.net/lists/kvm/msg139685.html) into the design envisioned above.

Holy cow, that sounds like a lot of work, especially as malloc is also not expected to work on s390x - guess because of which architecture I started to factor out ;)

phys_alloc_init() yields only arm and powerpc.

... so while having GTest would be the optimal solution, I wonder if it is worth the trouble right now. (sure, malloc for x86 and s390x would also be nice, but already getting that implemented sounds like it could take quite a while).

Quoted 1 line

Hmm.... probably have too look into the details of the malloc discussion...

It's really less work than it sounds. I'm not wed to GTest, but I think the ability to define test cases programmatically is useful (vmexit.c

for example has a case where the testcase list is actually in QEMU!) and it pretty much requires malloc.

Paolo

Quoted 95 lines · Paolo Bonzini

Quoted 94 lines · David Hildenbrand

Quoted 75 lines · Paolo Bonzini

Quoted 19 lines · David Hildenbrand

Quoted 10 lines · Paolo Bonzini

Quoted 9 lines · David Hildenbrand

I want to use this in another file, so instead of replicating, factoring it out feels like the right thing to do.

Let's directly provide it for all architectures, they just have to implement get_time_ms() in asm/time.h to use it.

Signed-off-by David Hildenbrand <[email protected]>

---

Quoted 7 lines

Quoted 4 lines

The next step would be allowing to add custom parameters to a test. (defining them similar to the test cases).

Any other requests/nice to have? Does this make sense?

It does, but we have at least this, vmx.c (v1 and v2) and vmexit.c, with not exactly overlapping usecases and command-line syntax. Would it make

Quoted 7 lines

Yes, the ultimate goal would be to combine all into one. I only had a look at some ARM tests yet. Will have a look at these.

Quoted 2 lines

sense to have an API that is not home-grown, e.g. a subset of GTest? The command-line arguments could be globs (like GTest's "-p").

I'll have a look. Any experts around?

Take a look at https://developer.gnome.org/glib/stable/glib-Testing.html.

But... one obstacle is probably the lack of malloc in x86, so this would be a prerequisite. Andrew first proposed it last year, and there was some discussion on the design and how to abstract giving more memory to malloc. You can find the result here:
https://patchwork.kernel.org/patch/9409843/ -- it's a longish thread, but it should be fun to implement. :)

The idea is to have four allocator APIs:

- "phys" (phys_alloc) provides contiguous physical addresses and is used in early allocations or for tests that do not enable the MMU. It need not be able to free anything. It also provides an arch-dependent interface to register the available memory.

- "page" (alloc_page) is initialized after the MMU is started. It picks whatever memory wasn't allocated by phys, and slices it in pages. Like "phys", it provides contiguous physical addresses.

- "virt" (alloc_vpages) is also initialized after the MMU is started, and it allocates virtual address space. It also provides an arch-dependent interface to install PTEs. It provides contiguous virtual addresses.

- "malloc" is the API we all love :) and it uses a pluggable allocator underneath, called "morecore" in the thread, with the prototype

    void *(*morecore)(size_t size, size_t align_min);

In the beginning "morecore" simply uses "phys", later it is switched (when the MMU is started) to use both "page" and "virt". That is it takes not necessarily consecutive physical memory at page granularity from "page", it assigning consecutive virtual address (from "virt") to it, and if needed it also slices pages into smaller allocations.

Of course each could be arbitrarily complicated, but it need not be.

"page" could be a buddy system, but really in practice it need not even support reusing freed pages, except for 1-page allocations. This makes it enough to use a simple free list of pages, plus a separately-tracked huge block of consecutive free physical addresses at the top.

Likewise, "malloc" could be Doug Lea's malloc, but really in practice it will just be a small veneer over morecore, with a dummy free. Like the testing thing, the important part is avoiding under-engineering.

Quoted 6 lines

Quoting from the thread, you can choose separately at each layer whether to implement freeing or not. "phys", "virt" and "morecore" do not need it. See in particular the Nov. 3, 2016, 5:34 p.m. message for a plan to transform Drew's v2 series
(https://www.spinics.net/lists/kvm/msg139685.html) into the design envisioned above.

Holy cow, that sounds like a lot of work, especially as malloc is also not expected to work on s390x - guess because of which architecture I started to factor out ;)

phys_alloc_init() yields only arm and powerpc.

... so while having GTest would be the optimal solution, I wonder if it is worth the trouble right now. (sure, malloc for x86 and s390x would also be nice, but already getting that implemented sounds like it could take quite a while).

Quoted 4 lines

Quoted 1 line

Hmm.... probably have too look into the details of the malloc discussion...

It's really less work than it sounds. I'm not wed to GTest, but I think the ability to define test cases programmatically is useful (vmexit.c

I agree, as I said I would also like to use that. But at least at the first sight this really looks like a lot of work.

Quoted 2 lines

for example has a case where the testcase list is actually in QEMU!) and it pretty much requires malloc.

gtestutils.c also uses g_timer() ... this will be fun :)

Quoted 2 lines

It's really less work than it sounds. I'm not wed to GTest, but I think the ability to define test cases programmatically is useful (vmexit.c

I agree, as I said I would also like to use that. But at least at the first sight this really looks like a lot of work.

Quoted 2 lines

Hmm.... probably have too look into the details of the malloc discussion...

It's really less work than it sounds. I'm not wed to GTest, but I think the ability to define test cases programmatically is useful (vmexit.c for example has a case where the testcase list is actually in QEMU!) and it pretty much requires malloc.

Paolo

Quoted 7 lines

Quoted 2 lines

Hmm.... probably have too look into the details of the malloc discussion...

It's really less work than it sounds. I'm not wed to GTest, but I think the ability to define test cases programmatically is useful (vmexit.c for example has a case where the testcase list is actually in QEMU!) and it pretty much requires malloc.

Btw, by "has a case where the testcase list is actually in QEMU" you mean simply selecting the list of tests to run via -apend?

This is already supported in this current RFC, and the way Thomas implemented it doesn't require malloc - see "run_it". (it's not 100% clean but for our purpose just ok).

Quoted 3 lines

Paolo

Quoted 8 lines · David Hildenbrand

Quoted 7 lines

Quoted 2 lines

Hmm.... probably have too look into the details of the malloc discussion...

It's really less work than it sounds. I'm not wed to GTest, but I think the ability to define test cases programmatically is useful (vmexit.c for example has a case where the testcase list is actually in QEMU!) and it pretty much requires malloc.

Quoted 2 lines

Btw, by "has a case where the testcase list is actually in QEMU" you mean simply selecting the list of tests to run via -apend?

No, QEMU has a test device that can provide the names of several testcases. So you could have a test named "name_of_test1" but name_of_test1 is actually not in kvm-unit-tests, it is in QEMU.

Paolo

Quoted 4 lines

This is already supported in this current RFC, and the way Thomas implemented it doesn't require malloc - see "run_it". (it's not 100% clean but for our purpose just ok).

Quoted 19 lines · David Hildenbrand

Quoted 10 lines · Paolo Bonzini

Quoted 9 lines · David Hildenbrand

I want to use this in another file, so instead of replicating, factoring it out feels like the right thing to do.

Let's directly provide it for all architectures, they just have to implement get_time_ms() in asm/time.h to use it.

Signed-off-by David Hildenbrand <[email protected]>

---

Quoted 7 lines

Quoted 4 lines

The next step would be allowing to add custom parameters to a test. (defining them similar to the test cases).

Any other requests/nice to have? Does this make sense?

It does, but we have at least this, vmx.c (v1 and v2) and vmexit.c, with not exactly overlapping usecases and command-line syntax. Would it make

Quoted 7 lines

Yes, the ultimate goal would be to combine all into one. I only had a look at some ARM tests yet. Will have a look at these.

Quoted 2 lines

sense to have an API that is not home-grown, e.g. a subset of GTest? The command-line arguments could be globs (like GTest's "-p").

I'll have a look. Any experts around?

Take a look at https://developer.gnome.org/glib/stable/glib-Testing.html.

But... one obstacle is probably the lack of malloc in x86, so this would be a prerequisite. Andrew first proposed it last year, and there was some discussion on the design and how to abstract giving more memory to malloc. You can find the result here:
https://patchwork.kernel.org/patch/9409843/ -- it's a longish thread, but it should be fun to implement. :)

The idea is to have four allocator APIs:

- "phys" (phys_alloc) provides contiguous physical addresses and is used in early allocations or for tests that do not enable the MMU. It need not be able to free anything. It also provides an arch-dependent interface to register the available memory.

- "page" (alloc_page) is initialized after the MMU is started. It picks whatever memory wasn't allocated by phys, and slices it in pages. Like "phys", it provides contiguous physical addresses.

- "virt" (alloc_vpages) is also initialized after the MMU is started, and it allocates virtual address space. It also provides an arch-dependent interface to install PTEs. It provides contiguous virtual addresses.

- "malloc" is the API we all love :) and it uses a pluggable allocator underneath, called "morecore" in the thread, with the prototype

    void *(*morecore)(size_t size, size_t align_min);

In the beginning "morecore" simply uses "phys", later it is switched (when the MMU is started) to use both "page" and "virt". That is it takes not necessarily consecutive physical memory at page granularity from "page", it assigning consecutive virtual address (from "virt") to it, and if needed it also slices pages into smaller allocations.

Of course each could be arbitrarily complicated, but it need not be.

"page" could be a buddy system, but really in practice it need not even support reusing freed pages, except for 1-page allocations. This makes it enough to use a simple free list of pages, plus a separately-tracked huge block of consecutive free physical addresses at the top.

Likewise, "malloc" could be Doug Lea's malloc, but really in practice it will just be a small veneer over morecore, with a dummy free. Like the testing thing, the important part is avoiding under-engineering.

Quoting from the thread, you can choose separately at each layer whether to implement freeing or not. "phys", "virt" and "morecore" do not need it. See in particular the Nov. 3, 2016, 5:34 p.m. message for a plan to transform Drew's v2 series
(https://www.spinics.net/lists/kvm/msg139685.html) into the design envisioned above.

lib/testrun.c new file +25 -0

@@ -0,0 +1,89 @@
+/*
+ * Utility functions for running/configuring a set of tests
+ *
+ * Copyright (c) 2017 Red Hat Inc
+ *
+ * Authors:
+ *  Thomas Huth <[email protected]>
+ *  David Hildenbrand <[email protected]>
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU Library General Public License version 2.
+ */
+
+#include <testrun.h>
+#include <asm/time.h>
+
+void parse_test_args(const int argc, const char **argv, struct test *test)
+{
+	bool run_all = true;
+	int i, ti;
+
+	test->iterations = 0;
+	test->time_to_run = 0;
+
+	for (i = 1; i < argc; i++) {

lib/testrun.c new file +8 -0

+			}
+		}
+	}
+
+	if (test->iterations == 0 && test->time_to_run == 0)
+		test->iterations = 1;
+
+	if ((test->iterations > 1 || test->time_to_run) &&

lib/testrun.c new file +28 -0

+{
+	unsigned long iterations = test->iterations;
+	unsigned long time_to_run = test->time_to_run;
+	uint64_t startclk;
+	int ti;
+
+	report_prefix_push(test->name);
+	startclk = get_clock_ms();
+	for (;;) {
+		for (ti = 0; test->test_cases[ti].name != NULL; ti++) {
+			report_prefix_push(test->test_cases[ti].name);
+			if (test->test_cases[ti].run_it)
+				test->test_cases[ti].func();
+			report_prefix_pop();
+		}
+		if (iterations) {
+			iterations -= 1;
+			if (iterations == 0)
+				break;
+		}
+		if (time_to_run) {
+			if (get_clock_ms() - startclk > time_to_run)
+				break;
+		}
+	}
+	report_prefix_pop();
+	return report_summary();
+}

lib/testrun.h new file +31 -0

new file mode 100644
index 0000000..8f9a2d7
@@ -0,0 +1,41 @@
+/*
+ * Utility functions for running/configuring a set of test cases
+ *
+ * Copyright (c) 2017 Red Hat Inc
+ *
+ * Authors:
+ *  Thomas Huth <[email protected]>
+ *  David Hildenbrand <[email protected]>
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU Library General Public License version 2.
+ */
+#ifndef __TESTRUN_H
+#define __TESTRUN_H
+
+#include <libcflat.h>
+
+struct test_case {
+	const char *name;
+	void (*func)(void);
+	/* initialized by parse_test_args() */
+	bool run_it;
+};
+
+#define DEFINE_TEST_CASE(_name, _func) { _name, _func, false }
+#define DEFINE_TEST_CASE_END_OF_LIST() { }
+
+struct test {
+	/* to be initialized by the test */
+	const char *name;
+	bool multirun_supported;

We have the under utilized lib/util.[ch] files already. I think either these functions should go there, or the functions there should get merged into whatever we want to call the files for these functions, and util.[ch] should then be removed. Actually, I think Radim wanted to rework the one and only function in util.[ch] too, to improve its interface...

Quoted 33 lines

lib/testrun.c new file +31 -0

@@ -0,0 +1,89 @@
+/*
+ * Utility functions for running/configuring a set of tests
+ *
+ * Copyright (c) 2017 Red Hat Inc
+ *
+ * Authors:
+ *  Thomas Huth <[email protected]>
+ *  David Hildenbrand <[email protected]>
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU Library General Public License version 2.
+ */
+
+#include <testrun.h>
+#include <asm/time.h>
+
+void parse_test_args(const int argc, const char **argv, struct test *test)
+{
+	bool run_all = true;
+	int i, ti;
+
+	test->iterations = 0;
+	test->time_to_run = 0;
+
+	for (i = 1; i < argc; i++) {
+		if (!strcmp("-i", argv[i])) {
+			i++;
+			if (i >= argc)
+				report_abort("-i needs a parameter");
+			test->iterations = atol(argv[i]);
+		} else if (!strcmp("-t", argv[i])) {

lib/testrun.c line 35

Quoted 4 lines

lib/testrun.c new file +4 -0

+			i++;
+			if (i >= argc)
+				report_abort("-t needs a parameter");
+			test->time_to_run = atol(argv[i]);

I'd prefer we use long form args, like --iterations=<num> and --time-to-run=<seconds>. That way unittests.cfg files will be self documenting.

lib/testrun.c line 36

Quoted 2 lines

lib/testrun.c new file +1 -0

+		} else for (ti = 0; test->test_cases[ti].name != NULL; ti++) {

Unusual (at least to me) coding style. I'd prefer the more usual (to me)

 else {
    for ... {
    }

}

lib/testrun.c line 40

Quoted 4 lines

lib/testrun.c new file +4 -0

+			if (!strcmp(test->test_cases[ti].name, argv[i])) {
+				run_all = false;
+				test->test_cases[ti].run_it = true;
+				break;

The break means we can only list a single test to run, right? What about being able to run more than one by listing all you want?

lib/testrun.c line 43

Quoted 4 lines

lib/testrun.c new file +3 -0

+			} else if (test->test_cases[ti + 1].name == NULL) {
+				report_abort("Unsupported parameter '%s'",
+					     argv[i]);

Instead of this, we could use a counter to track number of recognized args and then, after running the loop, if the counter doesn't match argc complain/abort.

Quoted 14 lines

lib/testrun.c new file +13 -0

+			}
+		}
+	}
+
+	if (test->iterations == 0 && test->time_to_run == 0)
+		test->iterations = 1;
+
+	if ((test->iterations > 1 || test->time_to_run) &&
+	    !test->multirun_supported)
+		report_abort("Test does not support multiruns.");
+
+	if (run_all) {
+		for (ti = 0; test->test_cases[ti].name != NULL; ti++)

lib/testrun.c line 61

Quoted 5 lines

lib/testrun.c new file +5 -0

+			test->test_cases[ti].run_it = true;
+	}
+}
+
+int run_test(struct test *test)

I prefer this be plural, run_tests, and the argument test doesn't seem named quite right either, "testset" maybe?

Quoted 7 lines

We have the under utilized lib/util.[ch] files already. I think either these functions should go there, or the functions there should get merged into whatever we want to call the files for these functions, and util.[ch] should then be removed. Actually, I think Radim wanted to rework the one and only function in util.[ch] too, to improve its interface...

In general, I think I'd prefer to drop util.X and rather give the child a name. :)

Quoted 33 lines

Quoted 33 lines

lib/testrun.c new file +31 -0

@@ -0,0 +1,89 @@
+/*
+ * Utility functions for running/configuring a set of tests
+ *
+ * Copyright (c) 2017 Red Hat Inc
+ *
+ * Authors:
+ *  Thomas Huth <[email protected]>
+ *  David Hildenbrand <[email protected]>
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU Library General Public License version 2.
+ */
+
+#include <testrun.h>
+#include <asm/time.h>
+
+void parse_test_args(const int argc, const char **argv, struct test *test)
+{
+	bool run_all = true;
+	int i, ti;
+
+	test->iterations = 0;
+	test->time_to_run = 0;
+
+	for (i = 1; i < argc; i++) {
+		if (!strcmp("-i", argv[i])) {
+			i++;
+			if (i >= argc)
+				report_abort("-i needs a parameter");
+			test->iterations = atol(argv[i]);
+		} else if (!strcmp("-t", argv[i])) {

Quoted 8 lines

lib/testrun.c line 35

Quoted 4 lines

lib/testrun.c new file +4 -0

+			i++;
+			if (i >= argc)
+				report_abort("-t needs a parameter");
+			test->time_to_run = atol(argv[i]);

I'd prefer we use long form args, like --iterations=<num> and --time-to-run=<seconds>. That way unittests.cfg files will be self documenting.

I just moved this code from intercept.c. So this sure can be done.

Quoted 5 lines

lib/testrun.c line 36

Quoted 2 lines

lib/testrun.c new file +1 -0

+		} else for (ti = 0; test->test_cases[ti].name != NULL; ti++) {

Unusual (at least to me) coding style. I'd prefer the more usual (to me)

Absolutely, I also just moved it. But this really looks ... special :)

Quoted 5 lines

 else {
    for ... {
    }

}

Quoted 7 lines

lib/testrun.c line 40

Quoted 4 lines

lib/testrun.c new file +4 -0

+			if (!strcmp(test->test_cases[ti].name, argv[i])) {
+				run_all = false;
+				test->test_cases[ti].run_it = true;
+				break;

The break means we can only list a single test to run, right? What about being able to run more than one by listing all you want?

That should be supported, no? The break will simply start looking for a test named "argv[i]". The next one (argv[i + 1]) would be processed afterwards.

Quoted 9 lines

lib/testrun.c line 43

Quoted 5 lines

lib/testrun.c new file +3 -0

+			} else if (test->test_cases[ti + 1].name == NULL) {
+				report_abort("Unsupported parameter '%s'",
+					     argv[i]);

Instead of this, we could use a counter to track number of recognized args and then, after running the loop, if the counter doesn't match argc complain/abort.

Yes, this could be refactored a lot, especially if we want to include parsing test specific parameters (specified in struct test.).

Quoted 14 lines

Quoted 14 lines

lib/testrun.c new file +13 -0

+			}
+		}
+	}
+
+	if (test->iterations == 0 && test->time_to_run == 0)
+		test->iterations = 1;
+
+	if ((test->iterations > 1 || test->time_to_run) &&
+	    !test->multirun_supported)
+		report_abort("Test does not support multiruns.");
+
+	if (run_all) {
+		for (ti = 0; test->test_cases[ti].name != NULL; ti++)

Quoted 8 lines

lib/testrun.c line 61

Quoted 5 lines

lib/testrun.c new file +5 -0

+			test->test_cases[ti].run_it = true;
+	}
+}
+
+int run_test(struct test *test)

I prefer this be plural, run_tests, and the argument test doesn't seem named quite right either, "testset" maybe?

I renamed this at least 10 times :)

We have a test that contains test cases. So it should be singular. and struct test handed over is not a list but only one element. But we would have to agree on a common terminology.

Main question is, if it makes sense to work on this or directly try to implement at least parts of the glib test API as Paolo suggests. Any thoughts?