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

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

[Nico Boehr]

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):
  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: fix compile of interrupt.c
  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   |   1 +
 lib/s390x/asm/interrupt.h  |   5 ++
 lib/s390x/asm/mem.h        |   1 +
 lib/s390x/interrupt.c      |  54 +++++++++++++++++
 lib/s390x/mmu.c            |   5 +-
 lib/s390x/sie.c            |  22 ++++++-
 s390x/Makefile             |   2 +
 s390x/sie-dat.c            | 120 +++++++++++++++++++++++++++++++++++++
 s390x/snippets/c/sie-dat.c |  58 ++++++++++++++++++
 s390x/unittests.cfg        |   3 +
 10 files changed, 268 insertions(+), 3 deletions(-)
 create mode 100644 s390x/sie-dat.c
 create mode 100644 s390x/snippets/c/sie-dat.c

-- 
2.39.1

[kvm-unit-tests PATCH v2 1/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     | 38 ++++++++++++++++++++++++++++++++++++++
 lib/s390x/mmu.c           |  5 +++--
 3 files changed, 45 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..1180ec44d72f 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,43 @@ 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 *irq_psw = irq_psws[0]; irq_psw != NULL; irq_psw++) {
+		if (!dat)
+			irq_psw->mask &= ~PSW_MASK_DAT;
+		else
+			irq_psw->mask |= PSW_MASK_DAT | as << (63 - 16);
+	}
+
+	mb();
+}
+
 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 v2 1/6] s390x: add function to set DAT mode for all interrupts

[Claudio Imbrenda]

On Tue, 16 May 2023 15:04:51 +0200
Nico Boehr <nrb@linux.ibm.com> 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     | 38 ++++++++++++++++++++++++++++++++++++++
>  lib/s390x/mmu.c           |  5 +++--
>  3 files changed, 45 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..1180ec44d72f 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,43 @@ 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 *irq_psw = irq_psws[0]; irq_psw != NULL; irq_psw++) {

just call it psw, or cur_psw, it's a little confusing otherwise

add in arch_def.c:

#define PSW_MASK_AS 0x0000C00000000000UL

> +		if (!dat)
> +			irq_psw->mask &= ~PSW_MASK_DAT;

cur_psw->mask &= ~(PSW_MASK_DAT | PSW_MASK_AS);
if (dat)
	cur_psw->mask |=  PSW_MASK_DAT | BIT_ULL(63 - 16);


alternatively, you can redefine psw with a bitfield (as you mentioned
offline):

cur_psw->mask.dat = dat;
if (dat)
	cur_psw->mask.as = as;

> +		else
> +			irq_psw->mask |= PSW_MASK_DAT | as << (63 - 16);

otherwise here you're ORing stuff to other stuff, if you had 3 and you
OR 0 you get 3, but you actually want 0

> +	}
> +
> +	mb();

what's the purpose of this?

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

[Nico Boehr]

