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

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

[Nico Boehr]

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   |  26 +++++++-
 lib/s390x/asm/interrupt.h  |  18 ++++++
 lib/s390x/asm/mem.h        |   1 +
 lib/s390x/interrupt.c      |  36 +++++++++++
 lib/s390x/mmu.c            |   5 +-
 lib/s390x/sie.c            |  22 ++++++-
 s390x/Makefile             |   2 +
 s390x/selftest.c           |  40 +++++++++++++
 s390x/sie-dat.c            | 120 +++++++++++++++++++++++++++++++++++++
 s390x/snippets/c/sie-dat.c |  58 ++++++++++++++++++
 s390x/unittests.cfg        |   3 +
 11 files changed, 327 insertions(+), 4 deletions(-)
 create mode 100644 s390x/sie-dat.c
 create mode 100644 s390x/snippets/c/sie-dat.c

-- 
2.39.1

[kvm-unit-tests PATCH v3 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 | 25 ++++++++++++++++++++++++-
 s390x/selftest.c         | 40 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 64 insertions(+), 1 deletion(-)

diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
index bb26e008cc68..84f6996c4d8c 100644
--- a/lib/s390x/asm/arch_def.h
+++ b/lib/s390x/asm/arch_def.h
@@ -37,12 +37,35 @@ 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;
 };
 
 #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.39.1

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

[Janosch Frank]

On 6/1/23 09:01, 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 | 25 ++++++++++++++++++++++++-
>   s390x/selftest.c         | 40 ++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 64 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
> index bb26e008cc68..84f6996c4d8c 100644
> --- a/lib/s390x/asm/arch_def.h
> +++ b/lib/s390x/asm/arch_def.h
> @@ -37,12 +37,35 @@ 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;

Hrm, since I already made the mistake of introducing bitfields with and 
without spaces between the ":" I'm in no position to complain here.

I'm also not sure what the consensus is.

> +		};
> +	};
>   	uint64_t	addr;
>   };

I've come to like static asserts for huge structs and bitfields since 
they can safe you from a *lot* of headaches.

>   
>   #define PSW(m, a) ((struct psw){ .mask = (m), .addr = (uint64_t)(a) })
>   
> +

Whitespace damage

>   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;

We're using lowercase chars for hex constants

> +	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 v3 1/6] lib: s390x: introduce bitfield for PSW mask

[Claudio Imbrenda]

On Thu, 1 Jun 2023 09:42:48 +0200
Janosch Frank <frankja@linux.ibm.com> wrote:

[...]

> Hrm, since I already made the mistake of introducing bitfields with and 
> without spaces between the ":" I'm in no position to complain here.
> 
> I'm also not sure what the consensus is.

tbh I don't really have an opinion, I don't mind either, to the point
that I don't even care if we mix them

> 
> > +		};
> > +	};
> >   	uint64_t	addr;
> >   };  
> 
> I've come to like static asserts for huge structs and bitfields since 
> they can safe you from a *lot* of headaches.

you mean statically asserting that the size is what it should be?
in that case fully agree

[...]

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

[Janosch Frank]

On 6/5/23 12:35, Claudio Imbrenda wrote:
> On Thu, 1 Jun 2023 09:42:48 +0200
> Janosch Frank <frankja@linux.ibm.com> wrote:
> 
> [...]
> 
>> Hrm, since I already made the mistake of introducing bitfields with and
>> without spaces between the ":" I'm in no position to complain here.
>>
>> I'm also not sure what the consensus is.
> 
> tbh I don't really have an opinion, I don't mind either, to the point
> that I don't even care if we mix them
> 
>>
>>> +		};
>>> +	};
>>>    	uint64_t	addr;
>>>    };
>>
>> I've come to like static asserts for huge structs and bitfields since
>> they can safe you from a *lot* of headaches.
> 
> you mean statically asserting that the size is what it should be?
> in that case fully agree
> 

Yes, asserting the size.

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

[Nico Boehr]

Quoting Janosch Frank (2023-06-01 09:42:48)
[...]
> I've come to like static asserts for huge structs and bitfields since 
> they can safe you from a *lot* of headaches.

