Re: [kvm-unit-tests PATCH] s390x: Add specification exception test

[Janis Schoetterl-Glausch <scgl@linux.vnet.ibm.com>]

On 7/21/21 3:26 PM, Thomas Huth wrote:
> On 06/07/2021 13.54, Janis Schoetterl-Glausch wrote:
>> Generate specification exceptions and check that they occur.
>> Also generate specification exceptions during a transaction,
>> which results in another interruption code.
>> With the iterations argument one can check if specification
>> exception interpretation occurs, e.g. by using a high value and
>> checking that the debugfs counters are substantially lower.
>> The argument is also useful for estimating the performance benefit
>> of interpretation.
>>
>> Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
>> ---
>>   s390x/Makefile           |   1 +
>>   lib/s390x/asm/arch_def.h |   1 +
>>   s390x/spec_ex.c          | 344 +++++++++++++++++++++++++++++++++++++++
>>   s390x/unittests.cfg      |   3 +
>>   4 files changed, 349 insertions(+)
>>   create mode 100644 s390x/spec_ex.c
>>
>> diff --git a/s390x/Makefile b/s390x/Makefile
>> index 8820e99..be100d3 100644
>> --- a/s390x/Makefile
>> +++ b/s390x/Makefile
>> @@ -23,6 +23,7 @@ tests += $(TEST_DIR)/sie.elf
>>   tests += $(TEST_DIR)/mvpg.elf
>>   tests += $(TEST_DIR)/uv-host.elf
>>   tests += $(TEST_DIR)/edat.elf
>> +tests += $(TEST_DIR)/spec_ex.elf
>>     tests_binary = $(patsubst %.elf,%.bin,$(tests))
>>   ifneq ($(HOST_KEY_DOCUMENT),)
>> diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
>> index 15cf7d4..7cb0b92 100644
>> --- a/lib/s390x/asm/arch_def.h
>> +++ b/lib/s390x/asm/arch_def.h
>> @@ -229,6 +229,7 @@ static inline uint64_t stctg(int cr)
>>       return value;
>>   }
>>   +#define CTL0_TRANSACT_EX_CTL    (63 -  8)
>>   #define CTL0_LOW_ADDR_PROT    (63 - 35)
>>   #define CTL0_EDAT        (63 - 40)
>>   #define CTL0_IEP        (63 - 43)
>> diff --git a/s390x/spec_ex.c b/s390x/spec_ex.c
>> new file mode 100644
>> index 0000000..2e05bfb
>> --- /dev/null
>> +++ b/s390x/spec_ex.c
>> @@ -0,0 +1,344 @@
> 
> Please add a short comment header at the top of the file with some information on what it is all about, and license information (e.g. a SPDX-License-Identifier)
> 
>> +#include <stdlib.h>
>> +#include <htmintrin.h>
>> +#include <libcflat.h>
>> +#include <asm/barrier.h>
>> +#include <asm/interrupt.h>
>> +#include <asm/facility.h>
>> +
>> +struct lowcore *lc = (struct lowcore *) 0;
>> +
>> +static bool expect_early;
>> +static struct psw expected_early_pgm_psw;
>> +static struct psw fixup_early_pgm_psw;
>> +
>> +static void fixup_early_pgm_ex(void)
> 
> Could you please add a comment in front of this function with a description why this is required / good for?

Sure, how about:

/* The standard program exception handler cannot deal with invalid old PSWs,
 * especially not invalid instruction addresses, as in that case one cannot
 * find the instruction following the faulting one from the old PSW.
 */

I'll also change some names since something like this is necessary for all
exceptions caused by invalid PSWs, not just the early ones:

