* [kvm-unit-tests PATCH v5 0/2] Add specification exception tests
@ 2022-07-20 14:25 Janis Schoetterl-Glausch
2022-07-20 14:25 ` [kvm-unit-tests PATCH v5 1/2] s390x: Add specification exception test Janis Schoetterl-Glausch
2022-07-20 14:25 ` [kvm-unit-tests PATCH v5 2/2] s390x: Test specification exceptions during transaction Janis Schoetterl-Glausch
0 siblings, 2 replies; 7+ messages in thread
From: Janis Schoetterl-Glausch @ 2022-07-20 14:25 UTC (permalink / raw)
To: Thomas Huth, Janosch Frank, Claudio Imbrenda, David Hildenbrand,
Richard Henderson
Cc: Janis Schoetterl-Glausch, kvm, linux-s390, qemu-s390x
Test that specification exceptions cause the correct interruption code
during both normal and transactional execution.
TCG fails the tests setting an invalid PSW bit.
I had a look at how best to fix it, but where best to check for early
PSW exceptions was not immediately clear to me. Ideas welcome.
v4 -> v5
add lpsw with invalid bit 12 test
TCG fails as with lpswe but must also invert bit 12
update copyright statement
add comments
cleanups and style fixes
v3 -> v4
remove iterations argument in order to simplify the code
for manual performance testing adding a for loop is easy
move report out of fixup_invalid_psw
simplify/improve readability of triggers
use positive error values
v2 -> v3
remove non-ascii symbol
clean up load_psw
fix nits
v1 -> v2
Add license and test description
Split test patch into normal test and transactional execution test
Add comments to
invalid PSW fixup function
with_transaction
Rename some variables/functions
Pass mask as single parameter to asm
Get rid of report_info_if macro
Introduce report_pass/fail and use them
Janis Schoetterl-Glausch (2):
s390x: Add specification exception test
s390x: Test specification exceptions during transaction
s390x/Makefile | 1 +
lib/s390x/asm/arch_def.h | 6 +
s390x/spec_ex.c | 369 +++++++++++++++++++++++++++++++++++++++
s390x/unittests.cfg | 3 +
4 files changed, 379 insertions(+)
create mode 100644 s390x/spec_ex.c
Range-diff against v4:
1: a242e84b ! 1: fd9780d8 s390x: Add specification exception test
@@ s390x/Makefile: tests += $(TEST_DIR)/uv-host.elf
tests += $(TEST_DIR)/spec_ex-sie.elf
+tests += $(TEST_DIR)/spec_ex.elf
tests += $(TEST_DIR)/firq.elf
+ tests += $(TEST_DIR)/epsw.elf
+ tests += $(TEST_DIR)/adtl-status.elf
+
+ ## lib/s390x/asm/arch_def.h ##
+@@ lib/s390x/asm/arch_def.h: struct psw {
+ uint64_t addr;
+ };
- tests_binary = $(patsubst %.elf,%.bin,$(tests))
++struct short_psw {
++ uint32_t mask;
++ uint32_t addr;
++};
++
+ #define AS_PRIM 0
+ #define AS_ACCR 1
+ #define AS_SECN 2
## s390x/spec_ex.c (new) ##
@@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
-+ * Copyright IBM Corp. 2021
++ * Copyright IBM Corp. 2021, 2022
+ *
+ * Specification exception test.
+ * Tests that specification exceptions occur when expected.
@@ s390x/spec_ex.c (new)
+#include <libcflat.h>
+#include <asm/interrupt.h>
+
-+static struct lowcore *lc = (struct lowcore *) 0;
-+
+static bool invalid_psw_expected;
+static struct psw expected_psw;
+static struct psw invalid_psw;
+static struct psw fixup_psw;
+
-+/* The standard program exception handler cannot deal with invalid old PSWs,
++/*
++ * 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.
+ * The PSW to return to is set by load_psw.
+ */
+static void fixup_invalid_psw(void)
+{
-+ // signal occurrence of invalid psw fixup
++ /* signal occurrence of invalid psw fixup */
+ invalid_psw_expected = false;
-+ invalid_psw = lc->pgm_old_psw;
-+ lc->pgm_old_psw = fixup_psw;
++ invalid_psw = lowcore.pgm_old_psw;
++ lowcore.pgm_old_psw = fixup_psw;
+}
+
-+/* Load possibly invalid psw, but setup fixup_psw before,
-+ * so that *fixup_invalid_psw() can bring us back onto the right track.
++/*
++ * Load possibly invalid psw, but setup fixup_psw before,
++ * so that fixup_invalid_psw() can bring us back onto the right track.
+ * Also acts as compiler barrier, -> none required in expect/check_invalid_psw
+ */
+static void load_psw(struct psw psw)
@@ s390x/spec_ex.c (new)
+ uint64_t scratch;
+
+ fixup_psw.mask = extract_psw_mask();
-+ asm volatile ( "larl %[scratch],nop%=\n"
++ asm volatile ( "larl %[scratch],0f\n"
+ " stg %[scratch],%[addr]\n"
+ " lpswe %[psw]\n"
-+ "nop%=: nop\n"
-+ : [scratch] "=&r"(scratch),
++ "0: nop\n"
++ : [scratch] "=&d"(scratch),
++ [addr] "=&T"(fixup_psw.addr)
++ : [psw] "Q"(psw)
++ : "cc", "memory"
++ );
++}
++
++static void load_short_psw(struct short_psw psw)
++{
++ uint64_t scratch;
++
++ fixup_psw.mask = extract_psw_mask();
++ asm volatile ( "larl %[scratch],0f\n"
++ " stg %[scratch],%[addr]\n"
++ " lpsw %[psw]\n"
++ "0: nop\n"
++ : [scratch] "=&d"(scratch),
+ [addr] "=&T"(fixup_psw.addr)
+ : [psw] "Q"(psw)
+ : "cc", "memory"
@@ s390x/spec_ex.c (new)
+
+static int check_invalid_psw(void)
+{
-+ // toggled to signal occurrence of invalid psw fixup
++ /* toggled to signal occurrence of invalid psw fixup */
+ if (!invalid_psw_expected) {
+ if (expected_psw.mask == invalid_psw.mask &&
+ expected_psw.addr == invalid_psw.addr)
@@ s390x/spec_ex.c (new)
+ struct psw invalid = { .mask = 0x0008000000000000, .addr = 0x00000000deadbeee};
+
+ expect_invalid_psw(invalid);
-+ load_psw(expected_psw);
++ load_psw(invalid);
+ return check_invalid_psw();
+}
+
++static int short_psw_bit_12_is_0(void)
++{
++ struct short_psw short_invalid = { .mask = 0x00000000, .addr = 0xdeadbeee};
++
++ /*
++ * lpsw may optionally check bit 12 before loading the new psw
++ * -> cannot check the expected invalid psw like with lpswe
++ */
++ load_short_psw(short_invalid);
++ return 0;
++}
++
+static int bad_alignment(void)
+{
+ uint32_t words[5] __attribute__((aligned(16)));
@@ s390x/spec_ex.c (new)
+{
+ uint64_t quad[2] __attribute__((aligned(16))) = {0};
+
-+ asm volatile (".insn rxy,0xe3000000008f,%%r7,%[quad]" //lpq %%r7,%[quad]
++ asm volatile (".insn rxy,0xe3000000008f,%%r7,%[quad]" /* lpq %%r7,%[quad] */
+ : : [quad] "T"(quad)
+ : "%r7", "%r8"
+ );
@@ s390x/spec_ex.c (new)
+/* List of all tests to execute */
+static const struct spec_ex_trigger spec_ex_triggers[] = {
+ { "psw_bit_12_is_1", &psw_bit_12_is_1, &fixup_invalid_psw },
++ { "short_psw_bit_12_is_0", &short_psw_bit_12_is_0, &fixup_invalid_psw },
+ { "bad_alignment", &bad_alignment, NULL },
+ { "not_even", ¬_even, NULL },
+ { NULL, NULL, NULL },
@@ s390x/spec_ex.c (new)
+
+static void test_spec_ex(const struct spec_ex_trigger *trigger)
+{
-+ uint16_t expected_pgm = PGM_INT_CODE_SPECIFICATION;
-+ uint16_t pgm;
+ int rc;
+
+ expect_pgm_int();
+ register_pgm_cleanup_func(trigger->fixup);
+ rc = trigger->func();
+ register_pgm_cleanup_func(NULL);
++ /* test failed, nothing to be done, reporting responsibility of trigger */
+ if (rc)
+ return;
-+ pgm = clear_pgm_int();
-+ report(pgm == expected_pgm, "Program interrupt: expected(%d) == received(%d)",
-+ expected_pgm, pgm);
++ check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
+}
+
+int main(int argc, char **argv)
@@ s390x/unittests.cfg: file = mvpg-sie.elf
+[spec_ex]
+file = spec_ex.elf
+
- [firq-linear-cpu-ids]
+ [firq-linear-cpu-ids-kvm]
file = firq.elf
timeout = 20
2: 16ce8bb0 ! 2: c14092a3 s390x: Test specification exceptions during transaction
@@ Commit message
Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
## lib/s390x/asm/arch_def.h ##
-@@ lib/s390x/asm/arch_def.h: struct psw {
+@@ lib/s390x/asm/arch_def.h: struct short_psw {
#define PSW_MASK_BA 0x0000000080000000UL
#define PSW_MASK_64 (PSW_MASK_BA | PSW_MASK_EA)
-+#define CTL0_TRANSACT_EX_CTL (63 - 8)
- #define CTL0_LOW_ADDR_PROT (63 - 35)
- #define CTL0_EDAT (63 - 40)
- #define CTL0_IEP (63 - 43)
++#define CTL0_TRANSACT_EX_CTL (63 - 8)
+ #define CTL0_LOW_ADDR_PROT (63 - 35)
+ #define CTL0_EDAT (63 - 40)
+ #define CTL0_FETCH_PROTECTION_OVERRIDE (63 - 38)
## s390x/spec_ex.c ##
@@
@@ s390x/spec_ex.c
#include <asm/interrupt.h>
+#include <asm/facility.h>
- static struct lowcore *lc = (struct lowcore *) 0;
-
+ static bool invalid_psw_expected;
+ static struct psw expected_psw;
@@ s390x/spec_ex.c: static int not_even(void)
/*
* Harness for specification exception testing.
@@ s390x/spec_ex.c: static int not_even(void)
/* List of all tests to execute */
static const struct spec_ex_trigger spec_ex_triggers[] = {
- { "psw_bit_12_is_1", &psw_bit_12_is_1, &fixup_invalid_psw },
+- { "short_psw_bit_12_is_0", &short_psw_bit_12_is_0, &fixup_invalid_psw },
- { "bad_alignment", &bad_alignment, NULL },
- { "not_even", ¬_even, NULL },
- { NULL, NULL, NULL },
+ { "psw_bit_12_is_1", &psw_bit_12_is_1, false, &fixup_invalid_psw },
++ { "short_psw_bit_12_is_0", &short_psw_bit_12_is_0, false, &fixup_invalid_psw },
+ { "bad_alignment", &bad_alignment, true, NULL },
+ { "not_even", ¬_even, true, NULL },
+ { NULL, NULL, false, NULL },
@@ s390x/spec_ex.c: static int not_even(void)
static void test_spec_ex(const struct spec_ex_trigger *trigger)
@@ s390x/spec_ex.c: static void test_spec_ex(const struct spec_ex_trigger *trigger)
- expected_pgm, pgm);
+ check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
}
+#define TRANSACTION_COMPLETED 4
+#define TRANSACTION_MAX_RETRIES 5
+
-+/* NULL must be passed to __builtin_tbegin via constant, forbid diagnose from
++/*
++ * NULL must be passed to __builtin_tbegin via constant, forbid diagnose from
+ * being NULL to keep things simple
+ */
+static int __attribute__((nonnull))
@@ s390x/spec_ex.c: static void test_spec_ex(const struct spec_ex_trigger *trigger)
+ int cc;
+
+ cc = __builtin_tbegin(diagnose);
++ /*
++ * Everything between tbegin and tend is part of the transaction,
++ * which either completes in its entirety or does not have any effect.
++ * If the transaction fails, execution is reset to this point with another
++ * condition code indicating why the transaction failed.
++ */
+ if (cc == _HTM_TBEGIN_STARTED) {
-+ /* return code is meaningless: transaction needs to complete
++ /*
++ * return code is meaningless: transaction needs to complete
+ * in order to return and completion indicates a test failure
+ */
+ trigger();
@@ s390x/spec_ex.c: static void test_spec_ex(const struct spec_ex_trigger *trigger)
+ trans_result = with_transaction(trigger->func, tdb);
+ if (trans_result == _HTM_TBEGIN_TRANSIENT) {
+ mb();
-+ pgm = lc->pgm_int_code;
-+ if (pgm == 0)
-+ continue;
-+ else if (pgm == expected_pgm)
++ pgm = lowcore.pgm_int_code;
++ if (pgm == expected_pgm)
+ return 0;
++ else if (pgm == 0)
++ /*
++ * Transaction failed for unknown reason but not because
++ * of an unexpected program exception. Give it another
++ * go so that hopefully it reaches the triggering instruction.
++ */
++ continue;
+ }
+ return trans_result;
+ }
@@ s390x/spec_ex.c: static void test_spec_ex(const struct spec_ex_trigger *trigger)
+ report_fail("Transaction completed without exception");
+ break;
+ case TRANSACTION_MAX_RETRIES:
-+ report_info("Retried transaction %lu times without exception",
++ report_skip("Transaction retried %lu times with transient failures, giving up",
+ args->max_retries);
+ break;
+ default:
-+ report_fail("Invalid return transaction result");
++ report_fail("Invalid transaction result");
+ break;
+ }
+
+ ctl_clear_bit(0, CTL0_TRANSACT_EX_CTL);
+}
+
++static bool parse_unsigned(const char *arg, unsigned int *out)
++{
++ char *end;
++ long num;
++
++ if (arg[0] == '\0')
++ return false;
++ num = strtol(arg, &end, 10);
++ if (end[0] != '\0' || num < 0)
++ return false;
++ *out = num;
++ return true;
++}
++
+static struct args parse_args(int argc, char **argv)
+{
+ struct args args = {
+ .max_retries = 20,
+ .diagnose = false
+ };
-+ unsigned int i;
-+ long arg;
-+ bool no_arg;
-+ char *end;
++ unsigned int i, arg;
++ bool has_arg;
+ const char *flag;
-+ uint64_t *argp;
+
+ for (i = 1; i < argc; i++) {
-+ no_arg = true;
-+ if (i < argc - 1) {
-+ no_arg = *argv[i + 1] == '\0';
-+ arg = strtol(argv[i + 1], &end, 10);
-+ no_arg |= *end != '\0';
-+ no_arg |= arg < 0;
-+ }
++ if (i + 1 < argc)
++ has_arg = parse_unsigned(argv[i + 1], &arg);
++ else
++ has_arg = false;
+
+ flag = "--max-retries";
-+ argp = &args.max_retries;
+ if (!strcmp(flag, argv[i])) {
-+ if (no_arg)
++ if (!has_arg)
+ report_abort("%s needs a positive parameter", flag);
-+ *argp = arg;
++ args.max_retries = arg;
+ ++i;
+ continue;
+ }
base-commit: ca85dda2671e88d34acfbca6de48a9ab32b1810d
--
2.36.1
^ permalink raw reply [flat|nested] 7+ messages in thread
* [kvm-unit-tests PATCH v5 1/2] s390x: Add specification exception test
2022-07-20 14:25 [kvm-unit-tests PATCH v5 0/2] Add specification exception tests Janis Schoetterl-Glausch
@ 2022-07-20 14:25 ` Janis Schoetterl-Glausch
2022-08-24 9:35 ` Janosch Frank
2022-07-20 14:25 ` [kvm-unit-tests PATCH v5 2/2] s390x: Test specification exceptions during transaction Janis Schoetterl-Glausch
1 sibling, 1 reply; 7+ messages in thread
From: Janis Schoetterl-Glausch @ 2022-07-20 14:25 UTC (permalink / raw)
To: Thomas Huth, Janosch Frank, Claudio Imbrenda, David Hildenbrand,
Richard Henderson
Cc: Janis Schoetterl-Glausch, kvm, linux-s390, qemu-s390x
Generate specification exceptions and check that they occur.
Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
---
s390x/Makefile | 1 +
lib/s390x/asm/arch_def.h | 5 ++
s390x/spec_ex.c | 180 +++++++++++++++++++++++++++++++++++++++
s390x/unittests.cfg | 3 +
4 files changed, 189 insertions(+)
create mode 100644 s390x/spec_ex.c
diff --git a/s390x/Makefile b/s390x/Makefile
index efd5e0c1..58b1bf54 100644
--- a/s390x/Makefile
+++ b/s390x/Makefile
@@ -27,6 +27,7 @@ tests += $(TEST_DIR)/uv-host.elf
tests += $(TEST_DIR)/edat.elf
tests += $(TEST_DIR)/mvpg-sie.elf
tests += $(TEST_DIR)/spec_ex-sie.elf
+tests += $(TEST_DIR)/spec_ex.elf
tests += $(TEST_DIR)/firq.elf
tests += $(TEST_DIR)/epsw.elf
tests += $(TEST_DIR)/adtl-status.elf
diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
index 78b257b7..8fbc451c 100644
--- a/lib/s390x/asm/arch_def.h
+++ b/lib/s390x/asm/arch_def.h
@@ -41,6 +41,11 @@ struct psw {
uint64_t addr;
};
+struct short_psw {
+ uint32_t mask;
+ uint32_t addr;
+};
+
#define AS_PRIM 0
#define AS_ACCR 1
#define AS_SECN 2
diff --git a/s390x/spec_ex.c b/s390x/spec_ex.c
new file mode 100644
index 00000000..77fc6246
--- /dev/null
+++ b/s390x/spec_ex.c
@@ -0,0 +1,180 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright IBM Corp. 2021, 2022
+ *
+ * Specification exception test.
+ * Tests that specification exceptions occur when expected.
+ *
+ * Can be extended by adding triggers to spec_ex_triggers, see comments below.
+ */
+#include <stdlib.h>
+#include <libcflat.h>
+#include <asm/interrupt.h>
+
+static bool invalid_psw_expected;
+static struct psw expected_psw;
+static struct psw invalid_psw;
+static struct psw fixup_psw;
+
+/*
+ * 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.
+ * The PSW to return to is set by load_psw.
+ */
+static void fixup_invalid_psw(void)
+{
+ /* signal occurrence of invalid psw fixup */
+ invalid_psw_expected = false;
+ invalid_psw = lowcore.pgm_old_psw;
+ lowcore.pgm_old_psw = fixup_psw;
+}
+
+/*
+ * Load possibly invalid psw, but setup fixup_psw before,
+ * so that fixup_invalid_psw() can bring us back onto the right track.
+ * Also acts as compiler barrier, -> none required in expect/check_invalid_psw
+ */
+static void load_psw(struct psw psw)
+{
+ uint64_t scratch;
+
+ fixup_psw.mask = extract_psw_mask();
+ asm volatile ( "larl %[scratch],0f\n"
+ " stg %[scratch],%[addr]\n"
+ " lpswe %[psw]\n"
+ "0: nop\n"
+ : [scratch] "=&d"(scratch),
+ [addr] "=&T"(fixup_psw.addr)
+ : [psw] "Q"(psw)
+ : "cc", "memory"
+ );
+}
+
+static void load_short_psw(struct short_psw psw)
+{
+ uint64_t scratch;
+
+ fixup_psw.mask = extract_psw_mask();
+ asm volatile ( "larl %[scratch],0f\n"
+ " stg %[scratch],%[addr]\n"
+ " lpsw %[psw]\n"
+ "0: nop\n"
+ : [scratch] "=&d"(scratch),
+ [addr] "=&T"(fixup_psw.addr)
+ : [psw] "Q"(psw)
+ : "cc", "memory"
+ );
+}
+
+static void expect_invalid_psw(struct psw psw)
+{
+ expected_psw = psw;
+ invalid_psw_expected = true;
+}
+
+static int check_invalid_psw(void)
+{
+ /* toggled to signal occurrence of invalid psw fixup */
+ if (!invalid_psw_expected) {
+ if (expected_psw.mask == invalid_psw.mask &&
+ expected_psw.addr == invalid_psw.addr)
+ return 0;
+ report_fail("Wrong invalid PSW");
+ } else {
+ report_fail("Expected exception due to invalid PSW");
+ }
+ return 1;
+}
+
+static int psw_bit_12_is_1(void)
+{
+ struct psw invalid = { .mask = 0x0008000000000000, .addr = 0x00000000deadbeee};
+
+ expect_invalid_psw(invalid);
+ load_psw(invalid);
+ return check_invalid_psw();
+}
+
+static int short_psw_bit_12_is_0(void)
+{
+ struct short_psw short_invalid = { .mask = 0x00000000, .addr = 0xdeadbeee};
+
+ /*
+ * lpsw may optionally check bit 12 before loading the new psw
+ * -> cannot check the expected invalid psw like with lpswe
+ */
+ load_short_psw(short_invalid);
+ return 0;
+}
+
+static int bad_alignment(void)
+{
+ uint32_t words[5] __attribute__((aligned(16)));
+ uint32_t (*bad_aligned)[4] = (uint32_t (*)[4])&words[1];
+
+ asm volatile ("lpq %%r6,%[bad]"
+ : : [bad] "T"(*bad_aligned)
+ : "%r6", "%r7"
+ );
+ return 0;
+}
+
+static int not_even(void)
+{
+ uint64_t quad[2] __attribute__((aligned(16))) = {0};
+
+ asm volatile (".insn rxy,0xe3000000008f,%%r7,%[quad]" /* lpq %%r7,%[quad] */
+ : : [quad] "T"(quad)
+ : "%r7", "%r8"
+ );
+ return 0;
+}
+
+/*
+ * Harness for specification exception testing.
+ * func only triggers exception, reporting is taken care of automatically.
+ */
+struct spec_ex_trigger {
+ const char *name;
+ int (*func)(void);
+ void (*fixup)(void);
+};
+
+/* List of all tests to execute */
+static const struct spec_ex_trigger spec_ex_triggers[] = {
+ { "psw_bit_12_is_1", &psw_bit_12_is_1, &fixup_invalid_psw },
+ { "short_psw_bit_12_is_0", &short_psw_bit_12_is_0, &fixup_invalid_psw },
+ { "bad_alignment", &bad_alignment, NULL },
+ { "not_even", ¬_even, NULL },
+ { NULL, NULL, NULL },
+};
+
+static void test_spec_ex(const struct spec_ex_trigger *trigger)
+{
+ int rc;
+
+ expect_pgm_int();
+ register_pgm_cleanup_func(trigger->fixup);
+ rc = trigger->func();
+ register_pgm_cleanup_func(NULL);
+ /* test failed, nothing to be done, reporting responsibility of trigger */
+ if (rc)
+ return;
+ check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
+}
+
+int main(int argc, char **argv)
+{
+ unsigned int i;
+
+ report_prefix_push("specification exception");
+ for (i = 0; spec_ex_triggers[i].name; i++) {
+ report_prefix_push(spec_ex_triggers[i].name);
+ test_spec_ex(&spec_ex_triggers[i]);
+ report_prefix_pop();
+ }
+ report_prefix_pop();
+
+ return report_summary();
+}
diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg
index 8e52f560..d2740a40 100644
--- a/s390x/unittests.cfg
+++ b/s390x/unittests.cfg
@@ -113,6 +113,9 @@ file = mvpg-sie.elf
[spec_ex-sie]
file = spec_ex-sie.elf
+[spec_ex]
+file = spec_ex.elf
+
[firq-linear-cpu-ids-kvm]
file = firq.elf
timeout = 20
--
2.36.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [kvm-unit-tests PATCH v5 2/2] s390x: Test specification exceptions during transaction
2022-07-20 14:25 [kvm-unit-tests PATCH v5 0/2] Add specification exception tests Janis Schoetterl-Glausch
2022-07-20 14:25 ` [kvm-unit-tests PATCH v5 1/2] s390x: Add specification exception test Janis Schoetterl-Glausch
@ 2022-07-20 14:25 ` Janis Schoetterl-Glausch
1 sibling, 0 replies; 7+ messages in thread
From: Janis Schoetterl-Glausch @ 2022-07-20 14:25 UTC (permalink / raw)
To: Thomas Huth, Janosch Frank, Claudio Imbrenda, David Hildenbrand,
Richard Henderson
Cc: Janis Schoetterl-Glausch, kvm, linux-s390, qemu-s390x
Program interruptions during transactional execution cause other
interruption codes.
Check that we see the expected code for (some) specification exceptions.
Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
---
lib/s390x/asm/arch_def.h | 1 +
s390x/spec_ex.c | 199 ++++++++++++++++++++++++++++++++++++++-
2 files changed, 195 insertions(+), 5 deletions(-)
diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
index 8fbc451c..94b104ca 100644
--- a/lib/s390x/asm/arch_def.h
+++ b/lib/s390x/asm/arch_def.h
@@ -60,6 +60,7 @@ struct short_psw {
#define PSW_MASK_BA 0x0000000080000000UL
#define PSW_MASK_64 (PSW_MASK_BA | PSW_MASK_EA)
+#define CTL0_TRANSACT_EX_CTL (63 - 8)
#define CTL0_LOW_ADDR_PROT (63 - 35)
#define CTL0_EDAT (63 - 40)
#define CTL0_FETCH_PROTECTION_OVERRIDE (63 - 38)
diff --git a/s390x/spec_ex.c b/s390x/spec_ex.c
index 77fc6246..1f1d7209 100644
--- a/s390x/spec_ex.c
+++ b/s390x/spec_ex.c
@@ -4,12 +4,18 @@
*
* Specification exception test.
* Tests that specification exceptions occur when expected.
+ * This includes specification exceptions occurring during transactional execution
+ * as these result in another interruption code (the transactional-execution-aborted
+ * bit is set).
*
* Can be extended by adding triggers to spec_ex_triggers, see comments below.
*/
#include <stdlib.h>
+#include <htmintrin.h>
#include <libcflat.h>
+#include <asm/barrier.h>
#include <asm/interrupt.h>
+#include <asm/facility.h>
static bool invalid_psw_expected;
static struct psw expected_psw;
@@ -134,20 +140,22 @@ static int not_even(void)
/*
* Harness for specification exception testing.
* func only triggers exception, reporting is taken care of automatically.
+ * If a trigger is transactable it will also be executed during a transaction.
*/
struct spec_ex_trigger {
const char *name;
int (*func)(void);
+ bool transactable;
void (*fixup)(void);
};
/* List of all tests to execute */
static const struct spec_ex_trigger spec_ex_triggers[] = {
- { "psw_bit_12_is_1", &psw_bit_12_is_1, &fixup_invalid_psw },
- { "short_psw_bit_12_is_0", &short_psw_bit_12_is_0, &fixup_invalid_psw },
- { "bad_alignment", &bad_alignment, NULL },
- { "not_even", ¬_even, NULL },
- { NULL, NULL, NULL },
+ { "psw_bit_12_is_1", &psw_bit_12_is_1, false, &fixup_invalid_psw },
+ { "short_psw_bit_12_is_0", &short_psw_bit_12_is_0, false, &fixup_invalid_psw },
+ { "bad_alignment", &bad_alignment, true, NULL },
+ { "not_even", ¬_even, true, NULL },
+ { NULL, NULL, false, NULL },
};
static void test_spec_ex(const struct spec_ex_trigger *trigger)
@@ -164,10 +172,181 @@ static void test_spec_ex(const struct spec_ex_trigger *trigger)
check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
}
+#define TRANSACTION_COMPLETED 4
+#define TRANSACTION_MAX_RETRIES 5
+
+/*
+ * NULL must be passed to __builtin_tbegin via constant, forbid diagnose from
+ * being NULL to keep things simple
+ */
+static int __attribute__((nonnull))
+with_transaction(int (*trigger)(void), struct __htm_tdb *diagnose)
+{
+ int cc;
+
+ cc = __builtin_tbegin(diagnose);
+ /*
+ * Everything between tbegin and tend is part of the transaction,
+ * which either completes in its entirety or does not have any effect.
+ * If the transaction fails, execution is reset to this point with another
+ * condition code indicating why the transaction failed.
+ */
+ if (cc == _HTM_TBEGIN_STARTED) {
+ /*
+ * return code is meaningless: transaction needs to complete
+ * in order to return and completion indicates a test failure
+ */
+ trigger();
+ __builtin_tend();
+ return TRANSACTION_COMPLETED;
+ } else {
+ return cc;
+ }
+}
+
+static int retry_transaction(const struct spec_ex_trigger *trigger, unsigned int max_retries,
+ struct __htm_tdb *tdb, uint16_t expected_pgm)
+{
+ int trans_result, i;
+ uint16_t pgm;
+
+ for (i = 0; i < max_retries; i++) {
+ expect_pgm_int();
+ trans_result = with_transaction(trigger->func, tdb);
+ if (trans_result == _HTM_TBEGIN_TRANSIENT) {
+ mb();
+ pgm = lowcore.pgm_int_code;
+ if (pgm == expected_pgm)
+ return 0;
+ else if (pgm == 0)
+ /*
+ * Transaction failed for unknown reason but not because
+ * of an unexpected program exception. Give it another
+ * go so that hopefully it reaches the triggering instruction.
+ */
+ continue;
+ }
+ return trans_result;
+ }
+ return TRANSACTION_MAX_RETRIES;
+}
+
+struct args {
+ uint64_t max_retries;
+ bool diagnose;
+};
+
+static void test_spec_ex_trans(struct args *args, const struct spec_ex_trigger *trigger)
+{
+ const uint16_t expected_pgm = PGM_INT_CODE_SPECIFICATION
+ | PGM_INT_CODE_TX_ABORTED_EVENT;
+ union {
+ struct __htm_tdb tdb;
+ uint64_t dwords[sizeof(struct __htm_tdb) / sizeof(uint64_t)];
+ } diag;
+ unsigned int i;
+ int trans_result;
+
+ if (!test_facility(73)) {
+ report_skip("transactional-execution facility not installed");
+ return;
+ }
+ ctl_set_bit(0, CTL0_TRANSACT_EX_CTL); /* enable transactional-exec */
+
+ register_pgm_cleanup_func(trigger->fixup);
+ trans_result = retry_transaction(trigger, args->max_retries, &diag.tdb, expected_pgm);
+ register_pgm_cleanup_func(NULL);
+ switch (trans_result) {
+ case 0:
+ report_pass("Program interrupt: expected(%d) == received(%d)",
+ expected_pgm, expected_pgm);
+ break;
+ case _HTM_TBEGIN_INDETERMINATE:
+ case _HTM_TBEGIN_PERSISTENT:
+ report_info("transaction failed with cc %d", trans_result);
+ report_info("transaction abort code: %llu", diag.tdb.abort_code);
+ if (args->diagnose)
+ for (i = 0; i < 32; i++)
+ report_info("diag+%03d: %016lx", i * 8, diag.dwords[i]);
+ break;
+ case _HTM_TBEGIN_TRANSIENT:
+ report_fail("Program interrupt: expected(%d) == received(%d)",
+ expected_pgm, clear_pgm_int());
+ break;
+ case TRANSACTION_COMPLETED:
+ report_fail("Transaction completed without exception");
+ break;
+ case TRANSACTION_MAX_RETRIES:
+ report_skip("Transaction retried %lu times with transient failures, giving up",
+ args->max_retries);
+ break;
+ default:
+ report_fail("Invalid transaction result");
+ break;
+ }
+
+ ctl_clear_bit(0, CTL0_TRANSACT_EX_CTL);
+}
+
+static bool parse_unsigned(const char *arg, unsigned int *out)
+{
+ char *end;
+ long num;
+
+ if (arg[0] == '\0')
+ return false;
+ num = strtol(arg, &end, 10);
+ if (end[0] != '\0' || num < 0)
+ return false;
+ *out = num;
+ return true;
+}
+
+static struct args parse_args(int argc, char **argv)
+{
+ struct args args = {
+ .max_retries = 20,
+ .diagnose = false
+ };
+ unsigned int i, arg;
+ bool has_arg;
+ const char *flag;
+
+ for (i = 1; i < argc; i++) {
+ if (i + 1 < argc)
+ has_arg = parse_unsigned(argv[i + 1], &arg);
+ else
+ has_arg = false;
+
+ flag = "--max-retries";
+ if (!strcmp(flag, argv[i])) {
+ if (!has_arg)
+ report_abort("%s needs a positive parameter", flag);
+ args.max_retries = arg;
+ ++i;
+ continue;
+ }
+ if (!strcmp("--diagnose", argv[i])) {
+ args.diagnose = true;
+ continue;
+ }
+ if (!strcmp("--no-diagnose", argv[i])) {
+ args.diagnose = false;
+ continue;
+ }
+ report_abort("Unsupported parameter '%s'",
+ argv[i]);
+ }
+
+ return args;
+}
+
int main(int argc, char **argv)
{
unsigned int i;
+ struct args args = parse_args(argc, argv);
+
report_prefix_push("specification exception");
for (i = 0; spec_ex_triggers[i].name; i++) {
report_prefix_push(spec_ex_triggers[i].name);
@@ -176,5 +355,15 @@ int main(int argc, char **argv)
}
report_prefix_pop();
+ report_prefix_push("specification exception during transaction");
+ for (i = 0; spec_ex_triggers[i].name; i++) {
+ if (spec_ex_triggers[i].transactable) {
+ report_prefix_push(spec_ex_triggers[i].name);
+ test_spec_ex_trans(&args, &spec_ex_triggers[i]);
+ report_prefix_pop();
+ }
+ }
+ report_prefix_pop();
+
return report_summary();
}
--
2.36.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [kvm-unit-tests PATCH v5 1/2] s390x: Add specification exception test
2022-07-20 14:25 ` [kvm-unit-tests PATCH v5 1/2] s390x: Add specification exception test Janis Schoetterl-Glausch
@ 2022-08-24 9:35 ` Janosch Frank
2022-08-25 7:38 ` Janosch Frank
2022-08-26 11:23 ` Janis Schoetterl-Glausch
0 siblings, 2 replies; 7+ messages in thread
From: Janosch Frank @ 2022-08-24 9:35 UTC (permalink / raw)
To: Janis Schoetterl-Glausch, Thomas Huth, Claudio Imbrenda,
David Hildenbrand, Richard Henderson
Cc: kvm, linux-s390, qemu-s390x
On 7/20/22 16:25, Janis Schoetterl-Glausch wrote:
> Generate specification exceptions and check that they occur.
>
> Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
> ---
> s390x/Makefile | 1 +
> lib/s390x/asm/arch_def.h | 5 ++
> s390x/spec_ex.c | 180 +++++++++++++++++++++++++++++++++++++++
> s390x/unittests.cfg | 3 +
> 4 files changed, 189 insertions(+)
> create mode 100644 s390x/spec_ex.c
>
> diff --git a/s390x/Makefile b/s390x/Makefile
> index efd5e0c1..58b1bf54 100644
> --- a/s390x/Makefile
> +++ b/s390x/Makefile
> @@ -27,6 +27,7 @@ tests += $(TEST_DIR)/uv-host.elf
> tests += $(TEST_DIR)/edat.elf
> tests += $(TEST_DIR)/mvpg-sie.elf
> tests += $(TEST_DIR)/spec_ex-sie.elf
> +tests += $(TEST_DIR)/spec_ex.elf
> tests += $(TEST_DIR)/firq.elf
> tests += $(TEST_DIR)/epsw.elf
> tests += $(TEST_DIR)/adtl-status.elf
> diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
> index 78b257b7..8fbc451c 100644
> --- a/lib/s390x/asm/arch_def.h
> +++ b/lib/s390x/asm/arch_def.h
> @@ -41,6 +41,11 @@ struct psw {
> uint64_t addr;
> };
>
> +struct short_psw {
> + uint32_t mask;
> + uint32_t addr;
> +};
> +
> #define AS_PRIM 0
> #define AS_ACCR 1
> #define AS_SECN 2
> diff --git a/s390x/spec_ex.c b/s390x/spec_ex.c
> new file mode 100644
> index 00000000..77fc6246
> --- /dev/null
> +++ b/s390x/spec_ex.c
> @@ -0,0 +1,180 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright IBM Corp. 2021, 2022
> + *
> + * Specification exception test.
> + * Tests that specification exceptions occur when expected.
> + *
> + * Can be extended by adding triggers to spec_ex_triggers, see comments below.
> + */
> +#include <stdlib.h>
Which things are you hoping to include from stdlib.h?
As we normally use libcflat including external files can be pretty
dangerous.
> +#include <libcflat.h>
> +#include <asm/interrupt.h>
> +
> +static bool invalid_psw_expected;
> +static struct psw expected_psw;
> +static struct psw invalid_psw;
> +static struct psw fixup_psw;
> +
> +/*
> + * 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.
> + * The PSW to return to is set by load_psw.
> + */
> +static void fixup_invalid_psw(void)
> +{
> + /* signal occurrence of invalid psw fixup */
> + invalid_psw_expected = false;
> + invalid_psw = lowcore.pgm_old_psw;
> + lowcore.pgm_old_psw = fixup_psw;
> +}
> +
> +/*
> + * Load possibly invalid psw, but setup fixup_psw before,
> + * so that fixup_invalid_psw() can bring us back onto the right track.
> + * Also acts as compiler barrier, -> none required in expect/check_invalid_psw
> + */
> +static void load_psw(struct psw psw)
> +{
> + uint64_t scratch;
> +
/*
Store a valid mask and the address of the nop into the fixup PSW.
Then load the possibly invalid PSW.
*/
> + fixup_psw.mask = extract_psw_mask();
> + asm volatile ( "larl %[scratch],0f\n"
> + " stg %[scratch],%[addr]\n"
> + " lpswe %[psw]\n"
> + "0: nop\n"
> + : [scratch] "=&d"(scratch),
> + [addr] "=&T"(fixup_psw.addr)
s/addr/psw_addr/ ?
> + : [psw] "Q"(psw)
> + : "cc", "memory"
> + );
> +}
> +
> +static void load_short_psw(struct short_psw psw)
> +{
> + uint64_t scratch;
> +
> + fixup_psw.mask = extract_psw_mask();
> + asm volatile ( "larl %[scratch],0f\n"
> + " stg %[scratch],%[addr]\n"
> + " lpsw %[psw]\n"
> + "0: nop\n"
> + : [scratch] "=&d"(scratch),
> + [addr] "=&T"(fixup_psw.addr)
> + : [psw] "Q"(psw)
> + : "cc", "memory"
> + );
Same story.
> +}
> +
> +static void expect_invalid_psw(struct psw psw)
> +{
> + expected_psw = psw;
> + invalid_psw_expected = true;
> +}
> +
> +static int check_invalid_psw(void)
> +{
> + /* toggled to signal occurrence of invalid psw fixup */
That comment's location is a bit weird.
Move it to the declaration of the variable.
> + if (!invalid_psw_expected) {
> + if (expected_psw.mask == invalid_psw.mask &&
> + expected_psw.addr == invalid_psw.addr)
> + return 0;
> + report_fail("Wrong invalid PSW");
> + } else {
> + report_fail("Expected exception due to invalid PSW");
> + }
> + return 1;
> +}
> +
/* For normal PSWs bit 12 has to be 0 to be a valid PSW*/
> +static int psw_bit_12_is_1(void)
> +{
> + struct psw invalid = { .mask = 0x0008000000000000, .addr = 0x00000000deadbeee};
You could use BIT(63-12) for the mask.
I usually but struct initializations on new lines, it's easier to read.
> +
> + expect_invalid_psw(invalid);
> + load_psw(invalid);
> + return check_invalid_psw();
> +}
> +
/* A short PSW needs to have bit 12 set to be valid. */
> +static int short_psw_bit_12_is_0(void)
> +{
> + struct short_psw short_invalid = { .mask = 0x00000000, .addr = 0xdeadbeee};
I don't see a reason to specify more than one 0 if the whole value is 0.
> +
> + /*
> + * lpsw may optionally check bit 12 before loading the new psw
> + * -> cannot check the expected invalid psw like with lpswe
> + */
> + load_short_psw(short_invalid);
> + return 0;
> +}
> +
> +static int bad_alignment(void)
> +{
> + uint32_t words[5] __attribute__((aligned(16)));
> + uint32_t (*bad_aligned)[4] = (uint32_t (*)[4])&words[1];
> +
/* lpq loads a quad word into a register pair and requires quad word
alignment */
> + asm volatile ("lpq %%r6,%[bad]"
Of course there's an instruction for that...
> + : : [bad] "T"(*bad_aligned)
> + : "%r6", "%r7"
> + );
> + return 0;
> +}
> +
> +static int not_even(void)
> +{
> + uint64_t quad[2] __attribute__((aligned(16))) = {0};
> +
> + asm volatile (".insn rxy,0xe3000000008f,%%r7,%[quad]" /* lpq %%r7,%[quad] */
> + : : [quad] "T"(quad)
Is there a reason you never put a space after the constraint?
> + : "%r7", "%r8"
> + );
> + return 0;
> +}
> +
> +/*
> + * Harness for specification exception testing.
> + * func only triggers exception, reporting is taken care of automatically.
> + */
> +struct spec_ex_trigger {
> + const char *name;
> + int (*func)(void);
> + void (*fixup)(void);
> +};
> +
> +/* List of all tests to execute */
> +static const struct spec_ex_trigger spec_ex_triggers[] = {
> + { "psw_bit_12_is_1", &psw_bit_12_is_1, &fixup_invalid_psw },
> + { "short_psw_bit_12_is_0", &short_psw_bit_12_is_0, &fixup_invalid_psw },
> + { "bad_alignment", &bad_alignment, NULL },
> + { "not_even", ¬_even, NULL },
> + { NULL, NULL, NULL },
> +};
> +
> +static void test_spec_ex(const struct spec_ex_trigger *trigger)
> +{
> + int rc;
> +
> + expect_pgm_int();
> + register_pgm_cleanup_func(trigger->fixup);
> + rc = trigger->func();
> + register_pgm_cleanup_func(NULL);
> + /* test failed, nothing to be done, reporting responsibility of trigger */
> + if (rc)
> + return;
> + check_pgm_int_code(PGM_INT_CODE_SPECIFICATION);
> +}
> +
> +int main(int argc, char **argv)
> +{
> + unsigned int i;
> +
> + report_prefix_push("specification exception");
> + for (i = 0; spec_ex_triggers[i].name; i++) {
> + report_prefix_push(spec_ex_triggers[i].name);
> + test_spec_ex(&spec_ex_triggers[i]);
> + report_prefix_pop();
> + }
> + report_prefix_pop();
> +
> + return report_summary();
> +}
> diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg
> index 8e52f560..d2740a40 100644
> --- a/s390x/unittests.cfg
> +++ b/s390x/unittests.cfg
> @@ -113,6 +113,9 @@ file = mvpg-sie.elf
> [spec_ex-sie]
> file = spec_ex-sie.elf
>
> +[spec_ex]
> +file = spec_ex.elf
> +
> [firq-linear-cpu-ids-kvm]
> file = firq.elf
> timeout = 20
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [kvm-unit-tests PATCH v5 1/2] s390x: Add specification exception test
2022-08-24 9:35 ` Janosch Frank
@ 2022-08-25 7:38 ` Janosch Frank
2022-08-26 11:23 ` Janis Schoetterl-Glausch
1 sibling, 0 replies; 7+ messages in thread
From: Janosch Frank @ 2022-08-25 7:38 UTC (permalink / raw)
To: Janis Schoetterl-Glausch, Thomas Huth, Claudio Imbrenda,
David Hildenbrand, Richard Henderson
Cc: kvm, linux-s390, qemu-s390x
On 8/24/22 11:35, Janosch Frank wrote:
> On 7/20/22 16:25, Janis Schoetterl-Glausch wrote:
>> Generate specification exceptions and check that they occur.
>>
>> Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
>> ---
>> s390x/Makefile | 1 +
>> lib/s390x/asm/arch_def.h | 5 ++
>> s390x/spec_ex.c | 180 +++++++++++++++++++++++++++++++++++++++
>> s390x/unittests.cfg | 3 +
>> 4 files changed, 189 insertions(+)
>> create mode 100644 s390x/spec_ex.c
>>
>> diff --git a/s390x/Makefile b/s390x/Makefile
>> index efd5e0c1..58b1bf54 100644
>> --- a/s390x/Makefile
>> +++ b/s390x/Makefile
>> @@ -27,6 +27,7 @@ tests += $(TEST_DIR)/uv-host.elf
>> tests += $(TEST_DIR)/edat.elf
>> tests += $(TEST_DIR)/mvpg-sie.elf
>> tests += $(TEST_DIR)/spec_ex-sie.elf
>> +tests += $(TEST_DIR)/spec_ex.elf
>> tests += $(TEST_DIR)/firq.elf
>> tests += $(TEST_DIR)/epsw.elf
>> tests += $(TEST_DIR)/adtl-status.elf
>> diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
>> index 78b257b7..8fbc451c 100644
>> --- a/lib/s390x/asm/arch_def.h
>> +++ b/lib/s390x/asm/arch_def.h
>> @@ -41,6 +41,11 @@ struct psw {
>> uint64_t addr;
>> };
>>
>> +struct short_psw {
>> + uint32_t mask;
>> + uint32_t addr;
>> +};
>> +
>> #define AS_PRIM 0
>> #define AS_ACCR 1
>> #define AS_SECN 2
>> diff --git a/s390x/spec_ex.c b/s390x/spec_ex.c
>> new file mode 100644
>> index 00000000..77fc6246
>> --- /dev/null
>> +++ b/s390x/spec_ex.c
>> @@ -0,0 +1,180 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright IBM Corp. 2021, 2022
>> + *
>> + * Specification exception test.
>> + * Tests that specification exceptions occur when expected.
>> + *
>> + * Can be extended by adding triggers to spec_ex_triggers, see comments below.
>> + */
>> +#include <stdlib.h>
>
> Which things are you hoping to include from stdlib.h?
> As we normally use libcflat including external files can be pretty
> dangerous.
Ignore that comment, we have stdlib in the lib...
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [kvm-unit-tests PATCH v5 1/2] s390x: Add specification exception test
2022-08-24 9:35 ` Janosch Frank
2022-08-25 7:38 ` Janosch Frank
@ 2022-08-26 11:23 ` Janis Schoetterl-Glausch
2022-08-26 11:55 ` Janosch Frank
1 sibling, 1 reply; 7+ messages in thread
From: Janis Schoetterl-Glausch @ 2022-08-26 11:23 UTC (permalink / raw)
To: Janosch Frank, Thomas Huth, Claudio Imbrenda, David Hildenbrand,
Richard Henderson
Cc: kvm, linux-s390, qemu-s390x
On Wed, 2022-08-24 at 11:35 +0200, Janosch Frank wrote:
> On 7/20/22 16:25, Janis Schoetterl-Glausch wrote:
> > Generate specification exceptions and check that they occur.
> >
> > Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
> > ---
> > s390x/Makefile | 1 +
> > lib/s390x/asm/arch_def.h | 5 ++
> > s390x/spec_ex.c | 180 +++++++++++++++++++++++++++++++++++++++
> > s390x/unittests.cfg | 3 +
> > 4 files changed, 189 insertions(+)
> > create mode 100644 s390x/spec_ex.c
> >
> >
> > +
> > +/*
> > + * Load possibly invalid psw, but setup fixup_psw before,
> > + * so that fixup_invalid_psw() can bring us back onto the right track.
> > + * Also acts as compiler barrier, -> none required in expect/check_invalid_psw
> > + */
> > +static void load_psw(struct psw psw)
> > +{
> > + uint64_t scratch;
> > +
[...]
> /*
> Store a valid mask and the address of the nop into the fixup PSW.
> Then load the possibly invalid PSW.
> */
This seems a bit redundant given the function comment, but I can
drop a comment in here describing how the fixup psw is computed.
>
> > + fixup_psw.mask = extract_psw_mask();
> > + asm volatile ( "larl %[scratch],0f\n"
> > + " stg %[scratch],%[addr]\n"
> > + " lpswe %[psw]\n"
> > + "0: nop\n"
> > + : [scratch] "=&d"(scratch),
> > + [addr] "=&T"(fixup_psw.addr)
>
> s/addr/psw_addr/ ?
>
> > + : [psw] "Q"(psw)
> > + : "cc", "memory"
> > + );
> > +}
> > +
> > +static void load_short_psw(struct short_psw psw)
> > +{
> > + uint64_t scratch;
> > +
> > + fixup_psw.mask = extract_psw_mask();
> > + asm volatile ( "larl %[scratch],0f\n"
> > + " stg %[scratch],%[addr]\n"
> > + " lpsw %[psw]\n"
> > + "0: nop\n"
> > + : [scratch] "=&d"(scratch),
> > + [addr] "=&T"(fixup_psw.addr)
> > + : [psw] "Q"(psw)
> > + : "cc", "memory"
> > + );
>
> Same story.
Do you want me to repeat the comments here or just rename addr?
[...]
> > +static int not_even(void)
> > +{
> > + uint64_t quad[2] __attribute__((aligned(16))) = {0};
> > +
> > + asm volatile (".insn rxy,0xe3000000008f,%%r7,%[quad]" /* lpq %%r7,%[quad] */
> > + : : [quad] "T"(quad)
>
> Is there a reason you never put a space after the constraint?
TBH I never noticed I'm unusual in that regard. I guess I tend to think
of the operand and constraint as one entity.
I'll add the spaces.
>
> > + : "%r7", "%r8"
> > + );
> > + return 0;
> > +}
> > +
[...]
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [kvm-unit-tests PATCH v5 1/2] s390x: Add specification exception test
2022-08-26 11:23 ` Janis Schoetterl-Glausch
@ 2022-08-26 11:55 ` Janosch Frank
0 siblings, 0 replies; 7+ messages in thread
From: Janosch Frank @ 2022-08-26 11:55 UTC (permalink / raw)
To: Janis Schoetterl-Glausch, Thomas Huth, Claudio Imbrenda,
David Hildenbrand, Richard Henderson
Cc: kvm, linux-s390, qemu-s390x
On 8/26/22 13:23, Janis Schoetterl-Glausch wrote:
> On Wed, 2022-08-24 at 11:35 +0200, Janosch Frank wrote:
>> On 7/20/22 16:25, Janis Schoetterl-Glausch wrote:
>>> Generate specification exceptions and check that they occur.
>>>
>>> Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
>>> ---
>>> s390x/Makefile | 1 +
>>> lib/s390x/asm/arch_def.h | 5 ++
>>> s390x/spec_ex.c | 180 +++++++++++++++++++++++++++++++++++++++
>>> s390x/unittests.cfg | 3 +
>>> 4 files changed, 189 insertions(+)
>>> create mode 100644 s390x/spec_ex.c
>>>
>>>
>>> +
>>> +/*
>>> + * Load possibly invalid psw, but setup fixup_psw before,
>>> + * so that fixup_invalid_psw() can bring us back onto the right track.
Not sure if the second line is needed as fixup_psw is a descriptive name
already.
>>> + * Also acts as compiler barrier, -> none required in expect/check_invalid_psw
>>> + */
>>> +static void load_psw(struct psw psw)
>>> +{
>>> + uint64_t scratch;
>>> +
>
> [...]
>
>> /*
>> Store a valid mask and the address of the nop into the fixup PSW.
>> Then load the possibly invalid PSW.
>> */
>
> This seems a bit redundant given the function comment, but I can
> drop a comment in here describing how the fixup psw is computed.
Well, I skipped the function comment, got confused by the addr asm
variable and then decided to propose the comment.
It's a bit confusing since you have the invalid PSW and the global fixup
PSW in one function.
Maybe something like:
/* From here to the lpswe we're computing and setting the fixup PSW */
>
>>
>>> + fixup_psw.mask = extract_psw_mask();
>>> + asm volatile ( "larl %[scratch],0f\n"
>>> + " stg %[scratch],%[addr]\n"
>>> + " lpswe %[psw]\n"
>>> + "0: nop\n"
>>> + : [scratch] "=&d"(scratch),
>>> + [addr] "=&T"(fixup_psw.addr)
>>
>> s/addr/psw_addr/ ?
>>
>>> + : [psw] "Q"(psw)
>>> + : "cc", "memory"
>>> + );
>>> +}
>>> +
>>> +static void load_short_psw(struct short_psw psw)
>>> +{
>>> + uint64_t scratch;
>>> +
>>> + fixup_psw.mask = extract_psw_mask();
>>> + asm volatile ( "larl %[scratch],0f\n"
>>> + " stg %[scratch],%[addr]\n"
>>> + " lpsw %[psw]\n"
>>> + "0: nop\n"
>>> + : [scratch] "=&d"(scratch),
>>> + [addr] "=&T"(fixup_psw.addr)
>>> + : [psw] "Q"(psw)
>>> + : "cc", "memory"
>>> + );
>>
>> Same story.
>
> Do you want me to repeat the comments here or just rename addr?
Just rename addr
>
> [...]
>
>>> +static int not_even(void)
>>> +{
>>> + uint64_t quad[2] __attribute__((aligned(16))) = {0};
>>> +
>>> + asm volatile (".insn rxy,0xe3000000008f,%%r7,%[quad]" /* lpq %%r7,%[quad] */
>>> + : : [quad] "T"(quad)
>>
>> Is there a reason you never put a space after the constraint?
>
> TBH I never noticed I'm unusual in that regard. I guess I tend to think
> of the operand and constraint as one entity.
> I'll add the spaces.
>
>>
>>> + : "%r7", "%r8"
>>> + );
>>> + return 0;
>>> +}
>>> +
>
> [...]
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2022-08-26 11:55 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-07-20 14:25 [kvm-unit-tests PATCH v5 0/2] Add specification exception tests Janis Schoetterl-Glausch
2022-07-20 14:25 ` [kvm-unit-tests PATCH v5 1/2] s390x: Add specification exception test Janis Schoetterl-Glausch
2022-08-24 9:35 ` Janosch Frank
2022-08-25 7:38 ` Janosch Frank
2022-08-26 11:23 ` Janis Schoetterl-Glausch
2022-08-26 11:55 ` Janosch Frank
2022-07-20 14:25 ` [kvm-unit-tests PATCH v5 2/2] s390x: Test specification exceptions during transaction Janis Schoetterl-Glausch
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.