I generally agree and I add a _Static_assert but I want to mention the
usefulness is a bit limited in this case, since we have a bitfield inside a
union. So it only really helps if you manage to exceed the size of mask.

There really is no way around the stuff I put in the selftests.

I could of course try to make that code _Static_asserts but it will not be
pretty.

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

[Claudio Imbrenda]

On Wed, 07 Jun 2023 17:56:13 +0200
Nico Boehr <nrb@linux.ibm.com> wrote:

> Quoting Janosch Frank (2023-06-01 09:42:48)
> [...]
> > I've come to like static asserts for huge structs and bitfields since 
> > they can safe you from a *lot* of headaches.  
> 
> I generally agree and I add a _Static_assert but I want to mention the
> usefulness is a bit limited in this case, since we have a bitfield inside a
> union. So it only really helps if you manage to exceed the size of mask.

better than nothing :)

if the struct becomes too big, the assert will catch it

> 
> There really is no way around the stuff I put in the selftests.
> 
> I could of course try to make that code _Static_asserts but it will not be
> pretty.

I think just a couple of asserts to make sure things aren't __too__
crazy would be good enough

[kvm-unit-tests PATCH v3 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     | 35 +++++++++++++++++++++++++++++++++++
 lib/s390x/mmu.c           |  5 +++--
 3 files changed, 42 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..d97b5a3a7e97 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,40 @@ 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),
+		NULL /* sentinel */
+	};
+
+	assert(as == AS_PRIM || as == AS_ACCR || as == AS_SECN || as == AS_HOME);
+
+	for (struct psw *psw = irq_psws[0]; psw != NULL; psw++) {
+		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.39.1

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

[Janosch Frank]

On 6/1/23 09:01, Nico Boehr wrote:
> When toggling DAT or switch address space modes, it is likely that

s/switch/switching/

> 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     | 35 +++++++++++++++++++++++++++++++++++
>   lib/s390x/mmu.c           |  5 +++--
>   3 files changed, 42 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..d97b5a3a7e97 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,40 @@ 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/

That seems to be a problematic word.

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

Same here.

> + *
> + * @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),
> +		NULL /* sentinel */
> +	};
> +
> +	assert(as == AS_PRIM || as == AS_ACCR || as == AS_SECN || as == AS_HOME);

/* There are only 4 spaces */
assert(as < 4); ?

> +
> +	for (struct psw *psw = irq_psws[0]; psw != NULL; psw++) {

While this is ok in gnu99/c99 I generally prefer declaring the variable 
outside of the loop since it's more readable when using structs and unions.

> +		psw->dat = dat;
> +		if (dat)
> +			psw->as = as;

Does that check even matter?

> +	}
> +}
> +
>   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 v3 2/6] s390x: add function to set DAT mode for all interrupts

[Nico Boehr]

Quoting Janosch Frank (2023-06-05 10:42:40)
> On 6/1/23 09:01, Nico Boehr wrote:
> > When toggling DAT or switch address space modes, it is likely that
> 
> s/switch/switching/

Done 

[...]
> > diff --git a/lib/s390x/interrupt.c b/lib/s390x/interrupt.c
> > index 3f993a363ae2..d97b5a3a7e97 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,40 @@ 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/

fixed

> > +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),
> > +             NULL /* sentinel */
> > +     };
> > +
> > +     assert(as == AS_PRIM || as == AS_ACCR || as == AS_SECN || as == AS_HOME);
> 
> /* There are only 4 spaces */
> assert(as < 4); ?

Well you won't find that when when grepping for any of the AS_*, so no I don't
think that's better.

