[kvm-unit-tests PATCH v5 0/6] s390x: Add support for running guests without MSO/MSL

[kvm-unit-tests PATCH v5 0/6] s390x: Add support for running guests without MSO/MSL

[Nico Boehr]

v5:
---
* fix a big oopsie in irq_set_dat_mode() which caused parts of lowcore being
  overwritten (thanks Claudio)

v4:
---
- add static assert for PSW bitfield (Janosch, Claudio)
- remove unneeded includes (Janosch)
- move variable decls to function start (Janosch)
- remove unneeded imports (Janosch)
- lowerocase hex (Janosch)
- remove unneeded attr (Janosch)
- tyop :-) fixes (Janosch)

v3:
---
* introduce bitfield for the PSW to make handling less clumsy
* some variable renames (Claudio)
* remove unneeded barriers (Claudio)
* remove rebase leftover sie_had_pgm_int (Claudio)
* move read_pgm_int_code to header (Claudio)
* squash include fix commit into the one causing the issue (Claudio)

v2:
---
* add function to change DAT/AS mode for all irq handlers (Janosch, Claudio)
* instead of a new flag in PROG0C, check the pgm int code in lowcore (Janosch)
* fix indents, comments (Nina)

Right now, all SIE tests in kvm-unit-tests (i.e. where kvm-unit-test is the
hypervisor) run using MSO/MSL.

This is convenient, because it's simple. But it also comes with
disadvantages, for example some features are unavailabe with MSO/MSL.

This series adds support for running guests without MSO/MSL with dedicated
guest page tables for the GPA->HPA translation.

Since SIE implicitly uses the primary space mode for the guest, the host
can't run in the primary space mode, too. To avoid moving all tests to the
home space mode, only switch to home space mode when it is actually needed.

This series also comes with various bugfixes that were caught while
develoing this.

Nico Boehr (6):
  lib: s390x: introduce bitfield for PSW mask
  s390x: add function to set DAT mode for all interrupts
  s390x: sie: switch to home space mode before entering SIE
  s390x: lib: don't forward PSW when handling exception in SIE
  s390x: lib: sie: don't reenter SIE on pgm int
  s390x: add a test for SIE without MSO/MSL

 lib/s390x/asm/arch_def.h   |  27 ++++++++-
 lib/s390x/asm/interrupt.h  |  18 ++++++
 lib/s390x/asm/mem.h        |   1 +
 lib/s390x/interrupt.c      |  37 ++++++++++++
 lib/s390x/mmu.c            |   5 +-
 lib/s390x/sie.c            |  22 ++++++-
 s390x/Makefile             |   2 +
 s390x/selftest.c           |  40 +++++++++++++
 s390x/sie-dat.c            | 115 +++++++++++++++++++++++++++++++++++++
 s390x/snippets/c/sie-dat.c |  58 +++++++++++++++++++
 s390x/unittests.cfg        |   3 +
 11 files changed, 324 insertions(+), 4 deletions(-)
 create mode 100644 s390x/sie-dat.c
 create mode 100644 s390x/snippets/c/sie-dat.c

-- 
2.40.1

[kvm-unit-tests PATCH v5 1/6] lib: s390x: introduce bitfield for PSW mask

[Nico Boehr]

Changing the PSW mask is currently little clumsy, since there is only the
PSW_MASK_* defines. This makes it hard to change e.g. only the address
space in the current PSW without a lot of bit fiddling.

Introduce a bitfield for the PSW mask. This makes this kind of
modifications much simpler and easier to read.

Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
---
 lib/s390x/asm/arch_def.h | 26 +++++++++++++++++++++++++-
 s390x/selftest.c         | 40 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 65 insertions(+), 1 deletion(-)

diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
index bb26e008cc68..53279572a9ee 100644
--- a/lib/s390x/asm/arch_def.h
+++ b/lib/s390x/asm/arch_def.h
@@ -37,12 +37,36 @@ struct stack_frame_int {
 };
 
 struct psw {
-	uint64_t	mask;
+	union {
+		uint64_t	mask;
+		struct {
+			uint8_t reserved00:1;
+			uint8_t per:1;
+			uint8_t reserved02:3;
+			uint8_t dat:1;
+			uint8_t io:1;
+			uint8_t ext:1;
+			uint8_t key:4;
+			uint8_t reserved12:1;
+			uint8_t mchk:1;
+			uint8_t wait:1;
+			uint8_t pstate:1;
+			uint8_t as:2;
+			uint8_t cc:2;
+			uint8_t prg_mask:4;
+			uint8_t reserved24:7;
+			uint8_t ea:1;
+			uint8_t ba:1;
+			uint32_t reserved33:31;
+		};
+	};
 	uint64_t	addr;
 };
+_Static_assert(sizeof(struct psw) == 16, "PSW size");
 
 #define PSW(m, a) ((struct psw){ .mask = (m), .addr = (uint64_t)(a) })
 
+
 struct short_psw {
 	uint32_t	mask;
 	uint32_t	addr;
diff --git a/s390x/selftest.c b/s390x/selftest.c
index 13fd36bc06f8..8d81ba312279 100644
--- a/s390x/selftest.c
+++ b/s390x/selftest.c
@@ -74,6 +74,45 @@ static void test_malloc(void)
 	report_prefix_pop();
 }
 
+static void test_psw_mask(void)
+{
+	uint64_t expected_key = 0xF;
+	struct psw test_psw = PSW(0, 0);
+
+	report_prefix_push("PSW mask");
+	test_psw.dat = 1;
+	report(test_psw.mask == PSW_MASK_DAT, "DAT matches expected=0x%016lx actual=0x%016lx", PSW_MASK_DAT, test_psw.mask);
+
+	test_psw.mask = 0;
+	test_psw.io = 1;
+	report(test_psw.mask == PSW_MASK_IO, "IO matches expected=0x%016lx actual=0x%016lx", PSW_MASK_IO, test_psw.mask);
+
+	test_psw.mask = 0;
+	test_psw.ext = 1;
+	report(test_psw.mask == PSW_MASK_EXT, "EXT matches expected=0x%016lx actual=0x%016lx", PSW_MASK_EXT, test_psw.mask);
+
+	test_psw.mask = expected_key << (63 - 11);
+	report(test_psw.key == expected_key, "PSW Key matches expected=0x%lx actual=0x%x", expected_key, test_psw.key);
+
+	test_psw.mask = 1UL << (63 - 13);
+	report(test_psw.mchk, "MCHK matches");
+
+	test_psw.mask = 0;
+	test_psw.wait = 1;
+	report(test_psw.mask == PSW_MASK_WAIT, "Wait matches expected=0x%016lx actual=0x%016lx", PSW_MASK_WAIT, test_psw.mask);
+
+	test_psw.mask = 0;
+	test_psw.pstate = 1;
+	report(test_psw.mask == PSW_MASK_PSTATE, "Pstate matches expected=0x%016lx actual=0x%016lx", PSW_MASK_PSTATE, test_psw.mask);
+
+	test_psw.mask = 0;
+	test_psw.ea = 1;
+	test_psw.ba = 1;
+	report(test_psw.mask == PSW_MASK_64, "BA/EA matches expected=0x%016lx actual=0x%016lx", PSW_MASK_64, test_psw.mask);
+
+	report_prefix_pop();
+}
+
 int main(int argc, char**argv)
 {
 	report_prefix_push("selftest");
@@ -89,6 +128,7 @@ int main(int argc, char**argv)
 	test_fp();
 	test_pgm_int();
 	test_malloc();
+	test_psw_mask();
 
 	return report_summary();
 }
-- 
2.40.1

[kvm-unit-tests PATCH v5 2/6] s390x: add function to set DAT mode for all interrupts

[Nico Boehr]

When toggling DAT or switch address space modes, it is likely that
interrupts should be handled in the same DAT or address space mode.

Add a function which toggles DAT and address space mode for all
interruptions, except restart interrupts.

Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
---
 lib/s390x/asm/interrupt.h |  4 ++++
 lib/s390x/interrupt.c     | 36 ++++++++++++++++++++++++++++++++++++
 lib/s390x/mmu.c           |  5 +++--
 3 files changed, 43 insertions(+), 2 deletions(-)

diff --git a/lib/s390x/asm/interrupt.h b/lib/s390x/asm/interrupt.h
index 35c1145f0349..55759002dce2 100644
--- a/lib/s390x/asm/interrupt.h
+++ b/lib/s390x/asm/interrupt.h
@@ -83,6 +83,10 @@ void expect_ext_int(void);
 uint16_t clear_pgm_int(void);
 void check_pgm_int_code(uint16_t code);
 
+#define IRQ_DAT_ON	true
+#define IRQ_DAT_OFF	false
+void irq_set_dat_mode(bool dat, uint64_t as);
+
 /* Activate low-address protection */
 static inline void low_prot_enable(void)
 {
diff --git a/lib/s390x/interrupt.c b/lib/s390x/interrupt.c
index 3f993a363ae2..9b1bc6ce819d 100644
--- a/lib/s390x/interrupt.c
+++ b/lib/s390x/interrupt.c
@@ -9,6 +9,7 @@
  */
 #include <libcflat.h>
 #include <asm/barrier.h>
+#include <asm/mem.h>
 #include <asm/asm-offsets.h>
 #include <sclp.h>
 #include <interrupt.h>
@@ -104,6 +105,41 @@ void register_ext_cleanup_func(void (*f)(struct stack_frame_int *))
 	THIS_CPU->ext_cleanup_func = f;
 }
 
+/**
+ * irq_set_dat_mode - Set the DAT mode of all interrupt handlers, except for
+ * restart.
+ * This will update the DAT mode and address space mode of all interrupt new
+ * PSWs.
+ *
+ * Since enabling DAT needs initalized CRs and the restart new PSW is often used
+ * to initalize CRs, the restart new PSW is never touched to avoid the chicken
+ * and egg situation.
+ *
+ * @dat specifies whether to use DAT or not
+ * @as specifies the address space mode to use - one of AS_PRIM, AS_ACCR,
+ * AS_SECN or AS_HOME.
+ */
+void irq_set_dat_mode(bool dat, uint64_t as)
+{
+	struct psw* irq_psws[] = {
+		OPAQUE_PTR(GEN_LC_EXT_NEW_PSW),
+		OPAQUE_PTR(GEN_LC_SVC_NEW_PSW),
+		OPAQUE_PTR(GEN_LC_PGM_NEW_PSW),
+		OPAQUE_PTR(GEN_LC_MCCK_NEW_PSW),
+		OPAQUE_PTR(GEN_LC_IO_NEW_PSW),
+	};
+	struct psw *psw;
+
+	assert(as == AS_PRIM || as == AS_ACCR || as == AS_SECN || as == AS_HOME);
+
+	for (size_t i = 0; i < ARRAY_SIZE(irq_psws); i++) {
+		psw = irq_psws[i];
+		psw->dat = dat;
+		if (dat)
+			psw->as = as;
+	}
+}
+
 static void fixup_pgm_int(struct stack_frame_int *stack)
 {
 	/* If we have an error on SIE we directly move to sie_exit */
diff --git a/lib/s390x/mmu.c b/lib/s390x/mmu.c
index b474d7021d3f..199bd3fbc9c8 100644
--- a/lib/s390x/mmu.c
+++ b/lib/s390x/mmu.c
@@ -12,6 +12,7 @@
 #include <asm/pgtable.h>
 #include <asm/arch_def.h>
 #include <asm/barrier.h>
+#include <asm/interrupt.h>
 #include <vmalloc.h>
 #include "mmu.h"
 
@@ -41,8 +42,8 @@ static void mmu_enable(pgd_t *pgtable)
 	/* enable dat (primary == 0 set as default) */
 	enable_dat();
 
-	/* we can now also use DAT unconditionally in our PGM handler */
-	lowcore.pgm_new_psw.mask |= PSW_MASK_DAT;
+	/* we can now also use DAT in all interrupt handlers */
+	irq_set_dat_mode(IRQ_DAT_ON, AS_PRIM);
 }
 
 /*
-- 
2.40.1

[kvm-unit-tests PATCH v5 3/6] s390x: sie: switch to home space mode before entering SIE

[Nico Boehr]

This is to prepare for running guests without MSO/MSL, which is
currently not possible.

We already have code in sie64a to setup a guest primary ASCE before
entering SIE, so we can in theory switch to the page tables which
translate gpa to hpa.

But the host is running in primary space mode already, so changing the
primary ASCE before entering SIE will also affect the host's code and
data.

To make this switch useful, the host should run in a different address
space mode. Hence, set up and change to home address space mode before
installing the guest ASCE.

The home space ASCE is just copied over from the primary space ASCE, so
no functional change is intended, also for tests that want to use
MSO/MSL. If a test intends to use a different primary space ASCE, it can
now just set the guest.asce in the save_area.

Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
---
 lib/s390x/asm/arch_def.h |  1 +
 lib/s390x/sie.c          | 18 ++++++++++++++++++
 2 files changed, 19 insertions(+)

diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
index 53279572a9ee..65e1cf58c7e7 100644
--- a/lib/s390x/asm/arch_def.h
+++ b/lib/s390x/asm/arch_def.h
@@ -91,6 +91,7 @@ struct cpu {
 #define AS_HOME				3
 
 #define PSW_MASK_DAT			0x0400000000000000UL
+#define PSW_MASK_HOME			0x0000C00000000000UL
 #define PSW_MASK_IO			0x0200000000000000UL
 #define PSW_MASK_EXT			0x0100000000000000UL
 #define PSW_MASK_KEY			0x00F0000000000000UL
diff --git a/lib/s390x/sie.c b/lib/s390x/sie.c
index 9241b4b4a512..ffa8ec91a423 100644
--- a/lib/s390x/sie.c
+++ b/lib/s390x/sie.c
@@ -46,6 +46,8 @@ void sie_handle_validity(struct vm *vm)
 
 void sie(struct vm *vm)
 {
+	uint64_t old_cr13;
+
 	if (vm->sblk->sdf == 2)
 		memcpy(vm->sblk->pv_grregs, vm->save_area.guest.grs,
 		       sizeof(vm->save_area.guest.grs));
@@ -53,6 +55,16 @@ void sie(struct vm *vm)
 	/* Reset icptcode so we don't trip over it below */
 	vm->sblk->icptcode = 0;
 