Quoting Claudio Imbrenda (2023-05-16 19:17:24)
[...]
> > diff --git a/lib/s390x/interrupt.c b/lib/s390x/interrupt.c
> > index 3f993a363ae2..1180ec44d72f 100644
> > --- a/lib/s390x/interrupt.c
> > +++ b/lib/s390x/interrupt.c
[...]
> > +void irq_set_dat_mode(bool dat, uint64_t as)
> > +{
[...]
> > +     for (struct psw *irq_psw = irq_psws[0]; irq_psw != NULL; irq_psw++) {
> 
> just call it psw, or cur_psw, it's a little confusing otherwise

will do. 

[...]
> alternatively, you can redefine psw with a bitfield (as you mentioned
> offline):
> 
> cur_psw->mask.dat = dat;
> if (dat)
>         cur_psw->mask.as = as;

Yep, I'll go with that.

> 
> > +             else
> > +                     irq_psw->mask |= PSW_MASK_DAT | as << (63 - 16);
> 
> otherwise here you're ORing stuff to other stuff, if you had 3 and you
> OR 0 you get 3, but you actually want 0

And that's the advantage of the bitfield. :)

> 
> > +     }
> > +
> > +     mb();
> 
> what's the purpose of this?

Make sure that the lowcore really has been written, but I think it's quite
useless, since a function is a sequence point, right?

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

[Claudio Imbrenda]

On Wed, 17 May 2023 14:25:01 +0200
Nico Boehr <nrb@linux.ibm.com> wrote:

> Quoting Claudio Imbrenda (2023-05-16 19:17:24)
> [...]
> > > diff --git a/lib/s390x/interrupt.c b/lib/s390x/interrupt.c
> > > index 3f993a363ae2..1180ec44d72f 100644
> > > --- a/lib/s390x/interrupt.c
> > > +++ b/lib/s390x/interrupt.c  
> [...]
> > > +void irq_set_dat_mode(bool dat, uint64_t as)
> > > +{  
> [...]
> > > +     for (struct psw *irq_psw = irq_psws[0]; irq_psw != NULL; irq_psw++) {  
> > 
> > just call it psw, or cur_psw, it's a little confusing otherwise  
> 
> will do. 
> 
> [...]
> > alternatively, you can redefine psw with a bitfield (as you mentioned
> > offline):
> > 
> > cur_psw->mask.dat = dat;
> > if (dat)
> >         cur_psw->mask.as = as;  
> 
> Yep, I'll go with that.
> 
> >   
> > > +             else
> > > +                     irq_psw->mask |= PSW_MASK_DAT | as << (63 - 16);  
> > 
> > otherwise here you're ORing stuff to other stuff, if you had 3 and you
> > OR 0 you get 3, but you actually want 0  
> 
> And that's the advantage of the bitfield. :)
> 
> >   
> > > +     }
> > > +
> > > +     mb();  
> > 
> > what's the purpose of this?  
> 
> Make sure that the lowcore really has been written, but I think it's quite
> useless, since a function is a sequence point, right?

yes

[kvm-unit-tests PATCH v2 2/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 ++++++++++++++++++
 lib/s390x/sie.h          |  1 +
 3 files changed, 20 insertions(+)

diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
index bb26e008cc68..ea9830a76e6d 100644
--- a/lib/s390x/asm/arch_def.h
+++ b/lib/s390x/asm/arch_def.h
@@ -67,6 +67,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));
diff --git a/lib/s390x/sie.h b/lib/s390x/sie.h
index 147cb0f2a556..0b00fb709776 100644
--- a/lib/s390x/sie.h
+++ b/lib/s390x/sie.h
@@ -284,5 +284,6 @@ void sie_handle_validity(struct vm *vm);
 void sie_guest_sca_create(struct vm *vm);
 void sie_guest_create(struct vm *vm, uint64_t guest_mem, uint64_t guest_mem_len);
 void sie_guest_destroy(struct vm *vm);
+bool sie_had_pgm_int(struct vm *vm);
 
 #endif /* _S390X_SIE_H_ */
-- 
2.39.1

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

[Claudio Imbrenda]

On Tue, 16 May 2023 15:04:52 +0200
Nico Boehr <nrb@linux.ibm.com> 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>
> ---

[...]

> diff --git a/lib/s390x/sie.h b/lib/s390x/sie.h
> index 147cb0f2a556..0b00fb709776 100644
> --- a/lib/s390x/sie.h
> +++ b/lib/s390x/sie.h
> @@ -284,5 +284,6 @@ void sie_handle_validity(struct vm *vm);
>  void sie_guest_sca_create(struct vm *vm);
>  void sie_guest_create(struct vm *vm, uint64_t guest_mem, uint64_t guest_mem_len);
>  void sie_guest_destroy(struct vm *vm);
> +bool sie_had_pgm_int(struct vm *vm);

what's this?

>  
>  #endif /* _S390X_SIE_H_ */

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

[Nico Boehr]

Quoting Claudio Imbrenda (2023-05-16 19:22:25)
[...]
> > diff --git a/lib/s390x/sie.h b/lib/s390x/sie.h
> > index 147cb0f2a556..0b00fb709776 100644
> > --- a/lib/s390x/sie.h
> > +++ b/lib/s390x/sie.h
> > @@ -284,5 +284,6 @@ void sie_handle_validity(struct vm *vm);
> >  void sie_guest_sca_create(struct vm *vm);
> >  void sie_guest_create(struct vm *vm, uint64_t guest_mem, uint64_t guest_mem_len);
> >  void sie_guest_destroy(struct vm *vm);
> > +bool sie_had_pgm_int(struct vm *vm);
> 
> what's this?

Opsie, fixed :)