> > +
> > +     for (struct psw *psw = irq_psws[0]; psw != NULL; psw++) {
> 
> While this is ok in gnu99/c99 I generally prefer declaring the variable 
> outside of the loop since it's more readable when using structs and unions.

OK.

> 
> > +             psw->dat = dat;
> > +             if (dat)
> > +                     psw->as = as;
> 
> Does that check even matter?

Well, when DAT is off, AS bacially becomes a don't care. I thought it's a good
idea to preserve the AS in this case, but this can certainly be argued about.

I updated the comment and clarified that AS is only touched if DAT is on. Let me
know if you prefer removal of the if.

[kvm-unit-tests PATCH v3 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 84f6996c4d8c..099289e7550e 100644
--- a/lib/s390x/asm/arch_def.h
+++ b/lib/s390x/asm/arch_def.h
@@ -90,6 +90,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.39.1

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

[Janosch Frank]

On 6/1/23 09:01, Nico Boehr wrote:
> This is to prepare for running guests without MSO/MSL, which is
> currently not possible.

This is a preparation patch for non-MSO/MSL guest support.

> 
> 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.

The important information that's missing here is that SIE always uses 
the prim space for guest to host translations.

> 
> 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.

And that's why this is an issue.

For the time being we'll copy the primary ASCE into the home space ASCE. 
No functional change is intended. But if a test intends to....

> 
> 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 84f6996c4d8c..099289e7550e 100644
> --- a/lib/s390x/asm/arch_def.h
> +++ b/lib/s390x/asm/arch_def.h
> @@ -90,6 +90,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);

I'm wondering why this needs to be two calls when you clearly want to 
have a convenience function that does a full space change.

We could introduce:

#define AS_REAL 4

static void mmu_set_addr_space(uint8_t space)
{
	if (space < 4)
		psw_mask_set_bits(space);
	irq_set_dat_mode(space);
}

The "addr" in the function name is optional in my opinion.
@Claudio: What's your opinion?

> +
>   	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));

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

[Nico Boehr]

Quoting Janosch Frank (2023-06-05 11:03:19)
> On 6/1/23 09:01, Nico Boehr wrote:
> > This is to prepare for running guests without MSO/MSL, which is
> > currently not possible.
> 
> This is a preparation patch for non-MSO/MSL guest support.
> 
> > 
> > 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.
> 
> The important information that's missing here is that SIE always uses 
> the prim space for guest to host translations.
> 
> > 
> > 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.
> 
> And that's why this is an issue.
> 
> For the time being we'll copy the primary ASCE into the home space ASCE. 
> No functional change is intended. But if a test intends to....

Thanks, I have to following text now:

 s390x: sie: switch to home space mode before entering SIE

 This is a preparation patch for running guests without MSO/MSL.

 We already have code in sie64a to setup a guest primary ASCE before
 entering SIE. Since SIE always uses the primary ASCE to translate gpa to
 hpa we can in theory switch to seperate guest page tables.

 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. That's why running the host in primary space is an issue.

 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.

 For the time being the primary ASCE is copied to the home space ASCE.
 No functional change is intended, but if a test intends to use a
 different primary space ASCE, it can now just set the guest.asce in the
 save_area.

