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@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxx>
> + *
> + * 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@xxxxxxxxxx>
--
Thanks,
David
