[kvm-unit-tests,RFC] s390x: factor out running of tests into common code
diff mbox

Message ID 20170912141658.8146-1-david@redhat.com
State New
Headers show

Commit Message

David Hildenbrand Sept. 12, 2017, 2:16 p.m. UTC
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 <david@redhat.com>
---

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

Comments

Paolo Bonzini Sept. 12, 2017, 3:01 p.m. UTC | #1
On 12/09/2017 16:16, David Hildenbrand wrote:
> 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 <david@redhat.com>
> ---
> 
> 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

>  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
> 
> diff --git a/lib/s390x/asm/time.h b/lib/s390x/asm/time.h
> new file mode 100644
> index 0000000..856facd
> --- /dev/null
> +++ b/lib/s390x/asm/time.h
> @@ -0,0 +1,24 @@
> +/*
> + * Copyright (c) 2017 Red Hat Inc
> + *
> + * Authors:
> + *  Thomas Huth <thuth@redhat.com>
> + *  David Hildenbrand <david@redhat.com>
> + *
> + * 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
> diff --git a/lib/testrun.c b/lib/testrun.c
> new file mode 100644
> index 0000000..6034767
> --- /dev/null
> +++ b/lib/testrun.c
> @@ -0,0 +1,89 @@
> +/*
> + * Utility functions for running/configuring a set of tests
> + *
> + * Copyright (c) 2017 Red Hat Inc
> + *
> + * Authors:
> + *  Thomas Huth <thuth@redhat.com>
> + *  David Hildenbrand <david@redhat.com>
> + *
> + * 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();
> +}
> diff --git a/lib/testrun.h b/lib/testrun.h
> new file mode 100644
> index 0000000..8f9a2d7
> --- /dev/null
> +++ b/lib/testrun.h
> @@ -0,0 +1,41 @@
> +/*
> + * Utility functions for running/configuring a set of test cases
> + *
> + * Copyright (c) 2017 Red Hat Inc
> + *
> + * Authors:
> + *  Thomas Huth <thuth@redhat.com>
> + *  David Hildenbrand <david@redhat.com>
> + *
> + * 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 */
> diff --git a/s390x/Makefile b/s390x/Makefile
> index bc099da..e174895 100644
> --- a/s390x/Makefile
> +++ b/s390x/Makefile
> @@ -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
> diff --git a/s390x/intercept.c b/s390x/intercept.c
> index 59e5fca..15e50f6 100644
> --- a/s390x/intercept.c
> +++ b/s390x/intercept.c
> @@ -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);
>  }
>
David Hildenbrand Sept. 12, 2017, 3:15 p.m. UTC | #2
On 12.09.2017 17:01, Paolo Bonzini wrote:
> On 12/09/2017 16:16, David Hildenbrand wrote:
>> 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 <david@redhat.com>
>> ---
>>
>> 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.

> 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?

> 
> Paolo
Paolo Bonzini Sept. 12, 2017, 3:46 p.m. UTC | #3
On 12/09/2017 17:15, David Hildenbrand wrote:
> On 12.09.2017 17:01, Paolo Bonzini wrote:
>> On 12/09/2017 16:16, David Hildenbrand wrote:
>>> 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 <david@redhat.com>
>>> ---
>>>
>>> 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.
> 
>> 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
David Hildenbrand Sept. 12, 2017, 4:17 p.m. UTC | #4
On 12.09.2017 17:46, Paolo Bonzini wrote:
> On 12/09/2017 17:15, David Hildenbrand wrote:
>> On 12.09.2017 17:01, Paolo Bonzini wrote:
>>> On 12/09/2017 16:16, David Hildenbrand wrote:
>>>> 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 <david@redhat.com>
>>>> ---
>>>>
>>>> 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.
>>
>>> 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
> 

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...
Paolo Bonzini Sept. 12, 2017, 4:24 p.m. UTC | #5
On 12/09/2017 18:17, David Hildenbrand wrote:
> On 12.09.2017 17:46, Paolo Bonzini wrote:
>> On 12/09/2017 17:15, David Hildenbrand wrote:
>>> On 12.09.2017 17:01, Paolo Bonzini wrote:
>>>> On 12/09/2017 16:16, David Hildenbrand wrote:
>>>>> 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 <david@redhat.com>
>>>>> ---
>>>>>
>>>>> 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.
>>>
>>>> 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.
> 
> 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...

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
David Hildenbrand Sept. 12, 2017, 4:27 p.m. UTC | #6
On 12.09.2017 18:24, Paolo Bonzini wrote:
> On 12/09/2017 18:17, David Hildenbrand wrote:
>> On 12.09.2017 17:46, Paolo Bonzini wrote:
>>> On 12/09/2017 17:15, David Hildenbrand wrote:
>>>> On 12.09.2017 17:01, Paolo Bonzini wrote:
>>>>> On 12/09/2017 16:16, David Hildenbrand wrote:
>>>>>> 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 <david@redhat.com>
>>>>>> ---
>>>>>>
>>>>>> 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.
>>>>
>>>>> 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.
>>
>> 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...
> 
> 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.

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