> > 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)
> >   {
[...]
> > +     /* 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);
> 
> I'm wondering why this needs to be two calls when you clearly want to 
> have a convenience function that does a full space change.
> 
> We could introduce:
> 
> #define AS_REAL 4
> 
> static void mmu_set_addr_space(uint8_t space)
> {
>         if (space < 4)
>                 psw_mask_set_bits(space);
>         irq_set_dat_mode(space);
> }
> 
> The "addr" in the function name is optional in my opinion.
> @Claudio: What's your opinion?

I don't agree; this convenience function would just hide your intention.

With the code right now it is perfectly clear that we're running in home and
handling all interruptions in home. It is perfectly valid to handle
interruptions in a space different from the one you're currently running in;
I guess that's what every OS does for userspace ;-)

[kvm-unit-tests PATCH v3 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 d97b5a3a7e97..3f07068877ee 100644
--- a/lib/s390x/interrupt.c
+++ b/lib/s390x/interrupt.c
@@ -145,6 +145,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.39.1

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

[Janosch Frank]

On 6/1/23 09:02, Nico Boehr wrote:
> 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 d97b5a3a7e97..3f07068877ee 100644
> --- a/lib/s390x/interrupt.c
> +++ b/lib/s390x/interrupt.c
> @@ -145,6 +145,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) {

Reviewed-by: Janosch Frank <frankja@linux.ibm.com>

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

[Claudio Imbrenda]

On Thu,  1 Jun 2023 09:02:00 +0200
Nico Boehr <nrb@linux.ibm.com> wrote:

> 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>

Reviewed-by: Claudio Imbrenda <imbrenda@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 d97b5a3a7e97..3f07068877ee 100644
> --- a/lib/s390x/interrupt.c
> +++ b/lib/s390x/interrupt.c
> @@ -145,6 +145,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) {

[kvm-unit-tests PATCH v3 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..fb4283a40a1b 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 occured.
+ */
+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.39.1

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

[Janosch Frank]

On 6/1/23 09:02, 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.

That's the reason why we set an invalid PGM PSW new for the assembly 
snippets. Seems like I didn't add it for C snippets for some reason -_-

This code is fine but it doesn't fully fix the usability aspect and 
leaves a few questions open:
  - Do we want to stick to the code 8 handling?
  - Do we want to assert like with validities and PGMs outside of SIE?
  - Should sie() have a int return code like in KVM?

> 
> 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..fb4283a40a1b 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.

All of the other functions are in the c file.

> + *
> + * This is similar to clear_pgm_int(), except that it doesn't clear the
> + * interruption information from lowcore.
> + *
> + * Returns 0 when none occured.

s/r/rr/

> + */
> +static inline uint16_t read_pgm_int_code(void)
> +{

No mb()?

> +	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);
>   	}

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

[Claudio Imbrenda]

On Mon, 5 Jun 2023 11:30:36 +0200
Janosch Frank <frankja@linux.ibm.com> wrote:

[,,,]

> > + */
> > +static inline uint16_t read_pgm_int_code(void)
> > +{  
> 
> No mb()?

is one needed there?

> 
> > +	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);
> >   	}  
> 

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

[Nico Boehr]

Quoting Janosch Frank (2023-06-05 11:30:36)
> On 6/1/23 09:02, 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.
> 
> That's the reason why we set an invalid PGM PSW new for the assembly 
> snippets. Seems like I didn't add it for C snippets for some reason -_-

True, C snippets should have a invalid PGM new PSW too. Let me have a try after
my holiday... *writes TODO*

> This code is fine but it doesn't fully fix the usability aspect and 
> leaves a few questions open:
>   - Do we want to stick to the code 8 handling?

Well, I think we need to distinguish between two kinds of PGMs:
- PGMs in the guest,
- PGMs caused by SIE on the host (e.g. because the gpa-hpa mapping is not
  present)

The first case is out of scope for this patch, but certainly something which can
be improved.

This patch focuses on the latter case, where code 8 handling is irrelevant since
the PGM is always delivered to the host.

>   - Do we want to assert like with validities and PGMs outside of SIE?

Well, we would assert() except if we have an expect_pgm_int(), which we do have
:)

>   - Should sie() have a int return code like in KVM?

That's a lovely idea, maybe we should have this at some point in the future, but
I suppose it would require reshuffling large parts of the tests, so please
excuse me if I don't want to do this in the context of this series :) *writes
another TODO*

[...]
> > Also add missing include of facility.h to mem.h.
> 
> ?

That explains why we're including facility.h in mem.h below.

[...]
> > diff --git a/lib/s390x/asm/interrupt.h b/lib/s390x/asm/interrupt.h
> > index 55759002dce2..fb4283a40a1b 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.
> 
> All of the other functions are in the c file.

Claudio requested this to be in the C file, I really don't mind much. Claudio,
maybe you can elaborate why you wanted it in the header.

> > + *
> > + * This is similar to clear_pgm_int(), except that it doesn't clear the
> > + * interruption information from lowcore.
> > + *
> > + * Returns 0 when none occured.
> 
> s/r/rr/

Fixed.

