On 02.06.2017 15:49, David Hildenbrand wrote:
> 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?
It's got to be %%c0 ... not sure whether this looks really nicer here?
>> + 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.
OK, I can do that (I guess I have to respin anyway).
>> +
>> +/* 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.
Sorry, I can't follow you here ... Do you mean to just split the
report() or the whole sequence of assembler instructions?
>> +
>> + 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.
I guess I could also work with report_prefix_push() here ... not sure
whether that looks nicer ... will give it a try...
>> +
>> + 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 ?
That's certainly wrong, since %0 is the reference to the first output
parameter (cc). You likely mean %%0 or %%r0 ... and that looks rather
cumbersome, too. I think I prefer the plain "0" here, what do you think?
>> + " 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>
>
