Re: [kvm-unit-tests PATCH v2] s390x: Interception tests

[David Hildenbrand <david@redhat.com>]

On 02.06.2017 14:44, Thomas Huth wrote:
> Certain CPU instructions will cause an exit of the virtual
> machine. Run some of these instructions to check whether
> they are emulated right by KVM (or QEMU).
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  Note: Test have been verified to pass with KVM of a 4.11 kernel.
>  Running the tests with QEMU TCG emulation does not work yet ...
>  QEMU first requires a bunch of fixes before this can pass there.
>  
>  v2:
>  - Added entry in s390x/unittests.cfg
>  - Use low-core GEN_LC_STFL definition instead of hard-coded magic value
>  - Added lots of exception tests (thanks to David's interrupt framework!)
>  - Fixed constraints of inline assembler
>  
>  (I haven't added a timing/iteration infrastructure like Paolo suggested
>  yet - will do that later once the basic tests have been accepted)
>  
>  lib/s390x/asm/interrupt.h |   1 +
>  lib/s390x/interrupt.c     |   5 ++
>  s390x/Makefile            |   1 +
>  s390x/intercept.c         | 170 ++++++++++++++++++++++++++++++++++++++++++++++
>  s390x/unittests.cfg       |   3 +
>  5 files changed, 180 insertions(+)
>  create mode 100644 s390x/intercept.c
> 
> diff --git a/lib/s390x/asm/interrupt.h b/lib/s390x/asm/interrupt.h
> index 383d312..926f858 100644
> --- a/lib/s390x/asm/interrupt.h
> +++ b/lib/s390x/asm/interrupt.h
> @@ -14,5 +14,6 @@
>  void handle_pgm_int(void);
>  void expect_pgm_int(void);
>  void check_pgm_int_code(uint16_t code);
> +uint16_t get_pgm_int_code(void);
>  
>  #endif
> diff --git a/lib/s390x/interrupt.c b/lib/s390x/interrupt.c
> index 8d861a2..b5cc7ce 100644
> --- a/lib/s390x/interrupt.c
> +++ b/lib/s390x/interrupt.c
> @@ -23,6 +23,11 @@ void expect_pgm_int(void)
>  	mb();
>  }
>  
> +uint16_t get_pgm_int_code(void)
> +{
> +	return lc->pgm_int_code;
> +}
> +
>  void check_pgm_int_code(uint16_t code)
>  {
>  	mb();
> diff --git a/s390x/Makefile b/s390x/Makefile
> index b48f8ab..a61e163 100644
> --- a/s390x/Makefile
> +++ b/s390x/Makefile
> @@ -1,4 +1,5 @@
>  tests = $(TEST_DIR)/selftest.elf
> +tests += $(TEST_DIR)/intercept.elf
>  
>  all: directories test_cases
>  
> diff --git a/s390x/intercept.c b/s390x/intercept.c
> new file mode 100644
> index 0000000..4e3fb57
> --- /dev/null
> +++ b/s390x/intercept.c
> @@ -0,0 +1,170 @@
> +/*
> + * Interception tests - for s390x CPU instruction that cause a VM exit
> + *
> + * Copyright (c) 2017 Red Hat Inc
> + *
> + * Authors:
> + *  Thomas Huth <thuth@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 <libcflat.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)));
> +
> +/* Enable or disable low-address protection */
> +static void set_low_prot(bool enable)
> +{
> +	uint64_t cr0;
> +
> +	asm volatile (" stctg 0,0,%0 " : : "Q"(cr0));

Use %c0 instead?

> +	if (enable)
> +		cr0 |= 1ULL << (63-35);
> +	else
> +		cr0 &= ~(1ULL << (63-35));
> +	asm volatile (" lctlg 0,0,%0 " : : "Q"(cr0));

dito.

> +}

Think it makes sense to move this to interrupt.c / interrupt.h, so other
tests can use it. But we can do this later.

> +
> +/* Test the SET PREFIX and STORE PREFIX instructions */
> +static void test_prefix(void)
> +{
> +	uint32_t old_prefix = -1U, tst_prefix = -1U;
> +	uint32_t new_prefix = (uint32_t)(intptr_t)pagebuf;
> +
> +	memset(pagebuf, 0, PAGE_SIZE * 2);
> +
> +	/*
> +	 * Temporarily change the prefix page to our buffer, and store
> +	 * some facility bits there ... at least some of them should be
> +	 * set in our buffer afterwards.
> +	 */
> +	asm volatile (
> +		" stpx	%0\n"
> +		" spx	%2\n"
> +		" stfl	0\n"
> +		" stpx	%1\n"
> +		" spx	%0\n"
> +		: "+Q"(old_prefix), "+Q"(tst_prefix)
> +		: "Q"(new_prefix)
> +		: "memory");
> +	report("spx + stfl", pagebuf[GEN_LC_STFL] != 0 &&
> +			     old_prefix == 0 && tst_prefix == new_prefix);

I would split this into two tests.

> +
> +	expect_pgm_int();
> +	asm volatile(" spx 0(%0) " : : "r"(1));
> +	report("spx alignment",
> +	       get_pgm_int_code() == PGM_INT_CODE_SPECIFICATION);

Wonder if it makes sense to pass check_pgm_int_code() an string like
"spx alignment", and let it handle the output. But this can also be
changed later.

> +
> +	expect_pgm_int();
> +	asm volatile(" spx 0(%0) " : : "r"(-8));
> +	report("spx addressing",
> +	       get_pgm_int_code() == PGM_INT_CODE_ADDRESSING);
> +
> +	expect_pgm_int();
> +	set_low_prot(true);
> +	asm volatile(" stpx 0(%0) " : : "r"(8));
> +	set_low_prot(false);
> +	report("stpx low-address protection",
> +	       get_pgm_int_code() == PGM_INT_CODE_PROTECTION);
> +
> +	expect_pgm_int();
> +	asm volatile(" stpx 0(%0) " : : "r"(1));
> +	report("stpx alignment",
> +	       get_pgm_int_code() == PGM_INT_CODE_SPECIFICATION);
> +
> +	expect_pgm_int();
> +	asm volatile(" stpx 0(%0) " : : "r"(-8));
> +	report("stpx addressing",
> +	       get_pgm_int_code() == PGM_INT_CODE_ADDRESSING);
> +}
> +
> +/* Test the STORE CPU ADDRESS instruction */
> +static void test_stap(void)
> +{
> +	uint16_t cpuid = 0xffff;
> +
> +	asm volatile ("stap %0\n" : "+Q"(cpuid));
> +	report("get cpu id", cpuid != 0xffff);
> +
> +	expect_pgm_int();
> +	set_low_prot(true);
> +	asm volatile ("stap 0(%0)\n" : : "r"(8));
> +	set_low_prot(false);
> +	report("low-address protection",
> +	       get_pgm_int_code() == PGM_INT_CODE_PROTECTION)> +
> +	expect_pgm_int();
> +	asm volatile ("stap 0(%0)\n" : : "r"(1));
> +	report("alignment", get_pgm_int_code() == PGM_INT_CODE_SPECIFICATION);
> +
> +	expect_pgm_int();
> +	asm volatile ("stap 0(%0)\n" : : "r"(-8));
> +	report("addressing", get_pgm_int_code() == PGM_INT_CODE_ADDRESSING);
> +}
> +
> +/* Test the TEST BLOCK instruction */
> +static void test_testblock(void)
> +{
> +	int cc;
> +
> +	memset(pagebuf, 0xaa, PAGE_SIZE);
> +
> +	asm volatile (
> +		" lghi	0,0\n"

%0,0 ?

> +		" tb	%1\n"
> +		" ipm	%0\n"
> +		" srl	%0,28\n"
> +		: "=d" (cc)
> +		: "a"(pagebuf + 0x123)
> +		: "memory", "0", "cc");
> +	report("page cleared",
> +	       cc == 0 && pagebuf[0] == 0 &&  pagebuf[PAGE_SIZE - 1] == 0);
> +
> +	expect_pgm_int();
> +	set_low_prot(true);
> +	asm volatile (" tb %0 " : : "r"(4096));
> +	set_low_prot(false);
> +	report("low-address protection",
> +	       get_pgm_int_code() == PGM_INT_CODE_PROTECTION);
> +
> +	expect_pgm_int();
> +	asm volatile (" tb %0 " : : "r"(-4096));
> +	report("addressing", get_pgm_int_code() == PGM_INT_CODE_ADDRESSING);
> +}
> +
> +struct {
> +	const char *name;
> +	void (*func)(void);
> +} tests[] = {
> +	{ "prefix", test_prefix },
> +	{ "stap", test_stap },
> +	{ "testblock", test_testblock },
> +	{ NULL, NULL }
> +};
> +
> +int main(int argc, char **argv)
> +{
> +	int all = 0;
> +	int i;
> +
> +	report_prefix_push("intercept");
> +
> +	if (argc < 2 || (argc == 2 && !strcmp(argv[1], "all")))
> +		all = 1;
> +
> +	for (i = 0; tests[i].name != NULL; i++) {
> +		report_prefix_push(tests[i].name);
> +		if (all || strcmp(argv[1], tests[i].name) == 0) {
> +			tests[i].func();
> +		}
> +		report_prefix_pop();
> +	}
> +
> +	report_prefix_pop();
> +
> +	return report_summary();
> +}
> diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg
> index 92e01ab..3b6b892 100644
> --- a/s390x/unittests.cfg
> +++ b/s390x/unittests.cfg
> @@ -22,3 +22,6 @@
>  file = selftest.elf
>  groups = selftest
>  extra_params = -append 'test 123'
> +
> +[intercept]
> +file = intercept.elf
> 

Nice! With or without these nits fixed.

Once upstream, I'll add a stidp test.

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 

Thanks,

David