> > + */
> > +static inline uint16_t read_pgm_int_code(void)
> > +{
> 
> No mb()?

This is a function call, so none should be needed, no?

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

[Janosch Frank]

On 6/30/23 16:59, Nico Boehr wrote:
> Quoting Janosch Frank (2023-06-05 11:30:36)
>> On 6/1/23 09:02, 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.
>>
>> That's the reason why we set an invalid PGM PSW new for the assembly
>> snippets. Seems like I didn't add it for C snippets for some reason -_-
> 
> True, C snippets should have a invalid PGM new PSW too. Let me have a try after
> my holiday... *writes TODO*
> 
>> This code is fine but it doesn't fully fix the usability aspect and
>> leaves a few questions open:
>>    - Do we want to stick to the code 8 handling?
> 
> Well, I think we need to distinguish between two kinds of PGMs:
> - PGMs in the guest,
> - PGMs caused by SIE on the host (e.g. because the gpa-hpa mapping is not
>    present)
> 
> The first case is out of scope for this patch, but certainly something which can
> be improved.
> 
> This patch focuses on the latter case, where code 8 handling is irrelevant since
> the PGM is always delivered to the host.

Seems like I wasn't fully awake and/or distracted by hunger when I was 
reading this patch -_-

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

[Claudio Imbrenda]

On Fri, 30 Jun 2023 16:59:06 +0200
Nico Boehr <nrb@linux.ibm.com> wrote:

> [...]
> > > diff --git a/lib/s390x/asm/interrupt.h b/lib/s390x/asm/interrupt.h
> > > index 55759002dce2..fb4283a40a1b 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.  
> > 
> > All of the other functions are in the c file.  
> 
> Claudio requested this to be in the C file, I really don't mind much. Claudio,

you meant header

> maybe you can elaborate why you wanted it in the header.

so it can be inlined it's literally just a read... you can put it in
the C file if you want, but it seems a waste to me tbh 

> 
> > > + *
> > > + * This is similar to clear_pgm_int(), except that it doesn't clear the
> > > + * interruption information from lowcore.
> > > + *
> > > + * Returns 0 when none occured.  
> > 
> > s/r/rr/  
> 
> Fixed.
> 
> > > + */
> > > +static inline uint16_t read_pgm_int_code(void)
> > > +{  
> > 
> > No mb()?  
> 
> This is a function call, so none should be needed, no?

[kvm-unit-tests PATCH v3 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            | 120 +++++++++++++++++++++++++++++++++++++
 s390x/snippets/c/sie-dat.c |  58 ++++++++++++++++++
 s390x/unittests.cfg        |   3 +
 4 files changed, 183 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..c490a2aa825c
--- /dev/null
+++ b/s390x/sie-dat.c
@@ -0,0 +1,120 @@
+/* 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/asm-offsets.h>
+#include <asm-generic/barrier.h>
+#include <asm/pgtable.h>
+#include <mmu.h>
+#include <asm/page.h>
+#include <asm/facility.h>
+#include <asm/interrupt.h>
+#include <asm/mem.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)
+{
+	uint8_t r1;
+	bool contents_match;
+	uint64_t test_page_gpa, test_page_hpa;
+	uint8_t *test_page_hva;
+
+	/* 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);
+	report(vm.sblk->icptcode == ICPT_INST &&
+	       (vm.sblk->ipa & 0xFF00) == 0x8300 && vm.sblk->ipb == 0x9c0000,
+	       "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);
+	report(vm.sblk->icptcode == ICPT_INST &&
+	       vm.sblk->ipa == 0x8300 && vm.sblk->ipb == 0x440000,
+	       "guest wrote to test buffer");
+
+	contents_match = true;
+	for (unsigned int i = 0; i < GUEST_TEST_PAGE_COUNT; i++) {
+		uint8_t 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..e156d0c36c4c
--- /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)
+	);
+}
+
+__attribute__((section(".text"))) 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.39.1

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

[Janosch Frank]

On 6/1/23 09:02, 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            | 120 +++++++++++++++++++++++++++++++++++++
>   s390x/snippets/c/sie-dat.c |  58 ++++++++++++++++++
>   s390x/unittests.cfg        |   3 +
>   4 files changed, 183 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..c490a2aa825c
> --- /dev/null
> +++ b/s390x/sie-dat.c
> @@ -0,0 +1,120 @@
> +/* 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/asm-offsets.h>

I only did a cursory glance and wasn't able to see a use for this include.

> +#include <asm-generic/barrier.h>
> +#include <asm/pgtable.h>
> +#include <mmu.h>
> +#include <asm/page.h>
> +#include <asm/facility.h>

The sclp.h include should be enough, no?
You're not using test_facility() as far as I can see.

> +#include <asm/interrupt.h>
> +#include <asm/mem.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)
> +{
> +	uint8_t r1;
> +	bool contents_match;
> +	uint64_t test_page_gpa, test_page_hpa;
> +	uint8_t *test_page_hva;
> +
> +	/* 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);
> +	report(vm.sblk->icptcode == ICPT_INST &&
> +	       (vm.sblk->ipa & 0xFF00) == 0x8300 && vm.sblk->ipb == 0x9c0000,
> +	       "test buffer gpa=0x%lx hva=%p", test_page_gpa, test_page_hva);

You could rebase on my pv_icptdata.h patch.
Also the report string and boolean don't really relate to each other.

Not every exit needs to be a report.
Some should rather be asserts() or report_info()s.

> +
> +	/* guest will now write to the test buffer and we verify the contents */
> +	sie(&vm);
> +	report(vm.sblk->icptcode == ICPT_INST &&
> +	       vm.sblk->ipa == 0x8300 && vm.sblk->ipb == 0x440000,
> +	       "guest wrote to test buffer");

Yup pv_icptdata.h

> +
> +	contents_match = true;
> +	for (unsigned int i = 0; i < GUEST_TEST_PAGE_COUNT; i++) {
> +		uint8_t expected_val = 42 + i;

Just because you can doesn't mean that you have to.
At least leave a \n when declaring new variables...

> +		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();
> +}
> +

[...]

> +}
> diff --git a/s390x/snippets/c/sie-dat.c b/s390x/snippets/c/sie-dat.c
> new file mode 100644
> index 000000000000..e156d0c36c4c
> --- /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)
> +	);
> +}