[kvm-unit-tests PATCH v2 3/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 1180ec44d72f..2e5309cee40f 100644
--- a/lib/s390x/interrupt.c
+++ b/lib/s390x/interrupt.c
@@ -148,6 +148,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

[kvm-unit-tests PATCH v2 4/6] s390x: fix compile of interrupt.c

[Nico Boehr]

A future commit will include interrupt.h from sie.c.

Since interrupt.h includes mem.h, but sie.c does not include facility.h,
this will lead to the following compile error:

In file included from lib/s390x/interrupt.c:10:
/home/nrb/kvm-unit-tests/lib/asm/mem.h: In function ‘set_storage_key_mb’:
/home/nrb/kvm-unit-tests/lib/asm/mem.h:42:16: error: implicit declaration of function ‘test_facility’ [-Werror=implicit-function-declaration]
   42 |         assert(test_facility(8));
         |                ^~~~~~~~~~~~~

Add the missing include in interrupt.h

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

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

Re: [kvm-unit-tests PATCH v2 4/6] s390x: fix compile of interrupt.c

[Claudio Imbrenda]

On Tue, 16 May 2023 15:04:54 +0200
Nico Boehr <nrb@linux.ibm.com> wrote:

> A future commit will include interrupt.h from sie.c.

can you merge this into that future commit that will require it?

> 
> Since interrupt.h includes mem.h, but sie.c does not include facility.h,
> this will lead to the following compile error:
> 
> In file included from lib/s390x/interrupt.c:10:
> /home/nrb/kvm-unit-tests/lib/asm/mem.h: In function ‘set_storage_key_mb’:
> /home/nrb/kvm-unit-tests/lib/asm/mem.h:42:16: error: implicit declaration of function ‘test_facility’ [-Werror=implicit-function-declaration]
>    42 |         assert(test_facility(8));
>          |                ^~~~~~~~~~~~~
> 
> Add the missing include in interrupt.h
> 
> Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
> ---
>  lib/s390x/asm/mem.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> 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)))

[kvm-unit-tests PATCH v2 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().

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

diff --git a/lib/s390x/asm/interrupt.h b/lib/s390x/asm/interrupt.h
index 55759002dce2..2d7eb1907458 100644
--- a/lib/s390x/asm/interrupt.h
+++ b/lib/s390x/asm/interrupt.h
@@ -81,6 +81,7 @@ void handle_svc_int(void);
 void expect_pgm_int(void);
 void expect_ext_int(void);
 uint16_t clear_pgm_int(void);
+uint16_t read_pgm_int_code(void);
 void check_pgm_int_code(uint16_t code);
 
 #define IRQ_DAT_ON	true
diff --git a/lib/s390x/interrupt.c b/lib/s390x/interrupt.c
index 2e5309cee40f..82b4259d433c 100644
--- a/lib/s390x/interrupt.c
+++ b/lib/s390x/interrupt.c
@@ -60,6 +60,21 @@ uint16_t clear_pgm_int(void)
 	return code;
 }
 