static void fixup_invalid_psw(void)
> 
>> +{
>> +    if (expect_early) {
>> +        report(expected_early_pgm_psw.mask == lc->pgm_old_psw.mask
>> +               && expected_early_pgm_psw.addr == lc->pgm_old_psw.addr,
>> +               "Early program new PSW as expected");
>> +        expect_early = false;
>> +    }
>> +    lc->pgm_old_psw = fixup_early_pgm_psw;
>> +}
>> +
>> +static void lpsw(uint64_t psw)
>> +{
>> +    uint32_t *high, *low;
>> +    uint64_t r0 = 0, r1 = 0;
>> +
>> +    high = (uint32_t *) &fixup_early_pgm_psw.mask;
>> +    low = high + 1;
>> +
>> +    asm volatile (
>> +        "    epsw    %0,%1\n"
>> +        "    st    %0,%[high]\n"
>> +        "    st    %1,%[low]\n"
> 
> What's all this magic with high and low good for? Looks like high and low are not used afterwards anymore?

Seems like the easiest way to store both halves of the current mask into the global fixup PSW.
> 
>> +        "    larl    %0,nop%=\n"
>> +        "    stg    %0,%[addr]\n"
>> +        "    lpsw    %[psw]\n"
>> +        "nop%=:    nop\n"
>> +        : "+&r"(r0), "+&a"(r1), [high] "=&R"(*high), [low] "=&R"(*low)
> 
> ... also not sure why you need the "&" modifiers here?

r0, r1 are stored into before reading psw, also there are implied input registers for the
memory output operands. To be honest, I didn't care to figure out the minimal '&' usage,
it's just test code after all.
> 
>> +        , [addr] "=&R"(fixup_early_pgm_psw.addr)
>> +        : [psw] "Q"(psw)
>> +        : "cc", "memory"
>> +    );
>> +}
>> +
>> +static void psw_bit_31_32_are_1_0(void)
>> +{
>> +    uint64_t bad_psw = 0x000800015eadbeef;
>> +
>> +    //bit 12 gets inverted when extending to 128-bit PSW
> 
> I'd prefer a space after the "//"
> 
>> +    expected_early_pgm_psw.mask = 0x0000000100000000;
>> +    expected_early_pgm_psw.addr = 0x000000005eadbeef;
>> +    expect_early = true;
>> +    lpsw(bad_psw);
>> +}
>> +
>> +static void bad_alignment(void)
>> +{
>> +    uint32_t words[5] = {0, 0, 0};
>> +    uint32_t (*bad_aligned)[4];
>> +
>> +    register uint64_t r1 asm("6");
>> +    register uint64_t r2 asm("7");
>> +    if (((uintptr_t)&words[0]) & 0xf) {
>> +        bad_aligned = (uint32_t (*)[4])&words[0];
>> +    } else {
>> +        bad_aligned = (uint32_t (*)[4])&words[1];
>> +    }
>> +    asm volatile ("lpq %0,%2"
>> +              : "=r"(r1), "=r"(r2)
>> +              : "T"(*bad_aligned)
>> +    );
>> +}
>> +
>> +static void not_even(void)
>> +{
>> +    uint64_t quad[2];
>> +
>> +    register uint64_t r1 asm("7");
>> +    register uint64_t r2 asm("8");
>> +    asm volatile (".insn    rxy,0xe3000000008f,%0,%2" //lpq %0,%2
>> +              : "=r"(r1), "=r"(r2)
>> +              : "T"(quad)
>> +    );
>> +}
>> +
>> +struct spec_ex_trigger {
>> +    const char *name;
>> +    void (*func)(void);
>> +    bool transactable;
>> +    void (*fixup)(void);
>> +};
>> +
>> +static const struct spec_ex_trigger spec_ex_triggers[] = {
>> +    { "psw_bit_31_32_are_1_0", &psw_bit_31_32_are_1_0, false, &fixup_early_pgm_ex},
>> +    { "bad_alignment", &bad_alignment, true, NULL},
>> +    { "not_even", &not_even, true, NULL},
>> +    { NULL, NULL, true, NULL},
>> +};
>> +
>> +struct args {
>> +    uint64_t iterations;
>> +    uint64_t max_retries;
>> +    uint64_t suppress_info;
>> +    uint64_t max_failures;
>> +    bool diagnose;
>> +};
>> +
>> +static void test_spec_ex(struct args *args,
>> +             const struct spec_ex_trigger *trigger)
>> +{
>> +    uint16_t expected_pgm = PGM_INT_CODE_SPECIFICATION;
>> +    uint16_t pgm;
>> +    unsigned int i;
>> +
>> +    register_pgm_cleanup_func(trigger->fixup);
>> +    for (i = 0; i < args->iterations; i++) {
>> +        expect_pgm_int();
>> +        trigger->func();
>> +        pgm = clear_pgm_int();
>> +        if (pgm != expected_pgm) {
>> +            report(0,
>> +            "Program interrupt: expected(%d) == received(%d)",
>> +            expected_pgm,
>> +            pgm);
>> +            return;
>> +        }
>> +    }
> 
> Maybe it would be nice to "unregister" the cleanup function at the end with register_pgm_cleanup_func(NULL) ?

Yeah, I think I'll also move them just before and after the trigger->func().
> 
>> +    report(1,
>> +    "Program interrupt: always expected(%d) == received(%d)",
>> +    expected_pgm,
>> +    expected_pgm);
>> +}
>> +
>> +#define TRANSACTION_COMPLETED 4
>> +#define TRANSACTION_MAX_RETRIES 5
>> +
>> +static int __attribute__((nonnull))
> 
> Not sure whether that attribute makes much sense with a static function? ... the compiler has information about the implementation details here, so it should be able to see that e.g. trigger must be non-NULL anyway?

One isn't supposed to pass NULL to __builtin_tbegin via a variable, only via a constant.
I didn't want to deal with that constraint, so that's what the nonnull is there for.
Maybe I should add a comment?
> 
>> +with_transaction(void (*trigger)(void), struct __htm_tdb *diagnose)
>> +{
>> +    int cc;
>> +
>> +    cc = __builtin_tbegin(diagnose);
>> +    if (cc == _HTM_TBEGIN_STARTED) {
>> +        trigger();
>> +        __builtin_tend();
>> +        return -TRANSACTION_COMPLETED;
>> +    } else {
>> +        return -cc;
>> +    }
>> +}
> [...]
> 
>  Thomas
> 
Thank you for your comments.