gtestutils.c also uses g_timer() ... this will be fun :)
Paolo Bonzini Sept. 12, 2017, 4:35 p.m. UTC | #7
On 12/09/2017 18:27, David Hildenbrand wrote:
> On 12.09.2017 18:24, Paolo Bonzini wrote:
>> On 12/09/2017 18:17, David Hildenbrand wrote:
>>> On 12.09.2017 17:46, Paolo Bonzini wrote:
>>>> On 12/09/2017 17:15, David Hildenbrand wrote:
>>>>> On 12.09.2017 17:01, Paolo Bonzini wrote:
>>>>>> On 12/09/2017 16:16, David Hildenbrand wrote:
>>>>>>> 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 <david@redhat.com>
>>>>>>> ---
>>>>>>>
>>>>>>> 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.
>>>>>
>>>>>> 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.
>>>
>>> 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...
>>
>> 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.
> 
>> 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 :)

You don't need to implement the full API.  Just g_test_{init,add*,run}
would be enough for a start.

Paolo
David Hildenbrand Sept. 12, 2017, 4:39 p.m. UTC | #8
>> 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).


> 
> Paolo
>
Paolo Bonzini Sept. 12, 2017, 4:43 p.m. UTC | #9
On 12/09/2017 18:39, David Hildenbrand wrote:
> 
>>> 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?

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

> 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).
> 
> 
>>
>> Paolo
>>
> 
>
David Hildenbrand Sept. 12, 2017, 4:50 p.m. UTC | #10
On 12.09.2017 18:43, Paolo Bonzini wrote:
> On 12/09/2017 18:39, David Hildenbrand wrote:
>>
>>>> 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?
> 
> 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
> 
>> 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).
>>

pci-testdev (I learned something today :)). Got it, basically
dynamically adding tests.
Andrew Jones Sept. 13, 2017, 12:26 p.m. UTC | #11
On Tue, Sep 12, 2017 at 05:46:48PM +0200, Paolo Bonzini wrote:
> On 12/09/2017 17:15, David Hildenbrand wrote:
> > On 12.09.2017 17:01, Paolo Bonzini wrote:
> >> On 12/09/2017 16:16, David Hildenbrand wrote:
> >>> 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 <david@redhat.com>
> >>> ---
> >>>
> >>> 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.
> > 
> >> 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.
>

I'd love to see this loop closed, and I like the GTest API idea too. I'm
not opposed to picking this back up myself, but it wouldn't be until after
KVM Forum. Anybody who wants to start now should feel free to :-)

Regarding this series: should we find a way to hack a malloc in order to
use the GTest API now? x86 and s390 could just reserve some memory for
malloc to use with their flat.lds files, for example.

I'll go ahead and give this series a quick review too, as any comments I
may have may apply to whatever followup series comes next.

Thanks,
drew
Andrew Jones Sept. 13, 2017, 12:57 p.m. UTC | #12
On Tue, Sep 12, 2017 at 04:16:58PM +0200, David Hildenbrand wrote:
> 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 <david@redhat.com>
> ---
> 
> 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
> 
> diff --git a/lib/s390x/asm/time.h b/lib/s390x/asm/time.h
> new file mode 100644
> index 0000000..856facd
> --- /dev/null
> +++ b/lib/s390x/asm/time.h
> @@ -0,0 +1,24 @@
> +/*
> + * Copyright (c) 2017 Red Hat Inc
> + *
> + * Authors:
> + *  Thomas Huth <thuth@redhat.com>
> + *  David Hildenbrand <david@redhat.com>
> + *
> + * 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
> diff --git a/lib/testrun.c b/lib/testrun.c
> new file mode 100644
> index 0000000..6034767
> --- /dev/null
> +++ b/lib/testrun.c

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...


> @@ -0,0 +1,89 @@
> +/*
> + * Utility functions for running/configuring a set of tests
> + *
> + * Copyright (c) 2017 Red Hat Inc
> + *
> + * Authors:
> + *  Thomas Huth <thuth@redhat.com>
> + *  David Hildenbrand <david@redhat.com>
> + *
> + * 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]);

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.

> +		} 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 ... {
    }
 } 

> +			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?

> +			} 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.

> +			}
> +		}
> +	}
> +
> +	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)

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

> +{
> +	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();
> +}
> diff --git a/lib/testrun.h b/lib/testrun.h
> new file mode 100644
> index 0000000..8f9a2d7
> --- /dev/null
> +++ b/lib/testrun.h
> @@ -0,0 +1,41 @@
> +/*
> + * Utility functions for running/configuring a set of test cases
> + *
> + * Copyright (c) 2017 Red Hat Inc
> + *
> + * Authors:
> + *  Thomas Huth <thuth@redhat.com>
> + *  David Hildenbrand <david@redhat.com>
> + *
> + * 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 */