+	/* set up home address space to match primary space */
+	old_cr13 = stctg(13);
+	lctlg(13, stctg(1));
+
+	/* switch to home space so guest tables can be different from host */
+	psw_mask_set_bits(PSW_MASK_HOME);
+
+	/* also handle all interruptions in home space while in SIE */
+	irq_set_dat_mode(IRQ_DAT_ON, AS_HOME);
+
 	while (vm->sblk->icptcode == 0) {
 		sie64a(vm->sblk, &vm->save_area);
 		sie_handle_validity(vm);
@@ -60,6 +72,12 @@ void sie(struct vm *vm)
 	vm->save_area.guest.grs[14] = vm->sblk->gg14;
 	vm->save_area.guest.grs[15] = vm->sblk->gg15;
 
+	irq_set_dat_mode(IRQ_DAT_ON, AS_PRIM);
+	psw_mask_clear_bits(PSW_MASK_HOME);
+
+	/* restore the old CR 13 */
+	lctlg(13, old_cr13);
+
 	if (vm->sblk->sdf == 2)
 		memcpy(vm->save_area.guest.grs, vm->sblk->pv_grregs,
 		       sizeof(vm->save_area.guest.grs));
-- 
2.40.1

[kvm-unit-tests PATCH v5 4/6] s390x: lib: don't forward PSW when handling exception in SIE

[Nico Boehr]

When we're handling a pgm int in SIE, we want to return to the SIE
cleanup after handling the exception. That's why we set pgm_old_psw to
the sie_exit label in fixup_pgm_int.

On nullifing pgm ints, fixup_pgm_int will also forward the old PSW such
that we don't cause an pgm int again.

However, when we want to return to the sie_exit label, this is not
needed (since we've manually set pgm_old_psw). Instead, forwarding the
PSW might cause us to skip an instruction or end up in the middle of an
instruction.

So, let's just skip the rest of the fixup in case we're inside SIE.

Note that we're intentionally not fixing up the PSW in the guest; that's
best left to the test at hand by registering their own psw fixup.

Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
---
 lib/s390x/interrupt.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lib/s390x/interrupt.c b/lib/s390x/interrupt.c
index 9b1bc6ce819d..5653220d166e 100644
--- a/lib/s390x/interrupt.c
+++ b/lib/s390x/interrupt.c
@@ -146,6 +146,7 @@ static void fixup_pgm_int(struct stack_frame_int *stack)
 	if (lowcore.pgm_old_psw.addr >= (uint64_t)&sie_entry &&
 	    lowcore.pgm_old_psw.addr <= (uint64_t)&sie_exit) {
 		lowcore.pgm_old_psw.addr = (uint64_t)&sie_exit;
+		return;
 	}
 
 	switch (lowcore.pgm_int_code) {
-- 
2.40.1

[kvm-unit-tests PATCH v5 5/6] s390x: lib: sie: don't reenter SIE on pgm int

[Nico Boehr]

At the moment, when a PGM int occurs while in SIE, we will just reenter
SIE after the interrupt handler was called.

This is because sie() has a loop which checks icptcode and re-enters SIE
if it is zero.

However, this behaviour is quite undesirable for SIE tests, since it
doesn't give the host the chance to assert on the PGM int. Instead, we
will just re-enter SIE, on nullifing conditions even causing the
exception again.

In sie(), check whether a pgm int code is set in lowcore. If it has,
exit the loop so the test can react to the interrupt. Add a new function
read_pgm_int_code() to obtain the interrupt code.

Note that this introduces a slight oddity with sie and pgm int in
certain cases: If a PGM int occurs between a expect_pgm_int() and sie(),
we will now never enter SIE until the pgm_int_code is cleared by e.g.
clear_pgm_int().

Also add missing include of facility.h to mem.h.

Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
---
 lib/s390x/asm/interrupt.h | 14 ++++++++++++++
 lib/s390x/asm/mem.h       |  1 +
 lib/s390x/sie.c           |  4 +++-
 3 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/lib/s390x/asm/interrupt.h b/lib/s390x/asm/interrupt.h
index 55759002dce2..9e509d2f4f1e 100644
--- a/lib/s390x/asm/interrupt.h
+++ b/lib/s390x/asm/interrupt.h
@@ -99,4 +99,18 @@ static inline void low_prot_disable(void)
 	ctl_clear_bit(0, CTL0_LOW_ADDR_PROT);
 }
 
+/**
+ * read_pgm_int_code - Get the program interruption code of the last pgm int
+ * on the current CPU.
+ *
+ * This is similar to clear_pgm_int(), except that it doesn't clear the
+ * interruption information from lowcore.
+ *
+ * Returns 0 when none occurred.
+ */
+static inline uint16_t read_pgm_int_code(void)
+{
+	return lowcore.pgm_int_code;
+}
+
 #endif
diff --git a/lib/s390x/asm/mem.h b/lib/s390x/asm/mem.h
index 64ef59b546a4..94d58c34f53f 100644
--- a/lib/s390x/asm/mem.h
+++ b/lib/s390x/asm/mem.h
@@ -8,6 +8,7 @@
 #ifndef _ASMS390X_MEM_H_
 #define _ASMS390X_MEM_H_
 #include <asm/arch_def.h>
+#include <asm/facility.h>
 
 /* create pointer while avoiding compiler warnings */
 #define OPAQUE_PTR(x) ((void *)(((uint64_t)&lowcore) + (x)))
diff --git a/lib/s390x/sie.c b/lib/s390x/sie.c
index ffa8ec91a423..632740edd431 100644
--- a/lib/s390x/sie.c
+++ b/lib/s390x/sie.c
@@ -13,6 +13,7 @@
 #include <libcflat.h>
 #include <sie.h>
 #include <asm/page.h>
+#include <asm/interrupt.h>
 #include <libcflat.h>
 #include <alloc_page.h>
 
@@ -65,7 +66,8 @@ void sie(struct vm *vm)
 	/* also handle all interruptions in home space while in SIE */
 	irq_set_dat_mode(IRQ_DAT_ON, AS_HOME);
 
-	while (vm->sblk->icptcode == 0) {
+	/* leave SIE when we have an intercept or an interrupt so the test can react to it */
+	while (vm->sblk->icptcode == 0 && !read_pgm_int_code()) {
 		sie64a(vm->sblk, &vm->save_area);
 		sie_handle_validity(vm);
 	}
-- 
2.40.1

[kvm-unit-tests PATCH v5 6/6] s390x: add a test for SIE without MSO/MSL

[Nico Boehr]

Since we now have the ability to run guests without MSO/MSL, add a test
to make sure this doesn't break.

Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
---
 s390x/Makefile             |   2 +
 s390x/sie-dat.c            | 115 +++++++++++++++++++++++++++++++++++++
 s390x/snippets/c/sie-dat.c |  58 +++++++++++++++++++
 s390x/unittests.cfg        |   3 +
 4 files changed, 178 insertions(+)
 create mode 100644 s390x/sie-dat.c
 create mode 100644 s390x/snippets/c/sie-dat.c

diff --git a/s390x/Makefile b/s390x/Makefile
index a80db538810e..4921669ee4c3 100644
--- a/s390x/Makefile
+++ b/s390x/Makefile
@@ -40,6 +40,7 @@ tests += $(TEST_DIR)/panic-loop-pgm.elf
 tests += $(TEST_DIR)/migration-sck.elf
 tests += $(TEST_DIR)/exittime.elf
 tests += $(TEST_DIR)/ex.elf
+tests += $(TEST_DIR)/sie-dat.elf
 
 pv-tests += $(TEST_DIR)/pv-diags.elf
 
@@ -120,6 +121,7 @@ snippet_lib = $(snippet_asmlib) lib/auxinfo.o
 # perquisites (=guests) for the snippet hosts.
 # $(TEST_DIR)/<snippet-host>.elf: snippets = $(SNIPPET_DIR)/<c/asm>/<snippet>.gbin
 $(TEST_DIR)/mvpg-sie.elf: snippets = $(SNIPPET_DIR)/c/mvpg-snippet.gbin
+$(TEST_DIR)/sie-dat.elf: snippets = $(SNIPPET_DIR)/c/sie-dat.gbin
 $(TEST_DIR)/spec_ex-sie.elf: snippets = $(SNIPPET_DIR)/c/spec_ex.gbin
 
 $(TEST_DIR)/pv-diags.elf: pv-snippets += $(SNIPPET_DIR)/asm/snippet-pv-diag-yield.gbin
diff --git a/s390x/sie-dat.c b/s390x/sie-dat.c
new file mode 100644
index 000000000000..b326995dfa85
--- /dev/null
+++ b/s390x/sie-dat.c
@@ -0,0 +1,115 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Tests SIE with paging.
+ *
+ * Copyright 2023 IBM Corp.
+ *
+ * Authors:
+ *    Nico Boehr <nrb@linux.ibm.com>
+ */
+#include <libcflat.h>
+#include <vmalloc.h>
+#include <asm/pgtable.h>
+#include <mmu.h>
+#include <asm/page.h>
+#include <asm/interrupt.h>
+#include <alloc_page.h>
+#include <sclp.h>
+#include <sie.h>
+#include <snippet.h>
+
+static struct vm vm;
+static pgd_t *guest_root;
+
+/* keep in sync with TEST_PAGE_COUNT in s390x/snippets/c/sie-dat.c */
+#define GUEST_TEST_PAGE_COUNT 10
+
+/* keep in sync with TOTAL_PAGE_COUNT in s390x/snippets/c/sie-dat.c */
+#define GUEST_TOTAL_PAGE_COUNT 256
+
+static void test_sie_dat(void)
+{
+	uint64_t test_page_gpa, test_page_hpa;
+	uint8_t *test_page_hva, expected_val;
+	bool contents_match;
+	uint8_t r1;
+
+	/* guest will tell us the guest physical address of the test buffer */
+	sie(&vm);
+
+	r1 = (vm.sblk->ipa & 0xf0) >> 4;
+	test_page_gpa = vm.save_area.guest.grs[r1];
+	test_page_hpa = virt_to_pte_phys(guest_root, (void*)test_page_gpa);
+	test_page_hva = __va(test_page_hpa);
+	assert(vm.sblk->icptcode == ICPT_INST &&
+	       (vm.sblk->ipa & 0xff00) == 0x8300 && vm.sblk->ipb == 0x9c0000);
+	report_info("test buffer gpa=0x%lx hva=%p", test_page_gpa, test_page_hva);
+
+	/* guest will now write to the test buffer and we verify the contents */
+	sie(&vm);
+	assert(vm.sblk->icptcode == ICPT_INST &&
+	       vm.sblk->ipa == 0x8300 && vm.sblk->ipb == 0x440000);
+
+	contents_match = true;
+	for (unsigned int i = 0; i < GUEST_TEST_PAGE_COUNT; i++) {
+		expected_val = 42 + i;
+		if (test_page_hva[i * PAGE_SIZE] != expected_val) {
+			report_fail("page %u mismatch actual_val=%x expected_val=%x",
+				    i, test_page_hva[i], expected_val);
+			contents_match = false;
+		}
+	}
+	report(contents_match, "test buffer contents match");
+
+	/* the guest will now write to an unmapped address and we check that this causes a segment translation exception */
+	report_prefix_push("guest write to unmapped");
+	expect_pgm_int();
+	sie(&vm);
+	check_pgm_int_code(PGM_INT_CODE_SEGMENT_TRANSLATION);
+	report_prefix_pop();
+}
+
+static void setup_guest(void)
+{
+	extern const char SNIPPET_NAME_START(c, sie_dat)[];
+	extern const char SNIPPET_NAME_END(c, sie_dat)[];
+	uint64_t guest_max_addr;
+
+	setup_vm();
+	snippet_setup_guest(&vm, false);
+
+	/* allocate a region-1 table */
+	guest_root = pgd_alloc_one();
+
+	/* map guest memory 1:1 */
+	guest_max_addr = GUEST_TOTAL_PAGE_COUNT * PAGE_SIZE;
+	for (uint64_t i = 0; i < guest_max_addr; i += PAGE_SIZE)
+		install_page(guest_root, __pa(vm.guest_mem + i), (void *)i);
+
+	/* set up storage limit supression - leave mso and msl intact they are ignored anyways */
+	vm.sblk->cpuflags |= CPUSTAT_SM;
+
+	/* set up the guest asce */
+	vm.save_area.guest.asce = __pa(guest_root) | ASCE_DT_REGION1 | REGION_TABLE_LENGTH;
+
+	snippet_init(&vm, SNIPPET_NAME_START(c, sie_dat),
+		     SNIPPET_LEN(c, sie_dat), SNIPPET_UNPACK_OFF);
+}
+
+int main(void)
+{
+	report_prefix_push("sie-dat");
+	if (!sclp_facilities.has_sief2) {
+		report_skip("SIEF2 facility unavailable");
+		goto done;
+	}
+
+	setup_guest();
+	test_sie_dat();
+	sie_guest_destroy(&vm);
+
+done:
+	report_prefix_pop();
+	return report_summary();
+
+}
diff --git a/s390x/snippets/c/sie-dat.c b/s390x/snippets/c/sie-dat.c
new file mode 100644
index 000000000000..0505e5aba62b
--- /dev/null
+++ b/s390x/snippets/c/sie-dat.c
@@ -0,0 +1,58 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Snippet used by the sie-dat.c test to verify paging without MSO/MSL
+ *
+ * Copyright (c) 2023 IBM Corp
+ *
+ * Authors:
+ *  Nico Boehr <nrb@linux.ibm.com>
+ */
+#include <stddef.h>
+#include <inttypes.h>
+#include <string.h>
+#include <asm-generic/page.h>
+
+/* keep in sync with GUEST_TEST_PAGE_COUNT in s390x/sie-dat.c */
+#define TEST_PAGE_COUNT 10
+static uint8_t test_page[TEST_PAGE_COUNT * PAGE_SIZE] __attribute__((__aligned__(PAGE_SIZE)));
+
+/* keep in sync with GUEST_TOTAL_PAGE_COUNT in s390x/sie-dat.c */
+#define TOTAL_PAGE_COUNT 256
+
+static inline void force_exit(void)
+{
+	asm volatile("diag	0,0,0x44\n");
+}
+
+static inline void force_exit_value(uint64_t val)
+{
+	asm volatile(
+		"diag	%[val],0,0x9c\n"
+		: : [val] "d"(val)
+	);
+}
+
+int main(void)
+{
+	uint8_t *invalid_ptr;
+
+	memset(test_page, 0, sizeof(test_page));
+	/* tell the host the page's physical address (we're running DAT off) */
+	force_exit_value((uint64_t)test_page);
+
+	/* write some value to the page so the host can verify it */
+	for (size_t i = 0; i < TEST_PAGE_COUNT; i++)
+		test_page[i * PAGE_SIZE] = 42 + i;
+
+	/* indicate we've written all pages */
+	force_exit();
+
+	/* the first unmapped address */
+	invalid_ptr = (uint8_t *)(TOTAL_PAGE_COUNT * PAGE_SIZE);
+	*invalid_ptr = 42;
+
+	/* indicate we've written the non-allowed page (should never get here) */
+	force_exit();
+
+	return 0;
+}
diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg
index b61faf0737c3..24cd27202a08 100644
--- a/s390x/unittests.cfg
+++ b/s390x/unittests.cfg
@@ -218,3 +218,6 @@ extra_params = -append '--parallel'
 
 [execute]
 file = ex.elf
+
+[sie-dat]
+file = sie-dat.elf
-- 
2.40.1

Re: [kvm-unit-tests PATCH v5 1/6] lib: s390x: introduce bitfield for PSW mask

[Thomas Huth]

On 12/07/2023 13.41, Nico Boehr wrote:
> Changing the PSW mask is currently little clumsy, since there is only the
> PSW_MASK_* defines. This makes it hard to change e.g. only the address
> space in the current PSW without a lot of bit fiddling.
> 
> Introduce a bitfield for the PSW mask. This makes this kind of
> modifications much simpler and easier to read.
> 
> Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
> ---
>   lib/s390x/asm/arch_def.h | 26 +++++++++++++++++++++++++-
>   s390x/selftest.c         | 40 ++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 65 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
> index bb26e008cc68..53279572a9ee 100644
> --- a/lib/s390x/asm/arch_def.h
> +++ b/lib/s390x/asm/arch_def.h
> @@ -37,12 +37,36 @@ struct stack_frame_int {
>   };
>   
>   struct psw {
> -	uint64_t	mask;
> +	union {
> +		uint64_t	mask;
> +		struct {
> +			uint8_t reserved00:1;
> +			uint8_t per:1;
> +			uint8_t reserved02:3;
> +			uint8_t dat:1;
> +			uint8_t io:1;
> +			uint8_t ext:1;
> +			uint8_t key:4;
> +			uint8_t reserved12:1;
> +			uint8_t mchk:1;
> +			uint8_t wait:1;
> +			uint8_t pstate:1;
> +			uint8_t as:2;
> +			uint8_t cc:2;
> +			uint8_t prg_mask:4;
> +			uint8_t reserved24:7;
> +			uint8_t ea:1;
> +			uint8_t ba:1;
> +			uint32_t reserved33:31;
> +		};
> +	};
>   	uint64_t	addr;
>   };
> +_Static_assert(sizeof(struct psw) == 16, "PSW size");
>   
>   #define PSW(m, a) ((struct psw){ .mask = (m), .addr = (uint64_t)(a) })
>   
> +
>   struct short_psw {
>   	uint32_t	mask;
>   	uint32_t	addr;
> diff --git a/s390x/selftest.c b/s390x/selftest.c
> index 13fd36bc06f8..8d81ba312279 100644
> --- a/s390x/selftest.c
> +++ b/s390x/selftest.c
> @@ -74,6 +74,45 @@ static void test_malloc(void)
>   	report_prefix_pop();
>   }
>   
> +static void test_psw_mask(void)
> +{
> +	uint64_t expected_key = 0xF;
> +	struct psw test_psw = PSW(0, 0);
> +
> +	report_prefix_push("PSW mask");
> +	test_psw.dat = 1;
> +	report(test_psw.mask == PSW_MASK_DAT, "DAT matches expected=0x%016lx actual=0x%016lx", PSW_MASK_DAT, test_psw.mask);
> +
> +	test_psw.mask = 0;
> +	test_psw.io = 1;
> +	report(test_psw.mask == PSW_MASK_IO, "IO matches expected=0x%016lx actual=0x%016lx", PSW_MASK_IO, test_psw.mask);
> +
> +	test_psw.mask = 0;
> +	test_psw.ext = 1;
> +	report(test_psw.mask == PSW_MASK_EXT, "EXT matches expected=0x%016lx actual=0x%016lx", PSW_MASK_EXT, test_psw.mask);
> +
> +	test_psw.mask = expected_key << (63 - 11);
> +	report(test_psw.key == expected_key, "PSW Key matches expected=0x%lx actual=0x%x", expected_key, test_psw.key);

Patch looks basically fine to me, but here my mind stumbled a little bit. 
This test is written the other way round than the others. Nothing wrong with 
that, it just feels a little bit inconsistent. I'd suggest to either do:

	test_psw.mask = 0;
	test_psw.key = expected_key;
	report(test_psw.mask == expected_key << (63 - 11), ...);

or maybe even switch all the other tests around instead, so you could get 
rid of the "test_psw.mask = 0" lines, e.g. :

	test_psw.mask == PSW_MASK_IO;
	report(test_psw.io, "IO matches ...");

etc.

  Thomas

Re: [kvm-unit-tests PATCH v5 1/6] lib: s390x: introduce bitfield for PSW mask

[Thomas Huth]

On 13/07/2023 08.56, Thomas Huth wrote:
> On 12/07/2023 13.41, Nico Boehr wrote:
>> Changing the PSW mask is currently little clumsy, since there is only the
>> PSW_MASK_* defines. This makes it hard to change e.g. only the address
>> space in the current PSW without a lot of bit fiddling.
>>
>> Introduce a bitfield for the PSW mask. This makes this kind of
>> modifications much simpler and easier to read.
>>
>> Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
>> ---
>>   lib/s390x/asm/arch_def.h | 26 +++++++++++++++++++++++++-
>>   s390x/selftest.c         | 40 ++++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 65 insertions(+), 1 deletion(-)
>>
>> diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
>> index bb26e008cc68..53279572a9ee 100644
>> --- a/lib/s390x/asm/arch_def.h
>> +++ b/lib/s390x/asm/arch_def.h
>> @@ -37,12 +37,36 @@ struct stack_frame_int {
>>   };
>>   struct psw {
>> -    uint64_t    mask;
>> +    union {
>> +        uint64_t    mask;
>> +        struct {
>> +            uint8_t reserved00:1;
>> +            uint8_t per:1;
>> +            uint8_t reserved02:3;
>> +            uint8_t dat:1;
>> +            uint8_t io:1;
>> +            uint8_t ext:1;
>> +            uint8_t key:4;
>> +            uint8_t reserved12:1;
>> +            uint8_t mchk:1;
>> +            uint8_t wait:1;
>> +            uint8_t pstate:1;
>> +            uint8_t as:2;
>> +            uint8_t cc:2;
>> +            uint8_t prg_mask:4;
>> +            uint8_t reserved24:7;
>> +            uint8_t ea:1;
>> +            uint8_t ba:1;
>> +            uint32_t reserved33:31;
>> +        };
>> +    };
>>       uint64_t    addr;
>>   };
>> +_Static_assert(sizeof(struct psw) == 16, "PSW size");
>>   #define PSW(m, a) ((struct psw){ .mask = (m), .addr = (uint64_t)(a) })
>> +
>>   struct short_psw {
>>       uint32_t    mask;
>>       uint32_t    addr;
>> diff --git a/s390x/selftest.c b/s390x/selftest.c
>> index 13fd36bc06f8..8d81ba312279 100644
>> --- a/s390x/selftest.c
>> +++ b/s390x/selftest.c
>> @@ -74,6 +74,45 @@ static void test_malloc(void)
>>       report_prefix_pop();
>>   }
>> +static void test_psw_mask(void)
>> +{
>> +    uint64_t expected_key = 0xF;
>> +    struct psw test_psw = PSW(0, 0);
>> +
>> +    report_prefix_push("PSW mask");
>> +    test_psw.dat = 1;
>> +    report(test_psw.mask == PSW_MASK_DAT, "DAT matches expected=0x%016lx 
>> actual=0x%016lx", PSW_MASK_DAT, test_psw.mask);
>> +
>> +    test_psw.mask = 0;
>> +    test_psw.io = 1;
>> +    report(test_psw.mask == PSW_MASK_IO, "IO matches expected=0x%016lx 
>> actual=0x%016lx", PSW_MASK_IO, test_psw.mask);
>> +
>> +    test_psw.mask = 0;
>> +    test_psw.ext = 1;
>> +    report(test_psw.mask == PSW_MASK_EXT, "EXT matches expected=0x%016lx 
>> actual=0x%016lx", PSW_MASK_EXT, test_psw.mask);
>> +
>> +    test_psw.mask = expected_key << (63 - 11);
>> +    report(test_psw.key == expected_key, "PSW Key matches expected=0x%lx 
>> actual=0x%x", expected_key, test_psw.key);
> 
> Patch looks basically fine to me, but here my mind stumbled a little bit. 
> This test is written the other way round than the others. Nothing wrong with 
> that, it just feels a little bit inconsistent. I'd suggest to either do:
> 
>      test_psw.mask = 0;
>      test_psw.key = expected_key;
>      report(test_psw.mask == expected_key << (63 - 11), ...);
> 
> or maybe even switch all the other tests around instead, so you could get 
> rid of the "test_psw.mask = 0" lines, e.g. :
> 
>      test_psw.mask == PSW_MASK_IO;

s/==/=/ of course!

Sorry for that typo.

  Thomas


>      report(test_psw.io, "IO matches ...");
> 
> etc.
> 
>   Thomas
> 

Re: [kvm-unit-tests PATCH v5 2/6] s390x: add function to set DAT mode for all interrupts

[Thomas Huth]

On 12/07/2023 13.41, Nico Boehr wrote:
> When toggling DAT or switch address space modes, it is likely that
> interrupts should be handled in the same DAT or address space mode.
> 
> Add a function which toggles DAT and address space mode for all
> interruptions, except restart interrupts.
> 
> Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
> ---
>   lib/s390x/asm/interrupt.h |  4 ++++
>   lib/s390x/interrupt.c     | 36 ++++++++++++++++++++++++++++++++++++
>   lib/s390x/mmu.c           |  5 +++--
>   3 files changed, 43 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/s390x/asm/interrupt.h b/lib/s390x/asm/interrupt.h
> index 35c1145f0349..55759002dce2 100644
> --- a/lib/s390x/asm/interrupt.h
> +++ b/lib/s390x/asm/interrupt.h
> @@ -83,6 +83,10 @@ void expect_ext_int(void);
>   uint16_t clear_pgm_int(void);
>   void check_pgm_int_code(uint16_t code);
>   
> +#define IRQ_DAT_ON	true
> +#define IRQ_DAT_OFF	false

Just a matter of taste, but IMHO having defines like this for just using 
them as boolean parameter to one function is a little bit overkill already. 
I'd rather rename the "bool dat" below into "bool use_dat" and then use 
"true" and "false" directly as a parameter for that function instead. 
Anyway, just my 0.02 €.

> +void irq_set_dat_mode(bool dat, uint64_t as);
> +
>   /* Activate low-address protection */
>   static inline void low_prot_enable(void)
>   {
> diff --git a/lib/s390x/interrupt.c b/lib/s390x/interrupt.c
> index 3f993a363ae2..9b1bc6ce819d 100644
> --- a/lib/s390x/interrupt.c
> +++ b/lib/s390x/interrupt.c
> @@ -9,6 +9,7 @@
>    */
>   #include <libcflat.h>
>   #include <asm/barrier.h>
> +#include <asm/mem.h>
>   #include <asm/asm-offsets.h>
>   #include <sclp.h>
>   #include <interrupt.h>
> @@ -104,6 +105,41 @@ void register_ext_cleanup_func(void (*f)(struct stack_frame_int *))
>   	THIS_CPU->ext_cleanup_func = f;
>   }
>   
> +/**
> + * irq_set_dat_mode - Set the DAT mode of all interrupt handlers, except for
> + * restart.
> + * This will update the DAT mode and address space mode of all interrupt new
> + * PSWs.
> + *
> + * Since enabling DAT needs initalized CRs and the restart new PSW is often used

s/initalized/initialized/

> + * to initalize CRs, the restart new PSW is never touched to avoid the chicken

dito

> + * and egg situation.
> + *
> + * @dat specifies whether to use DAT or not
> + * @as specifies the address space mode to use - one of AS_PRIM, AS_ACCR,
> + * AS_SECN or AS_HOME.
> + */
> +void irq_set_dat_mode(bool dat, uint64_t as)

why uint64_t for "as" ? "int" should be enough?

(alternatively, you could turn the AS_* defines into a properly named enum 
and use that type here instead)

  Thomas

> +{
> +	struct psw* irq_psws[] = {
> +		OPAQUE_PTR(GEN_LC_EXT_NEW_PSW),
> +		OPAQUE_PTR(GEN_LC_SVC_NEW_PSW),
> +		OPAQUE_PTR(GEN_LC_PGM_NEW_PSW),
> +		OPAQUE_PTR(GEN_LC_MCCK_NEW_PSW),
> +		OPAQUE_PTR(GEN_LC_IO_NEW_PSW),
> +	};
> +	struct psw *psw;
> +
> +	assert(as == AS_PRIM || as == AS_ACCR || as == AS_SECN || as == AS_HOME);
> +
> +	for (size_t i = 0; i < ARRAY_SIZE(irq_psws); i++) {
> +		psw = irq_psws[i];
> +		psw->dat = dat;
> +		if (dat)
> +			psw->as = as;
> +	}
> +}
> +
>   static void fixup_pgm_int(struct stack_frame_int *stack)
>   {
>   	/* If we have an error on SIE we directly move to sie_exit */
> diff --git a/lib/s390x/mmu.c b/lib/s390x/mmu.c
> index b474d7021d3f..199bd3fbc9c8 100644
> --- a/lib/s390x/mmu.c
> +++ b/lib/s390x/mmu.c
> @@ -12,6 +12,7 @@
>   #include <asm/pgtable.h>
>   #include <asm/arch_def.h>
>   #include <asm/barrier.h>
> +#include <asm/interrupt.h>
>   #include <vmalloc.h>
>   #include "mmu.h"
>   
> @@ -41,8 +42,8 @@ static void mmu_enable(pgd_t *pgtable)
>   	/* enable dat (primary == 0 set as default) */
>   	enable_dat();
>   
> -	/* we can now also use DAT unconditionally in our PGM handler */
> -	lowcore.pgm_new_psw.mask |= PSW_MASK_DAT;
> +	/* we can now also use DAT in all interrupt handlers */
> +	irq_set_dat_mode(IRQ_DAT_ON, AS_PRIM);
>   }
>   
>   /*

Re: [kvm-unit-tests PATCH v5 3/6] s390x: sie: switch to home space mode before entering SIE

[Thomas Huth]

On 12/07/2023 13.41, Nico Boehr wrote:
> This is to prepare for running guests without MSO/MSL, which is
> currently not possible.
> 
> We already have code in sie64a to setup a guest primary ASCE before
> entering SIE, so we can in theory switch to the page tables which
> translate gpa to hpa.
> 
> But the host is running in primary space mode already, so changing the
> primary ASCE before entering SIE will also affect the host's code and
> data.
> 
> To make this switch useful, the host should run in a different address
> space mode. Hence, set up and change to home address space mode before
> installing the guest ASCE.
> 
> The home space ASCE is just copied over from the primary space ASCE, so
> no functional change is intended, also for tests that want to use
> MSO/MSL. If a test intends to use a different primary space ASCE, it can
> now just set the guest.asce in the save_area.
> 
> Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
> ---
>   lib/s390x/asm/arch_def.h |  1 +
>   lib/s390x/sie.c          | 18 ++++++++++++++++++
>   2 files changed, 19 insertions(+)
> 
> diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
> index 53279572a9ee..65e1cf58c7e7 100644
> --- a/lib/s390x/asm/arch_def.h
> +++ b/lib/s390x/asm/arch_def.h
> @@ -91,6 +91,7 @@ struct cpu {
>   #define AS_HOME				3
>   
>   #define PSW_MASK_DAT			0x0400000000000000UL
> +#define PSW_MASK_HOME			0x0000C00000000000UL
>   #define PSW_MASK_IO			0x0200000000000000UL
>   #define PSW_MASK_EXT			0x0100000000000000UL
>   #define PSW_MASK_KEY			0x00F0000000000000UL
> diff --git a/lib/s390x/sie.c b/lib/s390x/sie.c
> index 9241b4b4a512..ffa8ec91a423 100644
> --- a/lib/s390x/sie.c
> +++ b/lib/s390x/sie.c
> @@ -46,6 +46,8 @@ void sie_handle_validity(struct vm *vm)
>   
>   void sie(struct vm *vm)
>   {
> +	uint64_t old_cr13;
> +
>   	if (vm->sblk->sdf == 2)
>   		memcpy(vm->sblk->pv_grregs, vm->save_area.guest.grs,
>   		       sizeof(vm->save_area.guest.grs));
> @@ -53,6 +55,16 @@ void sie(struct vm *vm)
>   	/* Reset icptcode so we don't trip over it below */
>   	vm->sblk->icptcode = 0;
>   
> +	/* set up home address space to match primary space */
> +	old_cr13 = stctg(13);
> +	lctlg(13, stctg(1));
> +
> +	/* switch to home space so guest tables can be different from host */
> +	psw_mask_set_bits(PSW_MASK_HOME);
> +
> +	/* also handle all interruptions in home space while in SIE */
> +	irq_set_dat_mode(IRQ_DAT_ON, AS_HOME);
> +
>   	while (vm->sblk->icptcode == 0) {
>   		sie64a(vm->sblk, &vm->save_area);
>   		sie_handle_validity(vm);
> @@ -60,6 +72,12 @@ void sie(struct vm *vm)
>   	vm->save_area.guest.grs[14] = vm->sblk->gg14;
>   	vm->save_area.guest.grs[15] = vm->sblk->gg15;
>   
> +	irq_set_dat_mode(IRQ_DAT_ON, AS_PRIM);
> +	psw_mask_clear_bits(PSW_MASK_HOME);
> +
> +	/* restore the old CR 13 */
> +	lctlg(13, old_cr13);

Wouldn't it be better to always switch to HOME address mode directly in our 
startup code already (where we enable DAT)? Switching back and forth every 
time we enter SIE looks confusing to me ... or is there a reason why we 
should continue to run in primary address mode by default and only switch to 
home mode here?

  Thomas

Re: [kvm-unit-tests PATCH v5 5/6] s390x: lib: sie: don't reenter SIE on pgm int

[Thomas Huth]

On 12/07/2023 13.41, Nico Boehr wrote:
> At the moment, when a PGM int occurs while in SIE, we will just reenter
> SIE after the interrupt handler was called.
> 
> This is because sie() has a loop which checks icptcode and re-enters SIE
> if it is zero.
> 
> However, this behaviour is quite undesirable for SIE tests, since it
> doesn't give the host the chance to assert on the PGM int. Instead, we
> will just re-enter SIE, on nullifing conditions even causing the
> exception again.
> 
> In sie(), check whether a pgm int code is set in lowcore. If it has,
> exit the loop so the test can react to the interrupt. Add a new function
> read_pgm_int_code() to obtain the interrupt code.
> 
> Note that this introduces a slight oddity with sie and pgm int in
> certain cases: If a PGM int occurs between a expect_pgm_int() and sie(),
> we will now never enter SIE until the pgm_int_code is cleared by e.g.
> clear_pgm_int().
> 
> Also add missing include of facility.h to mem.h.
> 
> Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
> ---
>   lib/s390x/asm/interrupt.h | 14 ++++++++++++++
>   lib/s390x/asm/mem.h       |  1 +
>   lib/s390x/sie.c           |  4 +++-
>   3 files changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/s390x/asm/interrupt.h b/lib/s390x/asm/interrupt.h
> index 55759002dce2..9e509d2f4f1e 100644
> --- a/lib/s390x/asm/interrupt.h
> +++ b/lib/s390x/asm/interrupt.h
> @@ -99,4 +99,18 @@ static inline void low_prot_disable(void)
>   	ctl_clear_bit(0, CTL0_LOW_ADDR_PROT);
>   }
>   
> +/**
> + * read_pgm_int_code - Get the program interruption code of the last pgm int
> + * on the current CPU.
> + *
> + * This is similar to clear_pgm_int(), except that it doesn't clear the
> + * interruption information from lowcore.
> + *
> + * Returns 0 when none occurred.
> + */
> +static inline uint16_t read_pgm_int_code(void)
> +{
> +	return lowcore.pgm_int_code;
> +}
> +
>   #endif
> diff --git a/lib/s390x/asm/mem.h b/lib/s390x/asm/mem.h
> index 64ef59b546a4..94d58c34f53f 100644
> --- a/lib/s390x/asm/mem.h
> +++ b/lib/s390x/asm/mem.h
> @@ -8,6 +8,7 @@
>   #ifndef _ASMS390X_MEM_H_
>   #define _ASMS390X_MEM_H_
>   #include <asm/arch_def.h>
> +#include <asm/facility.h>
>   
>   /* create pointer while avoiding compiler warnings */
>   #define OPAQUE_PTR(x) ((void *)(((uint64_t)&lowcore) + (x)))
> diff --git a/lib/s390x/sie.c b/lib/s390x/sie.c
> index ffa8ec91a423..632740edd431 100644
> --- a/lib/s390x/sie.c
> +++ b/lib/s390x/sie.c
> @@ -13,6 +13,7 @@
>   #include <libcflat.h>
>   #include <sie.h>
>   #include <asm/page.h>
> +#include <asm/interrupt.h>
>   #include <libcflat.h>
>   #include <alloc_page.h>
>   
> @@ -65,7 +66,8 @@ void sie(struct vm *vm)
>   	/* also handle all interruptions in home space while in SIE */
>   	irq_set_dat_mode(IRQ_DAT_ON, AS_HOME);
>   
> -	while (vm->sblk->icptcode == 0) {
> +	/* leave SIE when we have an intercept or an interrupt so the test can react to it */
> +	while (vm->sblk->icptcode == 0 && !read_pgm_int_code()) {
>   		sie64a(vm->sblk, &vm->save_area);
>   		sie_handle_validity(vm);
>   	}

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [kvm-unit-tests PATCH v5 3/6] s390x: sie: switch to home space mode before entering SIE

[Claudio Imbrenda]

On Thu, 13 Jul 2023 09:28:19 +0200
Thomas Huth <thuth@redhat.com> wrote:

[...]

> > +	irq_set_dat_mode(IRQ_DAT_ON, AS_PRIM);
> > +	psw_mask_clear_bits(PSW_MASK_HOME);
> > +
> > +	/* restore the old CR 13 */
> > +	lctlg(13, old_cr13);  
> 
> Wouldn't it be better to always switch to HOME address mode directly in our 
> startup code already (where we enable DAT)? Switching back and forth every 
> time we enter SIE looks confusing to me ... or is there a reason why we 
> should continue to run in primary address mode by default and only switch to 
> home mode here?

the existing tests are written with the assumption that they are
running in primary mode.

switching back and forth might be confusing, but avoids having to
fix all the tests

> 
>   Thomas
> 
> 

Re: [kvm-unit-tests PATCH v5 1/6] lib: s390x: introduce bitfield for PSW mask

[Claudio Imbrenda]

On Wed, 12 Jul 2023 13:41:44 +0200
Nico Boehr <nrb@linux.ibm.com> wrote:

> Changing the PSW mask is currently little clumsy, since there is only the
> PSW_MASK_* defines. This makes it hard to change e.g. only the address
> space in the current PSW without a lot of bit fiddling.
> 
> Introduce a bitfield for the PSW mask. This makes this kind of
> modifications much simpler and easier to read.
> 
> Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
> ---
>  lib/s390x/asm/arch_def.h | 26 +++++++++++++++++++++++++-
>  s390x/selftest.c         | 40 ++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 65 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
> index bb26e008cc68..53279572a9ee 100644
> --- a/lib/s390x/asm/arch_def.h
> +++ b/lib/s390x/asm/arch_def.h
> @@ -37,12 +37,36 @@ struct stack_frame_int {
>  };
>  
>  struct psw {
> -	uint64_t	mask;
> +	union {
> +		uint64_t	mask;
> +		struct {
> +			uint8_t reserved00:1;
> +			uint8_t per:1;
> +			uint8_t reserved02:3;
> +			uint8_t dat:1;
> +			uint8_t io:1;
> +			uint8_t ext:1;
> +			uint8_t key:4;
> +			uint8_t reserved12:1;
> +			uint8_t mchk:1;
> +			uint8_t wait:1;
> +			uint8_t pstate:1;
> +			uint8_t as:2;
> +			uint8_t cc:2;
> +			uint8_t prg_mask:4;
> +			uint8_t reserved24:7;
> +			uint8_t ea:1;
> +			uint8_t ba:1;
> +			uint32_t reserved33:31;

since you will need to respin this anyway:

can you use uint64_t everywhere in the bitfield? it would look more
consistent and it would avoid some potential pitfalls of using
bitfields. In our case we're lucky because all fields are properly
aligned, but using uint64_t makes it easier to understand.

> +		};
> +	};
>  	uint64_t	addr;
>  };
> +_Static_assert(sizeof(struct psw) == 16, "PSW size");
>  
>  #define PSW(m, a) ((struct psw){ .mask = (m), .addr = (uint64_t)(a) })
>  
> +
>  struct short_psw {
>  	uint32_t	mask;
>  	uint32_t	addr;
> diff --git a/s390x/selftest.c b/s390x/selftest.c
> index 13fd36bc06f8..8d81ba312279 100644
> --- a/s390x/selftest.c
> +++ b/s390x/selftest.c
> @@ -74,6 +74,45 @@ static void test_malloc(void)
>  	report_prefix_pop();
>  }
>  
> +static void test_psw_mask(void)
> +{
> +	uint64_t expected_key = 0xF;

I think we prefer lowercase hex

> +	struct psw test_psw = PSW(0, 0);
> +
> +	report_prefix_push("PSW mask");
> +	test_psw.dat = 1;
> +	report(test_psw.mask == PSW_MASK_DAT, "DAT matches expected=0x%016lx actual=0x%016lx", PSW_MASK_DAT, test_psw.mask);
> +
> +	test_psw.mask = 0;
> +	test_psw.io = 1;
> +	report(test_psw.mask == PSW_MASK_IO, "IO matches expected=0x%016lx actual=0x%016lx", PSW_MASK_IO, test_psw.mask);
> +
> +	test_psw.mask = 0;
> +	test_psw.ext = 1;
> +	report(test_psw.mask == PSW_MASK_EXT, "EXT matches expected=0x%016lx actual=0x%016lx", PSW_MASK_EXT, test_psw.mask);
> +
> +	test_psw.mask = expected_key << (63 - 11);
> +	report(test_psw.key == expected_key, "PSW Key matches expected=0x%lx actual=0x%x", expected_key, test_psw.key);
> +
> +	test_psw.mask = 1UL << (63 - 13);
> +	report(test_psw.mchk, "MCHK matches");
> +
> +	test_psw.mask = 0;
> +	test_psw.wait = 1;
> +	report(test_psw.mask == PSW_MASK_WAIT, "Wait matches expected=0x%016lx actual=0x%016lx", PSW_MASK_WAIT, test_psw.mask);
> +
> +	test_psw.mask = 0;
> +	test_psw.pstate = 1;
> +	report(test_psw.mask == PSW_MASK_PSTATE, "Pstate matches expected=0x%016lx actual=0x%016lx", PSW_MASK_PSTATE, test_psw.mask);
> +
> +	test_psw.mask = 0;
> +	test_psw.ea = 1;
> +	test_psw.ba = 1;
> +	report(test_psw.mask == PSW_MASK_64, "BA/EA matches expected=0x%016lx actual=0x%016lx", PSW_MASK_64, test_psw.mask);
> +
> +	report_prefix_pop();
> +}
> +
>  int main(int argc, char**argv)
>  {
>  	report_prefix_push("selftest");
> @@ -89,6 +128,7 @@ int main(int argc, char**argv)
>  	test_fp();
>  	test_pgm_int();
>  	test_malloc();
> +	test_psw_mask();
>  
>  	return report_summary();
>  }

Re: [kvm-unit-tests PATCH v5 3/6] s390x: sie: switch to home space mode before entering SIE

[Thomas Huth]

On 13/07/2023 10.17, Claudio Imbrenda wrote:
> On Thu, 13 Jul 2023 09:28:19 +0200
> Thomas Huth <thuth@redhat.com> wrote:
> 
> [...]
> 
>>> +	irq_set_dat_mode(IRQ_DAT_ON, AS_PRIM);
>>> +	psw_mask_clear_bits(PSW_MASK_HOME);
>>> +
>>> +	/* restore the old CR 13 */
>>> +	lctlg(13, old_cr13);
>>
>> Wouldn't it be better to always switch to HOME address mode directly in our
>> startup code already (where we enable DAT)? Switching back and forth every
>> time we enter SIE looks confusing to me ... or is there a reason why we
>> should continue to run in primary address mode by default and only switch to
>> home mode here?
> 
> the existing tests are written with the assumption that they are
> running in primary mode.
> 
> switching back and forth might be confusing, but avoids having to
> fix all the tests

Which tests are breaking? And why? And how much effort would it be to fix them?

  Thomas

Re: [kvm-unit-tests PATCH v5 2/6] s390x: add function to set DAT mode for all interrupts

[Claudio Imbrenda]

On Thu, 13 Jul 2023 09:17:28 +0200
Thomas Huth <thuth@redhat.com> wrote:

> On 12/07/2023 13.41, Nico Boehr wrote:
> > When toggling DAT or switch address space modes, it is likely that
> > interrupts should be handled in the same DAT or address space mode.
> > 
> > Add a function which toggles DAT and address space mode for all
> > interruptions, except restart interrupts.
> > 
> > Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
> > ---
> >   lib/s390x/asm/interrupt.h |  4 ++++
> >   lib/s390x/interrupt.c     | 36 ++++++++++++++++++++++++++++++++++++
> >   lib/s390x/mmu.c           |  5 +++--
> >   3 files changed, 43 insertions(+), 2 deletions(-)
> > 
> > diff --git a/lib/s390x/asm/interrupt.h b/lib/s390x/asm/interrupt.h
> > index 35c1145f0349..55759002dce2 100644
> > --- a/lib/s390x/asm/interrupt.h
> > +++ b/lib/s390x/asm/interrupt.h
> > @@ -83,6 +83,10 @@ void expect_ext_int(void);
> >   uint16_t clear_pgm_int(void);
> >   void check_pgm_int_code(uint16_t code);
> >   
> > +#define IRQ_DAT_ON	true
> > +#define IRQ_DAT_OFF	false  
> 
> Just a matter of taste, but IMHO having defines like this for just using 
> them as boolean parameter to one function is a little bit overkill already. 
> I'd rather rename the "bool dat" below into "bool use_dat" and then use 
> "true" and "false" directly as a parameter for that function instead. 
> Anyway, just my 0.02 €.

+1

(or an enum, but I think a bool would be better, since it's a boolean
value)

[...]

> > + * and egg situation.
> > + *
> > + * @dat specifies whether to use DAT or not
> > + * @as specifies the address space mode to use - one of AS_PRIM, AS_ACCR,
> > + * AS_SECN or AS_HOME.
> > + */
> > +void irq_set_dat_mode(bool dat, uint64_t as)  
> 
> why uint64_t for "as" ? "int" should be enough?

or even a char ;)

> 
> (alternatively, you could turn the AS_* defines into a properly named enum 
> and use that type here instead)

I like this more ^

> 
>   Thomas
> 
> > +{
> > +	struct psw* irq_psws[] = {
> > +		OPAQUE_PTR(GEN_LC_EXT_NEW_PSW),
> > +		OPAQUE_PTR(GEN_LC_SVC_NEW_PSW),
> > +		OPAQUE_PTR(GEN_LC_PGM_NEW_PSW),
> > +		OPAQUE_PTR(GEN_LC_MCCK_NEW_PSW),
> > +		OPAQUE_PTR(GEN_LC_IO_NEW_PSW),
> > +	};
> > +	struct psw *psw;
> > +
> > +	assert(as == AS_PRIM || as == AS_ACCR || as == AS_SECN || as == AS_HOME);
> > +
> > +	for (size_t i = 0; i < ARRAY_SIZE(irq_psws); i++) {
> > +		psw = irq_psws[i];
> > +		psw->dat = dat;
> > +		if (dat)
> > +			psw->as = as;
> > +	}
> > +}
> > +
> >   static void fixup_pgm_int(struct stack_frame_int *stack)
> >   {
> >   	/* If we have an error on SIE we directly move to sie_exit */
> > diff --git a/lib/s390x/mmu.c b/lib/s390x/mmu.c
> > index b474d7021d3f..199bd3fbc9c8 100644
> > --- a/lib/s390x/mmu.c
> > +++ b/lib/s390x/mmu.c
> > @@ -12,6 +12,7 @@
> >   #include <asm/pgtable.h>
> >   #include <asm/arch_def.h>
> >   #include <asm/barrier.h>
> > +#include <asm/interrupt.h>
> >   #include <vmalloc.h>
> >   #include "mmu.h"
> >   
> > @@ -41,8 +42,8 @@ static void mmu_enable(pgd_t *pgtable)
> >   	/* enable dat (primary == 0 set as default) */
> >   	enable_dat();
> >   
> > -	/* we can now also use DAT unconditionally in our PGM handler */
> > -	lowcore.pgm_new_psw.mask |= PSW_MASK_DAT;
> > +	/* we can now also use DAT in all interrupt handlers */
> > +	irq_set_dat_mode(IRQ_DAT_ON, AS_PRIM);
> >   }
> >   
> >   /*  
> 

Re: [kvm-unit-tests PATCH v5 2/6] s390x: add function to set DAT mode for all interrupts

[Claudio Imbrenda]

On Wed, 12 Jul 2023 13:41:45 +0200
Nico Boehr <nrb@linux.ibm.com> wrote:

[...]

> +/**
> + * irq_set_dat_mode - Set the DAT mode of all interrupt handlers, except for
> + * restart.
> + * This will update the DAT mode and address space mode of all interrupt new
> + * PSWs.
> + *
> + * Since enabling DAT needs initalized CRs and the restart new PSW is often used
> + * to initalize CRs, the restart new PSW is never touched to avoid the chicken
> + * and egg situation.
> + *
> + * @dat specifies whether to use DAT or not
> + * @as specifies the address space mode to use - one of AS_PRIM, AS_ACCR,

please mention here that  as  will not be set if  dat == false

> + * AS_SECN or AS_HOME.
> + */
> +void irq_set_dat_mode(bool dat, uint64_t as)
> +{
> +	struct psw* irq_psws[] = {
> +		OPAQUE_PTR(GEN_LC_EXT_NEW_PSW),
> +		OPAQUE_PTR(GEN_LC_SVC_NEW_PSW),
> +		OPAQUE_PTR(GEN_LC_PGM_NEW_PSW),
> +		OPAQUE_PTR(GEN_LC_MCCK_NEW_PSW),
> +		OPAQUE_PTR(GEN_LC_IO_NEW_PSW),
> +	};
> +	struct psw *psw;
> +
> +	assert(as == AS_PRIM || as == AS_ACCR || as == AS_SECN || as == AS_HOME);
> +
> +	for (size_t i = 0; i < ARRAY_SIZE(irq_psws); i++) {
> +		psw = irq_psws[i];
> +		psw->dat = dat;
> +		if (dat)
> +			psw->as = as;
> +	}
> +}
> +
>  static void fixup_pgm_int(struct stack_frame_int *stack)
>  {
>  	/* If we have an error on SIE we directly move to sie_exit */
> diff --git a/lib/s390x/mmu.c b/lib/s390x/mmu.c
> index b474d7021d3f..199bd3fbc9c8 100644
> --- a/lib/s390x/mmu.c
> +++ b/lib/s390x/mmu.c
> @@ -12,6 +12,7 @@
>  #include <asm/pgtable.h>
>  #include <asm/arch_def.h>
>  #include <asm/barrier.h>
> +#include <asm/interrupt.h>
>  #include <vmalloc.h>
>  #include "mmu.h"
>  
> @@ -41,8 +42,8 @@ static void mmu_enable(pgd_t *pgtable)
>  	/* enable dat (primary == 0 set as default) */
>  	enable_dat();
>  
> -	/* we can now also use DAT unconditionally in our PGM handler */
> -	lowcore.pgm_new_psw.mask |= PSW_MASK_DAT;
> +	/* we can now also use DAT in all interrupt handlers */
> +	irq_set_dat_mode(IRQ_DAT_ON, AS_PRIM);
>  }
>  
>  /*

Re: [kvm-unit-tests PATCH v5 6/6] s390x: add a test for SIE without MSO/MSL

[Thomas Huth]

On 12/07/2023 13.41, Nico Boehr wrote:
> Since we now have the ability to run guests without MSO/MSL, add a test
> to make sure this doesn't break.
> 
> Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
> ---
>   s390x/Makefile             |   2 +
>   s390x/sie-dat.c            | 115 +++++++++++++++++++++++++++++++++++++
>   s390x/snippets/c/sie-dat.c |  58 +++++++++++++++++++
>   s390x/unittests.cfg        |   3 +
>   4 files changed, 178 insertions(+)
>   create mode 100644 s390x/sie-dat.c
>   create mode 100644 s390x/snippets/c/sie-dat.c
> 
> diff --git a/s390x/Makefile b/s390x/Makefile
> index a80db538810e..4921669ee4c3 100644
> --- a/s390x/Makefile
> +++ b/s390x/Makefile
> @@ -40,6 +40,7 @@ tests += $(TEST_DIR)/panic-loop-pgm.elf
>   tests += $(TEST_DIR)/migration-sck.elf
>   tests += $(TEST_DIR)/exittime.elf
>   tests += $(TEST_DIR)/ex.elf
> +tests += $(TEST_DIR)/sie-dat.elf
>   
>   pv-tests += $(TEST_DIR)/pv-diags.elf
>   
> @@ -120,6 +121,7 @@ snippet_lib = $(snippet_asmlib) lib/auxinfo.o
>   # perquisites (=guests) for the snippet hosts.
>   # $(TEST_DIR)/<snippet-host>.elf: snippets = $(SNIPPET_DIR)/<c/asm>/<snippet>.gbin
>   $(TEST_DIR)/mvpg-sie.elf: snippets = $(SNIPPET_DIR)/c/mvpg-snippet.gbin
> +$(TEST_DIR)/sie-dat.elf: snippets = $(SNIPPET_DIR)/c/sie-dat.gbin
>   $(TEST_DIR)/spec_ex-sie.elf: snippets = $(SNIPPET_DIR)/c/spec_ex.gbin
>   
>   $(TEST_DIR)/pv-diags.elf: pv-snippets += $(SNIPPET_DIR)/asm/snippet-pv-diag-yield.gbin
> diff --git a/s390x/sie-dat.c b/s390x/sie-dat.c
> new file mode 100644
> index 000000000000..b326995dfa85
> --- /dev/null
> +++ b/s390x/sie-dat.c
> @@ -0,0 +1,115 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Tests SIE with paging.
> + *
> + * Copyright 2023 IBM Corp.
> + *
> + * Authors:
> + *    Nico Boehr <nrb@linux.ibm.com>
> + */
> +#include <libcflat.h>
> +#include <vmalloc.h>
> +#include <asm/pgtable.h>
> +#include <mmu.h>
> +#include <asm/page.h>
> +#include <asm/interrupt.h>
> +#include <alloc_page.h>
> +#include <sclp.h>
> +#include <sie.h>
> +#include <snippet.h>
> +
> +static struct vm vm;
> +static pgd_t *guest_root;
> +
> +/* keep in sync with TEST_PAGE_COUNT in s390x/snippets/c/sie-dat.c */
> +#define GUEST_TEST_PAGE_COUNT 10
> +
> +/* keep in sync with TOTAL_PAGE_COUNT in s390x/snippets/c/sie-dat.c */
> +#define GUEST_TOTAL_PAGE_COUNT 256

I'd maybe put the defines rather in a header a la s390x/snippets/c/sie-dat.h 
and include that header here and in the snippet C code.

Apart from that, test looks good to me:
Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [kvm-unit-tests PATCH v5 1/6] lib: s390x: introduce bitfield for PSW mask

[Nico Boehr]

Quoting Thomas Huth (2023-07-13 08:56:41)
> On 12/07/2023 13.41, Nico Boehr wrote:
> > Changing the PSW mask is currently little clumsy, since there is only the
> > PSW_MASK_* defines. This makes it hard to change e.g. only the address
> > space in the current PSW without a lot of bit fiddling.
> > 
> > Introduce a bitfield for the PSW mask. This makes this kind of
> > modifications much simpler and easier to read.
> > 
> > Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
> > ---
> >   lib/s390x/asm/arch_def.h | 26 +++++++++++++++++++++++++-
> >   s390x/selftest.c         | 40 ++++++++++++++++++++++++++++++++++++++++
> >   2 files changed, 65 insertions(+), 1 deletion(-)
> > 
> > diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
> > index bb26e008cc68..53279572a9ee 100644
> > --- a/lib/s390x/asm/arch_def.h
> > +++ b/lib/s390x/asm/arch_def.h
> > @@ -37,12 +37,36 @@ struct stack_frame_int {
> >   };
> >   
> >   struct psw {
> > -     uint64_t        mask;
> > +     union {
> > +             uint64_t        mask;
> > +             struct {
> > +                     uint8_t reserved00:1;
> > +                     uint8_t per:1;
> > +                     uint8_t reserved02:3;
> > +                     uint8_t dat:1;
> > +                     uint8_t io:1;
> > +                     uint8_t ext:1;
> > +                     uint8_t key:4;
> > +                     uint8_t reserved12:1;
> > +                     uint8_t mchk:1;
> > +                     uint8_t wait:1;
> > +                     uint8_t pstate:1;
> > +                     uint8_t as:2;
> > +                     uint8_t cc:2;
> > +                     uint8_t prg_mask:4;
> > +                     uint8_t reserved24:7;
> > +                     uint8_t ea:1;
> > +                     uint8_t ba:1;
> > +                     uint32_t reserved33:31;
> > +             };
> > +     };
> >       uint64_t        addr;
> >   };
> > +_Static_assert(sizeof(struct psw) == 16, "PSW size");
> >   
> >   #define PSW(m, a) ((struct psw){ .mask = (m), .addr = (uint64_t)(a) })
> >   
> > +
> >   struct short_psw {
> >       uint32_t        mask;
> >       uint32_t        addr;
> > diff --git a/s390x/selftest.c b/s390x/selftest.c
> > index 13fd36bc06f8..8d81ba312279 100644
> > --- a/s390x/selftest.c
> > +++ b/s390x/selftest.c
> > @@ -74,6 +74,45 @@ static void test_malloc(void)
> >       report_prefix_pop();
> >   }
> >   
> > +static void test_psw_mask(void)
> > +{
> > +     uint64_t expected_key = 0xF;
> > +     struct psw test_psw = PSW(0, 0);
> > +
> > +     report_prefix_push("PSW mask");
> > +     test_psw.dat = 1;
> > +     report(test_psw.mask == PSW_MASK_DAT, "DAT matches expected=0x%016lx actual=0x%016lx", PSW_MASK_DAT, test_psw.mask);
> > +
> > +     test_psw.mask = 0;
> > +     test_psw.io = 1;
> > +     report(test_psw.mask == PSW_MASK_IO, "IO matches expected=0x%016lx actual=0x%016lx", PSW_MASK_IO, test_psw.mask);
> > +
> > +     test_psw.mask = 0;
> > +     test_psw.ext = 1;
> > +     report(test_psw.mask == PSW_MASK_EXT, "EXT matches expected=0x%016lx actual=0x%016lx", PSW_MASK_EXT, test_psw.mask);
> > +
> > +     test_psw.mask = expected_key << (63 - 11);
> > +     report(test_psw.key == expected_key, "PSW Key matches expected=0x%lx actual=0x%x", expected_key, test_psw.key);
> 
> Patch looks basically fine to me, but here my mind stumbled a little bit. 
> This test is written the other way round than the others. Nothing wrong with 
> that, it just feels a little bit inconsistent. I'd suggest to either do:
> 
>         test_psw.mask = 0;
>         test_psw.key = expected_key;
>         report(test_psw.mask == expected_key << (63 - 11), ...);
> 
> or maybe even switch all the other tests around instead, so you could get 
> rid of the "test_psw.mask = 0" lines, e.g. :
> 
>         test_psw.mask == PSW_MASK_IO;
>         report(test_psw.io, "IO matches ...");
> 
> etc.

I like the latter option, thanks.

Re: [kvm-unit-tests PATCH v5 1/6] lib: s390x: introduce bitfield for PSW mask

[Nico Boehr]

Quoting Claudio Imbrenda (2023-07-13 10:20:13)
[...]
> > diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
> > index bb26e008cc68..53279572a9ee 100644
> > --- a/lib/s390x/asm/arch_def.h
> > +++ b/lib/s390x/asm/arch_def.h
> > @@ -37,12 +37,36 @@ struct stack_frame_int {
> >  };
> >  
> >  struct psw {
> > -     uint64_t        mask;
> > +     union {
> > +             uint64_t        mask;
> > +             struct {
> > +                     uint8_t reserved00:1;
> > +                     uint8_t per:1;
> > +                     uint8_t reserved02:3;
> > +                     uint8_t dat:1;
> > +                     uint8_t io:1;
> > +                     uint8_t ext:1;
> > +                     uint8_t key:4;
> > +                     uint8_t reserved12:1;
> > +                     uint8_t mchk:1;
> > +                     uint8_t wait:1;
> > +                     uint8_t pstate:1;
> > +                     uint8_t as:2;
> > +                     uint8_t cc:2;
> > +                     uint8_t prg_mask:4;
> > +                     uint8_t reserved24:7;
> > +                     uint8_t ea:1;
> > +                     uint8_t ba:1;
> > +                     uint32_t reserved33:31;
> 
> since you will need to respin this anyway:
> 
> can you use uint64_t everywhere in the bitfield? it would look more
> consistent and it would avoid some potential pitfalls of using
> bitfields. In our case we're lucky because all fields are properly
> aligned, but using uint64_t makes it easier to understand.

I will do that for consistency, I'd be interested to understand what pitfalls
you're talking about and how uint64_t avoids them, since we have a few bitfields
in the codebase.

Re: [kvm-unit-tests PATCH v5 2/6] s390x: add function to set DAT mode for all interrupts

[Nico Boehr]

Quoting Thomas Huth (2023-07-13 09:17:28)
> On 12/07/2023 13.41, Nico Boehr wrote:
> > When toggling DAT or switch address space modes, it is likely that
> > interrupts should be handled in the same DAT or address space mode.
> > 
> > Add a function which toggles DAT and address space mode for all
> > interruptions, except restart interrupts.
> > 
> > Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
> > ---
> >   lib/s390x/asm/interrupt.h |  4 ++++
> >   lib/s390x/interrupt.c     | 36 ++++++++++++++++++++++++++++++++++++
> >   lib/s390x/mmu.c           |  5 +++--
> >   3 files changed, 43 insertions(+), 2 deletions(-)
> > 
> > diff --git a/lib/s390x/asm/interrupt.h b/lib/s390x/asm/interrupt.h
> > index 35c1145f0349..55759002dce2 100644
> > --- a/lib/s390x/asm/interrupt.h
> > +++ b/lib/s390x/asm/interrupt.h
> > @@ -83,6 +83,10 @@ void expect_ext_int(void);
> >   uint16_t clear_pgm_int(void);
> >   void check_pgm_int_code(uint16_t code);
> >   
> > +#define IRQ_DAT_ON   true
> > +#define IRQ_DAT_OFF  false
> 
> Just a matter of taste, but IMHO having defines like this for just using 
> them as boolean parameter to one function is a little bit overkill already. 
> I'd rather rename the "bool dat" below into "bool use_dat" and then use 
> "true" and "false" directly as a parameter for that function instead. 
> Anyway, just my 0.02 \u20ac.

The point of having these defines was to convey the meaning of the parameter to my reader.

When I read

    irq_set_dat_mode(true, AS_HOME);

it's less clear to me that the first parameter toggles the DAT mode compared to this:

    irq_set_dat_mode(IRQ_DAT_ON, AS_HOME);

That being said, here it's pretty clear from the function name what the first parameter is, so what's the preferred opinion?

[...]
> > diff --git a/lib/s390x/interrupt.c b/lib/s390x/interrupt.c
> > index 3f993a363ae2..9b1bc6ce819d 100644
> > --- a/lib/s390x/interrupt.c
> > +++ b/lib/s390x/interrupt.c
> > @@ -9,6 +9,7 @@
> >    */
> >   #include <libcflat.h>
> >   #include <asm/barrier.h>
> > +#include <asm/mem.h>
> >   #include <asm/asm-offsets.h>
> >   #include <sclp.h>
> >   #include <interrupt.h>
> > @@ -104,6 +105,41 @@ void register_ext_cleanup_func(void (*f)(struct stack_frame_int *))
> >       THIS_CPU->ext_cleanup_func = f;
> >   }
> >   
> > +/**
> > + * irq_set_dat_mode - Set the DAT mode of all interrupt handlers, except for
> > + * restart.
> > + * This will update the DAT mode and address space mode of all interrupt new
> > + * PSWs.
> > + *
> > + * Since enabling DAT needs initalized CRs and the restart new PSW is often used
> 
> s/initalized/initialized/

Argh, thanks.

*reprioritizes task to look for a spell checker*

> 
> > + * to initalize CRs, the restart new PSW is never touched to avoid the chicken
> 
> dito
> 
> > + * and egg situation.
> > + *
> > + * @dat specifies whether to use DAT or not
> > + * @as specifies the address space mode to use - one of AS_PRIM, AS_ACCR,
> > + * AS_SECN or AS_HOME.
> > + */
> > +void irq_set_dat_mode(bool dat, uint64_t as)
> 
> why uint64_t for "as" ? "int" should be enough?
> 
> (alternatively, you could turn the AS_* defines into a properly named enum 
> and use that type here instead)

Yes, let's just do that.

Re: [kvm-unit-tests PATCH v5 2/6] s390x: add function to set DAT mode for all interrupts

[Nico Boehr]

Quoting Claudio Imbrenda (2023-07-13 10:24:13)
> On Wed, 12 Jul 2023 13:41:45 +0200
> Nico Boehr <nrb@linux.ibm.com> wrote:
> 
> [...]
> 
> > +/**
> > + * irq_set_dat_mode - Set the DAT mode of all interrupt handlers, except for
> > + * restart.
> > + * This will update the DAT mode and address space mode of all interrupt new
> > + * PSWs.
> > + *
> > + * Since enabling DAT needs initalized CRs and the restart new PSW is often used
> > + * to initalize CRs, the restart new PSW is never touched to avoid the chicken
> > + * and egg situation.
> > + *
> > + * @dat specifies whether to use DAT or not
> > + * @as specifies the address space mode to use - one of AS_PRIM, AS_ACCR,
> 
> please mention here that  as  will not be set if  dat == false

Right, done.

Re: [kvm-unit-tests PATCH v5 2/6] s390x: add function to set DAT mode for all interrupts

[Thomas Huth]

On 13/07/2023 17.30, Nico Boehr wrote:
> Quoting Thomas Huth (2023-07-13 09:17:28)
>> On 12/07/2023 13.41, Nico Boehr wrote:
>>> When toggling DAT or switch address space modes, it is likely that
>>> interrupts should be handled in the same DAT or address space mode.
>>>
>>> Add a function which toggles DAT and address space mode for all
>>> interruptions, except restart interrupts.
>>>
>>> Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
>>> ---
>>>    lib/s390x/asm/interrupt.h |  4 ++++
>>>    lib/s390x/interrupt.c     | 36 ++++++++++++++++++++++++++++++++++++
>>>    lib/s390x/mmu.c           |  5 +++--
>>>    3 files changed, 43 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/lib/s390x/asm/interrupt.h b/lib/s390x/asm/interrupt.h
>>> index 35c1145f0349..55759002dce2 100644
>>> --- a/lib/s390x/asm/interrupt.h
>>> +++ b/lib/s390x/asm/interrupt.h
>>> @@ -83,6 +83,10 @@ void expect_ext_int(void);
>>>    uint16_t clear_pgm_int(void);
>>>    void check_pgm_int_code(uint16_t code);
>>>    
>>> +#define IRQ_DAT_ON   true
>>> +#define IRQ_DAT_OFF  false
>>
>> Just a matter of taste, but IMHO having defines like this for just using
>> them as boolean parameter to one function is a little bit overkill already.
>> I'd rather rename the "bool dat" below into "bool use_dat" and then use
>> "true" and "false" directly as a parameter for that function instead.
>> Anyway, just my 0.02 \u20ac.
> 
> The point of having these defines was to convey the meaning of the parameter to my reader.
> 
> When I read
> 
>      irq_set_dat_mode(true, AS_HOME);
> 
> it's less clear to me that the first parameter toggles the DAT mode compared to this:
> 
>      irq_set_dat_mode(IRQ_DAT_ON, AS_HOME);
> 
> That being said, here it's pretty clear from the function name what the first parameter is, so what's the preferred opinion?

I see your point, but if it is clear from the function name like it is in 
this case, I'd go with "true" and "false" directly, without the indirection 
via #define.

...
>>> + * Since enabling DAT needs initalized CRs and the restart new PSW is often used
>>
>> s/initalized/initialized/
> 
> Argh, thanks.
> 
> *reprioritizes task to look for a spell checker*

codespell (https://github.com/codespell-project/codespell) is your friend!

  Thomas

Re: [kvm-unit-tests PATCH v5 3/6] s390x: sie: switch to home space mode before entering SIE

[Nico Boehr]

Quoting Thomas Huth (2023-07-13 10:21:12)
> On 13/07/2023 10.17, Claudio Imbrenda wrote:
> > On Thu, 13 Jul 2023 09:28:19 +0200
> > Thomas Huth <thuth@redhat.com> wrote:
> > 
> > [...]
> > 
> >>> +   irq_set_dat_mode(IRQ_DAT_ON, AS_PRIM);
> >>> +   psw_mask_clear_bits(PSW_MASK_HOME);
> >>> +
> >>> +   /* restore the old CR 13 */
> >>> +   lctlg(13, old_cr13);
> >>
> >> Wouldn't it be better to always switch to HOME address mode directly in our
> >> startup code already (where we enable DAT)? Switching back and forth every
> >> time we enter SIE looks confusing to me ... or is there a reason why we
> >> should continue to run in primary address mode by default and only switch to
> >> home mode here?
> > 
> > the existing tests are written with the assumption that they are
> > running in primary mode.
> > 
> > switching back and forth might be confusing, but avoids having to
> > fix all the tests
> 
> Which tests are breaking? And why? And how much effort would it be to fix them?

Since you're not the first asking this, I took the time and
moved^Whacked everything to home space mode:

- all SIE-related tests time out, even when we load CR1 properly before SIE
  entry. Most likely just an oversight and fixable.
- the skey test encounters an unexpected PGM int with a weird backtrace
  where I couldn't easily figure out what goes wrong
- edat test fails with a similar looking backtrace

All in all, it is probably fixable, but additional effort.

I think explicitly switching the address space mode gives us additional
flexibility, since sie() doesn't need to make assumptions about which address
space we're running in. It currently does, but that's fixable - in contrast to
when we don't switch.

Re: [kvm-unit-tests PATCH v5 3/6] s390x: sie: switch to home space mode before entering SIE

[Thomas Huth]

On 14/07/2023 10.21, Nico Boehr wrote:
> Quoting Thomas Huth (2023-07-13 10:21:12)
>> On 13/07/2023 10.17, Claudio Imbrenda wrote:
>>> On Thu, 13 Jul 2023 09:28:19 +0200
>>> Thomas Huth <thuth@redhat.com> wrote:
>>>
>>> [...]
>>>
>>>>> +   irq_set_dat_mode(IRQ_DAT_ON, AS_PRIM);
>>>>> +   psw_mask_clear_bits(PSW_MASK_HOME);
>>>>> +
>>>>> +   /* restore the old CR 13 */
>>>>> +   lctlg(13, old_cr13);
>>>>
>>>> Wouldn't it be better to always switch to HOME address mode directly in our
>>>> startup code already (where we enable DAT)? Switching back and forth every
>>>> time we enter SIE looks confusing to me ... or is there a reason why we
>>>> should continue to run in primary address mode by default and only switch to
>>>> home mode here?
>>>
>>> the existing tests are written with the assumption that they are
>>> running in primary mode.
>>>
>>> switching back and forth might be confusing, but avoids having to
>>> fix all the tests
>>
>> Which tests are breaking? And why? And how much effort would it be to fix them?
> 
> Since you're not the first asking this, I took the time and
> moved^Whacked everything to home space mode:
> 
> - all SIE-related tests time out, even when we load CR1 properly before SIE
>    entry. Most likely just an oversight and fixable.
> - the skey test encounters an unexpected PGM int with a weird backtrace
>    where I couldn't easily figure out what goes wrong
> - edat test fails with a similar looking backtrace
> 
> All in all, it is probably fixable, but additional effort.
> 
> I think explicitly switching the address space mode gives us additional
> flexibility, since sie() doesn't need to make assumptions about which address
> space we're running in.

Ok, thanks for checking. Then let's go with this patch here for now, but 
maybe you could add a more detailed comment in the source code that talks 
about the reasons for switching each time instead of only once during startup?

Thanks,
  Thomas

Re: [kvm-unit-tests PATCH v5 6/6] s390x: add a test for SIE without MSO/MSL

[Nico Boehr]

Quoting Thomas Huth (2023-07-13 10:29:48)
[...]
> > diff --git a/s390x/sie-dat.c b/s390x/sie-dat.c
> > new file mode 100644
> > index 000000000000..b326995dfa85
> > --- /dev/null
> > +++ b/s390x/sie-dat.c
> > @@ -0,0 +1,115 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +/*
> > + * Tests SIE with paging.
> > + *
> > + * Copyright 2023 IBM Corp.
> > + *
> > + * Authors:
> > + *    Nico Boehr <nrb@linux.ibm.com>
> > + */
> > +#include <libcflat.h>
> > +#include <vmalloc.h>
> > +#include <asm/pgtable.h>
> > +#include <mmu.h>
> > +#include <asm/page.h>
> > +#include <asm/interrupt.h>
> > +#include <alloc_page.h>
> > +#include <sclp.h>
> > +#include <sie.h>
> > +#include <snippet.h>
> > +
> > +static struct vm vm;
> > +static pgd_t *guest_root;
> > +
> > +/* keep in sync with TEST_PAGE_COUNT in s390x/snippets/c/sie-dat.c */
> > +#define GUEST_TEST_PAGE_COUNT 10
> > +
> > +/* keep in sync with TOTAL_PAGE_COUNT in s390x/snippets/c/sie-dat.c */
> > +#define GUEST_TOTAL_PAGE_COUNT 256
> 
> I'd maybe put the defines rather in a header a la s390x/snippets/c/sie-dat.h 
> and include that header here and in the snippet C code.

I'd have to

#include "../s390x/snippets/c/sie-dat.h"

and it feels like I shouldn't be doing this, should I?

Or move the include to lib, but we agreed we don't want test-specific stuff
in the lib.

Re: [kvm-unit-tests PATCH v5 6/6] s390x: add a test for SIE without MSO/MSL

[Thomas Huth]

On 14/07/2023 10.35, Nico Boehr wrote:
> Quoting Thomas Huth (2023-07-13 10:29:48)
> [...]
>>> diff --git a/s390x/sie-dat.c b/s390x/sie-dat.c
>>> new file mode 100644
>>> index 000000000000..b326995dfa85
>>> --- /dev/null
>>> +++ b/s390x/sie-dat.c
>>> @@ -0,0 +1,115 @@
>>> +/* SPDX-License-Identifier: GPL-2.0-only */
>>> +/*
>>> + * Tests SIE with paging.
>>> + *
>>> + * Copyright 2023 IBM Corp.
>>> + *
>>> + * Authors:
>>> + *    Nico Boehr <nrb@linux.ibm.com>
>>> + */
>>> +#include <libcflat.h>
>>> +#include <vmalloc.h>
>>> +#include <asm/pgtable.h>
>>> +#include <mmu.h>
>>> +#include <asm/page.h>
>>> +#include <asm/interrupt.h>
>>> +#include <alloc_page.h>
>>> +#include <sclp.h>
>>> +#include <sie.h>
>>> +#include <snippet.h>
>>> +
>>> +static struct vm vm;
>>> +static pgd_t *guest_root;
>>> +
>>> +/* keep in sync with TEST_PAGE_COUNT in s390x/snippets/c/sie-dat.c */
>>> +#define GUEST_TEST_PAGE_COUNT 10
>>> +
>>> +/* keep in sync with TOTAL_PAGE_COUNT in s390x/snippets/c/sie-dat.c */
>>> +#define GUEST_TOTAL_PAGE_COUNT 256
>>
>> I'd maybe put the defines rather in a header a la s390x/snippets/c/sie-dat.h
>> and include that header here and in the snippet C code.
> 
> I'd have to
> 
> #include "../s390x/snippets/c/sie-dat.h"
> 
> and it feels like I shouldn't be doing this, should I?

Why "../s390x/" ? Isn't #include "snippets/c/sie-dat.h" enough? ... that 
would look reasonable to me.

  Thomas

Re: [kvm-unit-tests PATCH v5 3/6] s390x: sie: switch to home space mode before entering SIE

[Nico Boehr]

Quoting Thomas Huth (2023-07-14 10:30:33)
> On 14/07/2023 10.21, Nico Boehr wrote:
> > Quoting Thomas Huth (2023-07-13 10:21:12)
> >> On 13/07/2023 10.17, Claudio Imbrenda wrote:
> >>> On Thu, 13 Jul 2023 09:28:19 +0200
> >>> Thomas Huth <thuth@redhat.com> wrote:
> >>>
> >>> [...]
> >>>
> >>>>> +   irq_set_dat_mode(IRQ_DAT_ON, AS_PRIM);
> >>>>> +   psw_mask_clear_bits(PSW_MASK_HOME);
> >>>>> +
> >>>>> +   /* restore the old CR 13 */
> >>>>> +   lctlg(13, old_cr13);
> >>>>
> >>>> Wouldn't it be better to always switch to HOME address mode directly in our
> >>>> startup code already (where we enable DAT)? Switching back and forth every
> >>>> time we enter SIE looks confusing to me ... or is there a reason why we
> >>>> should continue to run in primary address mode by default and only switch to
> >>>> home mode here?
> >>>
> >>> the existing tests are written with the assumption that they are
> >>> running in primary mode.
> >>>
> >>> switching back and forth might be confusing, but avoids having to
> >>> fix all the tests
> >>
> >> Which tests are breaking? And why? And how much effort would it be to fix them?
> > 
> > Since you're not the first asking this, I took the time and
> > moved^Whacked everything to home space mode:
> > 
> > - all SIE-related tests time out, even when we load CR1 properly before SIE
> >    entry. Most likely just an oversight and fixable.
> > - the skey test encounters an unexpected PGM int with a weird backtrace
> >    where I couldn't easily figure out what goes wrong
> > - edat test fails with a similar looking backtrace
> > 
> > All in all, it is probably fixable, but additional effort.
> > 
> > I think explicitly switching the address space mode gives us additional
> > flexibility, since sie() doesn't need to make assumptions about which address
> > space we're running in.
> 
> Ok, thanks for checking. Then let's go with this patch here for now, but 
> maybe you could add a more detailed comment in the source code that talks 
> about the reasons for switching each time instead of only once during startup?

OK, how about this?

* Set up home address space to match primary space. Instead of running
* in home space all the time, we switch every time in sie() because:
* - tests that depend on running in primary space mode don't need to be
*   touched
* - it avoids regressions in tests
* - switching every time makes it easier to extend this in the future,
*   for example to allow tests to run in whatever space they want

Re: [kvm-unit-tests PATCH v5 2/6] s390x: add function to set DAT mode for all interrupts

[Nico Boehr]

Quoting Thomas Huth (2023-07-14 08:44:10)
> On 13/07/2023 17.30, Nico Boehr wrote:
> > Quoting Thomas Huth (2023-07-13 09:17:28)
> >> On 12/07/2023 13.41, Nico Boehr wrote:
> >>> When toggling DAT or switch address space modes, it is likely that
> >>> interrupts should be handled in the same DAT or address space mode.
> >>>
> >>> Add a function which toggles DAT and address space mode for all
> >>> interruptions, except restart interrupts.
> >>>
> >>> Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
> >>> ---
> >>>    lib/s390x/asm/interrupt.h |  4 ++++
> >>>    lib/s390x/interrupt.c     | 36 ++++++++++++++++++++++++++++++++++++
> >>>    lib/s390x/mmu.c           |  5 +++--
> >>>    3 files changed, 43 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/lib/s390x/asm/interrupt.h b/lib/s390x/asm/interrupt.h
> >>> index 35c1145f0349..55759002dce2 100644
> >>> --- a/lib/s390x/asm/interrupt.h
> >>> +++ b/lib/s390x/asm/interrupt.h
> >>> @@ -83,6 +83,10 @@ void expect_ext_int(void);
> >>>    uint16_t clear_pgm_int(void);
> >>>    void check_pgm_int_code(uint16_t code);
> >>>    
> >>> +#define IRQ_DAT_ON   true
> >>> +#define IRQ_DAT_OFF  false
> >>
> >> Just a matter of taste, but IMHO having defines like this for just using
> >> them as boolean parameter to one function is a little bit overkill already.
> >> I'd rather rename the "bool dat" below into "bool use_dat" and then use
> >> "true" and "false" directly as a parameter for that function instead.
> >> Anyway, just my 0.02 \u20ac.
> > 
> > The point of having these defines was to convey the meaning of the parameter to my reader.
> > 
> > When I read
> > 
> >      irq_set_dat_mode(true, AS_HOME);
> > 
> > it's less clear to me that the first parameter toggles the DAT mode compared to this:
> > 
> >      irq_set_dat_mode(IRQ_DAT_ON, AS_HOME);
> > 
> > That being said, here it's pretty clear from the function name what the first parameter is, so what's the preferred opinion?
> 
> I see your point, but if it is clear from the function name like it is in 
> this case, I'd go with "true" and "false" directly, without the indirection 
> via #define.

OK, will do.

> ...
> >>> + * Since enabling DAT needs initalized CRs and the restart new PSW is often used
> >>
> >> s/initalized/initialized/
> > 
> > Argh, thanks.
> > 
> > *reprioritizes task to look for a spell checker*
> 
> codespell (https://github.com/codespell-project/codespell) is your friend!

Thanks!

Re: [kvm-unit-tests PATCH v5 6/6] s390x: add a test for SIE without MSO/MSL

[Nico Boehr]

Quoting Thomas Huth (2023-07-14 10:40:28)
> On 14/07/2023 10.35, Nico Boehr wrote:
> > Quoting Thomas Huth (2023-07-13 10:29:48)
> > [...]
> >>> diff --git a/s390x/sie-dat.c b/s390x/sie-dat.c
> >>> new file mode 100644
> >>> index 000000000000..b326995dfa85
> >>> --- /dev/null
> >>> +++ b/s390x/sie-dat.c
> >>> @@ -0,0 +1,115 @@
> >>> +/* SPDX-License-Identifier: GPL-2.0-only */
> >>> +/*
> >>> + * Tests SIE with paging.
> >>> + *
> >>> + * Copyright 2023 IBM Corp.
> >>> + *
> >>> + * Authors:
> >>> + *    Nico Boehr <nrb@linux.ibm.com>
> >>> + */
> >>> +#include <libcflat.h>
> >>> +#include <vmalloc.h>
> >>> +#include <asm/pgtable.h>
> >>> +#include <mmu.h>
> >>> +#include <asm/page.h>
> >>> +#include <asm/interrupt.h>
> >>> +#include <alloc_page.h>
> >>> +#include <sclp.h>
> >>> +#include <sie.h>
> >>> +#include <snippet.h>
> >>> +
> >>> +static struct vm vm;
> >>> +static pgd_t *guest_root;
> >>> +
> >>> +/* keep in sync with TEST_PAGE_COUNT in s390x/snippets/c/sie-dat.c */
> >>> +#define GUEST_TEST_PAGE_COUNT 10
> >>> +
> >>> +/* keep in sync with TOTAL_PAGE_COUNT in s390x/snippets/c/sie-dat.c */
> >>> +#define GUEST_TOTAL_PAGE_COUNT 256
> >>
> >> I'd maybe put the defines rather in a header a la s390x/snippets/c/sie-dat.h
> >> and include that header here and in the snippet C code.
> > 
> > I'd have to
> > 
> > #include "../s390x/snippets/c/sie-dat.h"
> > 
> > and it feels like I shouldn't be doing this, should I?
> 
> Why "../s390x/" ? Isn't #include "snippets/c/sie-dat.h" enough? ... that 
> would look reasonable to me.

No, it isn't at least on my box:

s390x/snippets/c/sie-dat.c:15:10: fatal error: snippets/c/sie-dat.h: No such file or directory
   15 | #include "snippets/c/sie-dat.h"
      |          ^~~~~~~~~~~~~~~~~~~~~~
compilation terminated.

Re: [kvm-unit-tests PATCH v5 6/6] s390x: add a test for SIE without MSO/MSL

[Thomas Huth]

On 14/07/2023 12.39, Nico Boehr wrote:
> Quoting Thomas Huth (2023-07-14 10:40:28)
>> On 14/07/2023 10.35, Nico Boehr wrote:
>>> Quoting Thomas Huth (2023-07-13 10:29:48)
>>> [...]
>>>>> diff --git a/s390x/sie-dat.c b/s390x/sie-dat.c
>>>>> new file mode 100644
>>>>> index 000000000000..b326995dfa85
>>>>> --- /dev/null
>>>>> +++ b/s390x/sie-dat.c
>>>>> @@ -0,0 +1,115 @@
>>>>> +/* SPDX-License-Identifier: GPL-2.0-only */
>>>>> +/*
>>>>> + * Tests SIE with paging.
>>>>> + *
>>>>> + * Copyright 2023 IBM Corp.
>>>>> + *
>>>>> + * Authors:
>>>>> + *    Nico Boehr <nrb@linux.ibm.com>
>>>>> + */
>>>>> +#include <libcflat.h>
>>>>> +#include <vmalloc.h>
>>>>> +#include <asm/pgtable.h>
>>>>> +#include <mmu.h>
>>>>> +#include <asm/page.h>
>>>>> +#include <asm/interrupt.h>
>>>>> +#include <alloc_page.h>
>>>>> +#include <sclp.h>
>>>>> +#include <sie.h>
>>>>> +#include <snippet.h>
>>>>> +
>>>>> +static struct vm vm;
>>>>> +static pgd_t *guest_root;
>>>>> +
>>>>> +/* keep in sync with TEST_PAGE_COUNT in s390x/snippets/c/sie-dat.c */
>>>>> +#define GUEST_TEST_PAGE_COUNT 10
>>>>> +
>>>>> +/* keep in sync with TOTAL_PAGE_COUNT in s390x/snippets/c/sie-dat.c */
>>>>> +#define GUEST_TOTAL_PAGE_COUNT 256
>>>>
>>>> I'd maybe put the defines rather in a header a la s390x/snippets/c/sie-dat.h
>>>> and include that header here and in the snippet C code.
>>>
>>> I'd have to
>>>
>>> #include "../s390x/snippets/c/sie-dat.h"
>>>
>>> and it feels like I shouldn't be doing this, should I?
>>
>> Why "../s390x/" ? Isn't #include "snippets/c/sie-dat.h" enough? ... that
>> would look reasonable to me.
> 
> No, it isn't at least on my box:
> 
> s390x/snippets/c/sie-dat.c:15:10: fatal error: snippets/c/sie-dat.h: No such file or directory
>     15 | #include "snippets/c/sie-dat.h"
>        |          ^~~~~~~~~~~~~~~~~~~~~~
> compilation terminated.

Maybe add $(SRCDIR)/s390x to INCLUDE_PATHS in the s390x/Makefile ?

  Thomas

Re: [kvm-unit-tests PATCH v5 6/6] s390x: add a test for SIE without MSO/MSL

[Nico Boehr]

Quoting Thomas Huth (2023-07-14 12:52:59)
[...]
> Maybe add $(SRCDIR)/s390x to INCLUDE_PATHS in the s390x/Makefile ?

Yeah, that would work, but do we want that? I'd assume that it is a
concious decision not to have tests depend on one another.

Re: [kvm-unit-tests PATCH v5 6/6] s390x: add a test for SIE without MSO/MSL

[Thomas Huth]

On 01/08/2023 08.51, Nico Boehr wrote:
> Quoting Thomas Huth (2023-07-14 12:52:59)
> [...]
>> Maybe add $(SRCDIR)/s390x to INCLUDE_PATHS in the s390x/Makefile ?
> 
> Yeah, that would work, but do we want that? I'd assume that it is a
> concious decision not to have tests depend on one another.

IMHO this would still be OK ... Janosch, Claudio, what's your opinion on this?

  Thomas

Re: [kvm-unit-tests PATCH v5 6/6] s390x: add a test for SIE without MSO/MSL

[Janosch Frank]

On 8/14/23 16:59, Thomas Huth wrote:
> On 01/08/2023 08.51, Nico Boehr wrote:
>> Quoting Thomas Huth (2023-07-14 12:52:59)
>> [...]
>>> Maybe add $(SRCDIR)/s390x to INCLUDE_PATHS in the s390x/Makefile ?
>>
>> Yeah, that would work, but do we want that? I'd assume that it is a
>> concious decision not to have tests depend on one another.
> 
> IMHO this would still be OK ... Janosch, Claudio, what's your opinion on this?

And the headers are then ONLY available via snippets/* ?
Pardon my question, not enough cycles, too much work.

Re: [kvm-unit-tests PATCH v5 6/6] s390x: add a test for SIE without MSO/MSL

[Thomas Huth]

On 15/08/2023 13.30, Janosch Frank wrote:
> On 8/14/23 16:59, Thomas Huth wrote:
>> On 01/08/2023 08.51, Nico Boehr wrote:
>>> Quoting Thomas Huth (2023-07-14 12:52:59)
>>> [...]
>>>> Maybe add $(SRCDIR)/s390x to INCLUDE_PATHS in the s390x/Makefile ?
>>>
>>> Yeah, that would work, but do we want that? I'd assume that it is a
>>> concious decision not to have tests depend on one another.
>>
>> IMHO this would still be OK ... Janosch, Claudio, what's your opinion on 
>> this?
> 
> And the headers are then ONLY available via snippets/* ?
> Pardon my question, not enough cycles, too much work.

No, it's about being able to #include "snippets/c/sie-dat.h" from 
s390x/sie-dat.c, so that guest and host code can share some #defines.

  Thomas

Re: [kvm-unit-tests PATCH v5 6/6] s390x: add a test for SIE without MSO/MSL

[Janosch Frank]

On 8/15/23 16:07, Thomas Huth wrote:
> On 15/08/2023 13.30, Janosch Frank wrote:
>> On 8/14/23 16:59, Thomas Huth wrote:
>>> On 01/08/2023 08.51, Nico Boehr wrote:
>>>> Quoting Thomas Huth (2023-07-14 12:52:59)
>>>> [...]
>>>>> Maybe add $(SRCDIR)/s390x to INCLUDE_PATHS in the s390x/Makefile ?
>>>>
>>>> Yeah, that would work, but do we want that? I'd assume that it is a
>>>> concious decision not to have tests depend on one another.
>>>
>>> IMHO this would still be OK ... Janosch, Claudio, what's your opinion on
>>> this?
>>
>> And the headers are then ONLY available via snippets/* ?
>> Pardon my question, not enough cycles, too much work.
> 
> No, it's about being able to #include "snippets/c/sie-dat.h" from
> s390x/sie-dat.c, so that guest and host code can share some #defines.
> 
>    Thomas
> 


I'm fine with that.