+/**
+ * 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.
+ */
+uint16_t read_pgm_int_code(void)
+{
+	mb();
+	return lowcore.pgm_int_code;
+}
+
 /**
  * check_pgm_int_code - Check the program interrupt code on the current CPU.
  * @code the expected program interrupt code on the current CPU
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);
 	}
diff --git a/lib/s390x/sie.h b/lib/s390x/sie.h
index 0b00fb709776..147cb0f2a556 100644
--- a/lib/s390x/sie.h
+++ b/lib/s390x/sie.h
@@ -284,6 +284,5 @@ void sie_handle_validity(struct vm *vm);
 void sie_guest_sca_create(struct vm *vm);
 void sie_guest_create(struct vm *vm, uint64_t guest_mem, uint64_t guest_mem_len);
 void sie_guest_destroy(struct vm *vm);
-bool sie_had_pgm_int(struct vm *vm);
 
 #endif /* _S390X_SIE_H_ */
-- 
2.39.1

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

[Claudio Imbrenda]

On Tue, 16 May 2023 15:04:55 +0200
Nico Boehr <nrb@linux.ibm.com> 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().
> 
> Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
> ---
>  lib/s390x/asm/interrupt.h |  1 +
>  lib/s390x/interrupt.c     | 15 +++++++++++++++
>  lib/s390x/sie.c           |  4 +++-
>  lib/s390x/sie.h           |  1 -
>  4 files changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/s390x/asm/interrupt.h b/lib/s390x/asm/interrupt.h
> index 55759002dce2..2d7eb1907458 100644
> --- a/lib/s390x/asm/interrupt.h
> +++ b/lib/s390x/asm/interrupt.h
> @@ -81,6 +81,7 @@ void handle_svc_int(void);
>  void expect_pgm_int(void);
>  void expect_ext_int(void);
>  uint16_t clear_pgm_int(void);
> +uint16_t read_pgm_int_code(void);
>  void check_pgm_int_code(uint16_t code);
>  
>  #define IRQ_DAT_ON	true
> diff --git a/lib/s390x/interrupt.c b/lib/s390x/interrupt.c
> index 2e5309cee40f..82b4259d433c 100644
> --- a/lib/s390x/interrupt.c
> +++ b/lib/s390x/interrupt.c
> @@ -60,6 +60,21 @@ uint16_t clear_pgm_int(void)
>  	return code;
>  }
>  
> +/**
> + * 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.
> + */
> +uint16_t read_pgm_int_code(void)

could this whole function go in the header as static inline?

> +{
> +	mb();

is the mb really needed?

> +	return lowcore.pgm_int_code;
> +}
> +
>  /**
>   * check_pgm_int_code - Check the program interrupt code on the current CPU.
>   * @code the expected program interrupt code on the current CPU
> 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);
>  	}
> diff --git a/lib/s390x/sie.h b/lib/s390x/sie.h
> index 0b00fb709776..147cb0f2a556 100644
> --- a/lib/s390x/sie.h
> +++ b/lib/s390x/sie.h
> @@ -284,6 +284,5 @@ void sie_handle_validity(struct vm *vm);
>  void sie_guest_sca_create(struct vm *vm);
>  void sie_guest_create(struct vm *vm, uint64_t guest_mem, uint64_t guest_mem_len);
>  void sie_guest_destroy(struct vm *vm);
> -bool sie_had_pgm_int(struct vm *vm);

... ?

>  
>  #endif /* _S390X_SIE_H_ */

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

[Nico Boehr]

Quoting Claudio Imbrenda (2023-05-16 19:30:18)
[...]
> > diff --git a/lib/s390x/interrupt.c b/lib/s390x/interrupt.c
> > index 2e5309cee40f..82b4259d433c 100644
> > --- a/lib/s390x/interrupt.c
> > +++ b/lib/s390x/interrupt.c
[...]
> > +/**
> > + * 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.
> > + */
> > +uint16_t read_pgm_int_code(void)
> 
> could this whole function go in the header as static inline?

Yes, sure, changed.

> > +{
> > +     mb();
> 
> is the mb really needed?

No, I don't think so, since this is a function, I'll remove this.

[kvm-unit-tests PATCH v2 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