Thanks,
drew
David Hildenbrand Sept. 13, 2017, 1:07 p.m. UTC | #13
> 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. :)

> 
>> @@ -0,0 +1,89 @@
>> +/*
>> + * Utility functions for running/configuring a set of tests
>> + *
>> + * Copyright (c) 2017 Red Hat Inc
>> + *
>> + * Authors:
>> + *  Thomas Huth <thuth@redhat.com>
>> + *  David Hildenbrand <david@redhat.com>
>> + *
>> + * 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]);
> 
> 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.

> 
>> +		} 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 :)

>  else {
>     for ... {
>     }
>  } 
> 
>> +			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.

> 
>> +			} 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.).

> 
>> +			}
>> +		}
>> +	}
>> +
>> +	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)
> 
> 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?

Thanks!
Andrew Jones Sept. 13, 2017, 1:34 p.m. UTC | #14
On Wed, Sep 13, 2017 at 03:07:23PM +0200, David Hildenbrand wrote:
> 
> > 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. :)
> 
> > 
> >> @@ -0,0 +1,89 @@
> >> +/*
> >> + * Utility functions for running/configuring a set of tests
> >> + *
> >> + * Copyright (c) 2017 Red Hat Inc
> >> + *
> >> + * Authors:
> >> + *  Thomas Huth <thuth@redhat.com>
> >> + *  David Hildenbrand <david@redhat.com>
> >> + *
> >> + * 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]);
> > 
> > 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.
> 
> > 
> >> +		} 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 :)
> 
> >  else {
> >     for ... {
> >     }
> >  } 
> > 
> >> +			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.

I somehow forgot there was an outer for loop... So, yeah, this is fine.

> 
> > 
> >> +			} 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.).
> 
> > 
> >> +			}
> >> +		}
> >> +	}
> >> +
> >> +	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)
> > 
> > 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.

I can retrain my brain to think this way, and then the terminology makes
sense. I just wonder if I'm the only one who needs retraining, or if
the API naming will always be a source of confusion? Anyway, the API
naming can be left to GTest.

> 
> 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?

Yes, I prefer the GTest idea as well. I just wanted to review this patch
too, in case any of the code would still be used when converting it.

Thanks,
drew
Paolo Bonzini Oct. 23, 2017, 11:19 a.m. UTC | #15
On 12/09/2017 18:27, David Hildenbrand wrote:
>> 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.

I did the first part, splitting the architecture-independent part of
lib/x86/vmalloc.c.

Paolo

Patch
diff mbox

diff --git a/lib/s390x/asm/time.h b/lib/s390x/asm/time.h
new file mode 100644
index 0000000..856facd
--- /dev/null
+++ b/lib/s390x/asm/time.h
@@ -0,0 +1,24 @@ 
+/*
+ * Copyright (c) 2017 Red Hat Inc
+ *
+ * Authors:
+ *  Thomas Huth <thuth@redhat.com>
+ *  David Hildenbrand <david@redhat.com>
+ *
+ * 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
diff --git a/lib/testrun.c b/lib/testrun.c
new file mode 100644
index 0000000..6034767
--- /dev/null
+++ b/lib/testrun.c
@@ -0,0 +1,89 @@ 
+/*
+ * Utility functions for running/configuring a set of tests
+ *
+ * Copyright (c) 2017 Red Hat Inc
+ *
+ * Authors:
+ *  Thomas Huth <thuth@redhat.com>
+ *  David Hildenbrand <david@redhat.com>
+ *
+ * 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();
+}
diff --git a/lib/testrun.h b/lib/testrun.h
new file mode 100644
index 0000000..8f9a2d7
--- /dev/null
+++ b/lib/testrun.h
@@ -0,0 +1,41 @@ 
+/*
+ * Utility functions for running/configuring a set of test cases
+ *
+ * Copyright (c) 2017 Red Hat Inc
+ *
+ * Authors:
+ *  Thomas Huth <thuth@redhat.com>
+ *  David Hildenbrand <david@redhat.com>
+ *
+ * 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 */
diff --git a/s390x/Makefile b/s390x/Makefile
index bc099da..e174895 100644
--- a/s390x/Makefile
+++ b/s390x/Makefile
@@ -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
diff --git a/s390x/intercept.c b/s390x/intercept.c
index 59e5fca..15e50f6 100644
--- a/s390x/intercept.c
+++ b/s390x/intercept.c
@@ -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);
 }