It feels like these need to go into a snippet lib.

> +
> +__attribute__((section(".text"))) int main(void)

The attribute shouldn't be needed anymore.

> +{
> +	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++)

Why is i a size_t type?

> +		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

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

[Nico Boehr]

Quoting Janosch Frank (2023-06-05 11:57:58)
[...]
> > diff --git a/s390x/sie-dat.c b/s390x/sie-dat.c
> > new file mode 100644
> > index 000000000000..c490a2aa825c
[...]
> > +#include <libcflat.h>
> > +#include <vmalloc.h>
> > +#include <asm/asm-offsets.h>
> 
> I only did a cursory glance and wasn't able to see a use for this include.

Yep, thanks, I cleaned up the includes a bit.

[...]
> > +static void test_sie_dat(void)
> > +{
> > +     uint8_t r1;
> > +     bool contents_match;
> > +     uint64_t test_page_gpa, test_page_hpa;
> > +     uint8_t *test_page_hva;
> > +
> > +     /* 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);
> > +     report(vm.sblk->icptcode == ICPT_INST &&
> > +            (vm.sblk->ipa & 0xFF00) == 0x8300 && vm.sblk->ipb == 0x9c0000,
> > +            "test buffer gpa=0x%lx hva=%p", test_page_gpa, test_page_hva);
> 
> You could rebase on my pv_icptdata.h patch.
> Also the report string and boolean don't really relate to each other.

Which patch are we talking about? pv_icptdata_check_diag()?

Note that this is not a PV test, so I guess it's not applicable here?

> Not every exit needs to be a report.
> Some should rather be asserts() or report_info()s.

Yeah, I have made report()s where it doesn't make sense to continue assert()s

> > +     contents_match = true;
> > +     for (unsigned int i = 0; i < GUEST_TEST_PAGE_COUNT; i++) {
> > +             uint8_t expected_val = 42 + i;
> 
> Just because you can doesn't mean that you have to.
> At least leave a \n when declaring new variables...

I am a bit confused but I *guess* you wanted me to move the declaration of
expected_val to the beginning of the function?

[...]
> > diff --git a/s390x/snippets/c/sie-dat.c b/s390x/snippets/c/sie-dat.c
> > new file mode 100644
> > index 000000000000..e156d0c36c4c
> > --- /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)
> > +     );
> > +}
> 
> It feels like these need to go into a snippet lib.

A bunch of other tests do similar things, so I'll write a TODO and tackle it in
a seperate series.

[...]
> > +
> > +__attribute__((section(".text"))) int main(void)
> 
> The attribute shouldn't be needed anymore.

OK, removed.

[...]
> > +{
> > +     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++)
> 
> Why is i a size_t type?

Because it's a suitable unsigned type for use as an array index.

What should it be instead?

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

[Janosch Frank]

On 7/10/23 16:29, Nico Boehr wrote:
> Quoting Janosch Frank (2023-06-05 11:57:58)
> [...]
>>> diff --git a/s390x/sie-dat.c b/s390x/sie-dat.c
>>> new file mode 100644
>>> index 000000000000..c490a2aa825c
> [...]
>>> +#include <libcflat.h>
>>> +#include <vmalloc.h>
>>> +#include <asm/asm-offsets.h>
>>
>> I only did a cursory glance and wasn't able to see a use for this include.
> 
> Yep, thanks, I cleaned up the includes a bit.
> 
> [...]
>>> +static void test_sie_dat(void)
>>> +{
>>> +     uint8_t r1;
>>> +     bool contents_match;
>>> +     uint64_t test_page_gpa, test_page_hpa;
>>> +     uint8_t *test_page_hva;
>>> +
>>> +     /* 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);
>>> +     report(vm.sblk->icptcode == ICPT_INST &&
>>> +            (vm.sblk->ipa & 0xFF00) == 0x8300 && vm.sblk->ipb == 0x9c0000,
>>> +            "test buffer gpa=0x%lx hva=%p", test_page_gpa, test_page_hva);
>>
>> You could rebase on my pv_icptdata.h patch.
>> Also the report string and boolean don't really relate to each other.
> 
> Which patch are we talking about? pv_icptdata_check_diag()?
> 
> Note that this is not a PV test, so I guess it's not applicable here?

Ah right, we could extend that but for one use this should be fine.
Let's see if there'll be more SIE tests that need this before building a 
a full intercept check lib.

Could you lower-case the 0xFF00 when you re-spin?

> 
>> Not every exit needs to be a report.
>> Some should rather be asserts() or report_info()s.
> 
> Yeah, I have made report()s where it doesn't make sense to continue assert()s
> 
>>> +     contents_match = true;
>>> +     for (unsigned int i = 0; i < GUEST_TEST_PAGE_COUNT; i++) {
>>> +             uint8_t expected_val = 42 + i;
>>
>> Just because you can doesn't mean that you have to.
>> At least leave a \n when declaring new variables...
> 
> I am a bit confused but I *guess* you wanted me to move the declaration of
> expected_val to the beginning of the function?
> 

Personally I'm not a big fan of declaring variables in the lower 
function body, they are way too easy to overlook.

I dimly remember there being a rule but when I used a few minutes to 
look for it I couldn't find it anymore. Hmmmm, maybe I'm getting old.

> [...]
>>> diff --git a/s390x/snippets/c/sie-dat.c b/s390x/snippets/c/sie-dat.c
>>> new file mode 100644
>>> index 000000000000..e156d0c36c4c
>>> --- /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)
>>> +     );
>>> +}
>>
>> It feels like these need to go into a snippet lib.
> 
> A bunch of other tests do similar things, so I'll write a TODO and tackle it in
> a seperate series.

Thanks :)

> 
> [...]
>>> +
>>> +__attribute__((section(".text"))) int main(void)
>>
>> The attribute shouldn't be needed anymore.
> 
> OK, removed.
> 
> [...]
>>> +{
>>> +     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++)
>>
>> Why is i a size_t type?
> 
> Because it's a suitable unsigned type for use as an array index.
> 
> What should it be instead?

I would have used a standard uint type but to be fair this doesn't kill 
me either.