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

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

[Nico Boehr]

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 (4):
  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   |   2 +
 lib/s390x/interrupt.c      |   7 +++
 lib/s390x/sie.c            |  36 ++++++++++-
 lib/s390x/sie.h            |   2 +
 s390x/Makefile             |   2 +
 s390x/sie-dat.c            | 121 +++++++++++++++++++++++++++++++++++++
 s390x/snippets/c/sie-dat.c |  58 ++++++++++++++++++
 s390x/unittests.cfg        |   3 +
 8 files changed, 230 insertions(+), 1 deletion(-)
 create mode 100644 s390x/sie-dat.c
 create mode 100644 s390x/snippets/c/sie-dat.c

-- 
2.39.1

[kvm-unit-tests PATCH v1 1/4] 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 |  2 ++
 lib/s390x/sie.c          | 26 ++++++++++++++++++++++++++
 lib/s390x/sie.h          |  1 +
 3 files changed, 29 insertions(+)

diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
index bb26e008cc68..dd95e27abf48 100644
--- a/lib/s390x/asm/arch_def.h
+++ b/lib/s390x/asm/arch_def.h
@@ -67,6 +67,8 @@ struct cpu {
 #define AS_HOME				3
 
 #define PSW_MASK_DAT			0x0400000000000000UL
+#define PSW_MASK_DAT_HOME		0x0400C00000000000UL
+#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..22141ded1a90 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,19 @@ 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 */
+	lowcore.pgm_new_psw.mask |= PSW_MASK_DAT_HOME;
+	lowcore.ext_new_psw.mask |= PSW_MASK_DAT_HOME;
+	lowcore.io_new_psw.mask |= PSW_MASK_DAT_HOME;
+	mb();
+
 	while (vm->sblk->icptcode == 0) {
 		sie64a(vm->sblk, &vm->save_area);
 		sie_handle_validity(vm);
@@ -60,6 +75,17 @@ void sie(struct vm *vm)
 	vm->save_area.guest.grs[14] = vm->sblk->gg14;
 	vm->save_area.guest.grs[15] = vm->sblk->gg15;
 
+	lowcore.pgm_new_psw.mask &= ~PSW_MASK_HOME;
+	lowcore.ext_new_psw.mask &= ~PSW_MASK_HOME;
+	lowcore.io_new_psw.mask &= ~PSW_MASK_HOME;
+	mb();
+
+	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 v1 1/4] s390x: sie: switch to home space mode before entering SIE

[Janosch Frank]

On 3/27/23 10:21, Nico Boehr wrote:
> This is to prepare for running guests without MSO/MSL, which is
> currently not possible.
> 
> We already have code in sie64a to setup a guest primary ASCE before
> entering SIE, so we can in theory switch to the page tables which
> translate gpa to hpa.
> 
> But the host is running in primary space mode already, so changing the
> primary ASCE before entering SIE will also affect the host's code and
> data.
> 
> To make this switch useful, the host should run in a different address
> space mode. Hence, set up and change to home address space mode before
> installing the guest ASCE.
> 
> The home space ASCE is just copied over from the primary space ASCE, so
> no functional change is intended, also for tests that want to use
> MSO/MSL. If a test intends to use a different primary space ASCE, it can
> now just set the guest.asce in the save_area.
> 
[...]
> +	/* 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 */
> +	lowcore.pgm_new_psw.mask |= PSW_MASK_DAT_HOME;

> +	lowcore.ext_new_psw.mask |= PSW_MASK_DAT_HOME;
> +	lowcore.io_new_psw.mask |= PSW_MASK_DAT_HOME;
We didn't enable DAT in these two cases as far as I can see so this is 
superfluous or we should change the mmu code. Also it's missing the svc 
and machine check.

The whole bit manipulation thing looks a bit crude. It might make more 
sense to drop into real mode for a few instructions and have a dedicated 
storage location for an extended PSW mask and an interrupt ASCE as part 
of the interrupt call code instead.

Opinions?

> +	mb();
> +
>   	while (vm->sblk->icptcode == 0) {
>   		sie64a(vm->sblk, &vm->save_area);
>   		sie_handle_validity(vm);
> @@ -60,6 +75,17 @@ void sie(struct vm *vm)
>   	vm->save_area.guest.grs[14] = vm->sblk->gg14;
>   	vm->save_area.guest.grs[15] = vm->sblk->gg15;
>   
> +	lowcore.pgm_new_psw.mask &= ~PSW_MASK_HOME;
> +	lowcore.ext_new_psw.mask &= ~PSW_MASK_HOME;
> +	lowcore.io_new_psw.mask &= ~PSW_MASK_HOME;
> +	mb();
> +
> +	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_ */

Re: [kvm-unit-tests PATCH v1 1/4] s390x: sie: switch to home space mode before entering SIE

[Nico Boehr]

Quoting Janosch Frank (2023-03-28 16:13:04)
> On 3/27/23 10:21, Nico Boehr wrote:
> > This is to prepare for running guests without MSO/MSL, which is
> > currently not possible.
> > 
> > We already have code in sie64a to setup a guest primary ASCE before
> > entering SIE, so we can in theory switch to the page tables which
> > translate gpa to hpa.
> > 
> > But the host is running in primary space mode already, so changing the
> > primary ASCE before entering SIE will also affect the host's code and
> > data.
> > 
> > To make this switch useful, the host should run in a different address
> > space mode. Hence, set up and change to home address space mode before
> > installing the guest ASCE.
> > 
> > The home space ASCE is just copied over from the primary space ASCE, so
> > no functional change is intended, also for tests that want to use
> > MSO/MSL. If a test intends to use a different primary space ASCE, it can
> > now just set the guest.asce in the save_area.
> > 
> [...]
> > +     /* 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 */
> > +     lowcore.pgm_new_psw.mask |= PSW_MASK_DAT_HOME;
> 
> > +     lowcore.ext_new_psw.mask |= PSW_MASK_DAT_HOME;
> > +     lowcore.io_new_psw.mask |= PSW_MASK_DAT_HOME;
> We didn't enable DAT in these two cases as far as I can see so this is 
> superfluous or we should change the mmu code. Also it's missing the svc 
> and machine check.

Right. Is there a particular reason why we only run DAT on for PGM ints?

> The whole bit manipulation thing looks a bit crude. It might make more 
> sense to drop into real mode for a few instructions and have a dedicated 
> storage location for an extended PSW mask and an interrupt ASCE as part 
> of the interrupt call code instead.
> 
> Opinions?

Maybe I don't get it, but I personally don't quite see the advantage. It seems
to me this would make things much more complicated just to avoid a few simple
bitops.

It maybe also depends on how many new_psws we have to touch. If it's really just
the PGM, the current solution seems simple enough.

But if others also prefer Janosch's suggestion, I am happy to implement it.

Re: [kvm-unit-tests PATCH v1 1/4] s390x: sie: switch to home space mode before entering SIE

[Claudio Imbrenda]

On Wed, 29 Mar 2023 14:50:50 +0200
Nico Boehr <nrb@linux.ibm.com> wrote:

> Quoting Janosch Frank (2023-03-28 16:13:04)
> > On 3/27/23 10:21, Nico Boehr wrote:  
> > > This is to prepare for running guests without MSO/MSL, which is
> > > currently not possible.
> > > 
> > > We already have code in sie64a to setup a guest primary ASCE before
> > > entering SIE, so we can in theory switch to the page tables which
> > > translate gpa to hpa.
> > > 
> > > But the host is running in primary space mode already, so changing the
> > > primary ASCE before entering SIE will also affect the host's code and
> > > data.
> > > 
> > > To make this switch useful, the host should run in a different address
> > > space mode. Hence, set up and change to home address space mode before
> > > installing the guest ASCE.
> > > 
> > > The home space ASCE is just copied over from the primary space ASCE, so
> > > no functional change is intended, also for tests that want to use
> > > MSO/MSL. If a test intends to use a different primary space ASCE, it can
> > > now just set the guest.asce in the save_area.
> > >   
> > [...]  
> > > +     /* 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 */
> > > +     lowcore.pgm_new_psw.mask |= PSW_MASK_DAT_HOME;  
> >   
> > > +     lowcore.ext_new_psw.mask |= PSW_MASK_DAT_HOME;
> > > +     lowcore.io_new_psw.mask |= PSW_MASK_DAT_HOME;  
> > We didn't enable DAT in these two cases as far as I can see so this is 
> > superfluous or we should change the mmu code. Also it's missing the svc 
> > and machine check.  
> 
> Right. Is there a particular reason why we only run DAT on for PGM ints?

a fixup handler for PGM it might need to run with DAT on (e.g. to
access data that is not identity mapped), whereas for other interrupts
it's not needed (at least not yet ;) )

> 
> > The whole bit manipulation thing looks a bit crude. It might make more 
> > sense to drop into real mode for a few instructions and have a dedicated 
> > storage location for an extended PSW mask and an interrupt ASCE as part 
> > of the interrupt call code instead.
> > 
> > Opinions?  
> 
> Maybe I don't get it, but I personally don't quite see the advantage. It seems
> to me this would make things much more complicated just to avoid a few simple
> bitops.
> 
> It maybe also depends on how many new_psws we have to touch. If it's really just
> the PGM, the current solution seems simple enough.
> 
> But if others also prefer Janosch's suggestion, I am happy to implement it.

Re: [kvm-unit-tests PATCH v1 1/4] s390x: sie: switch to home space mode before entering SIE

[Janosch Frank]

On 3/29/23 15:00, Claudio Imbrenda wrote:
> On Wed, 29 Mar 2023 14:50:50 +0200
> Nico Boehr <nrb@linux.ibm.com> wrote:
> 
>> Quoting Janosch Frank (2023-03-28 16:13:04)
>>> On 3/27/23 10:21, Nico Boehr wrote:
>>>> This is to prepare for running guests without MSO/MSL, which is
>>>> currently not possible.
>>>>
>>>> We already have code in sie64a to setup a guest primary ASCE before
>>>> entering SIE, so we can in theory switch to the page tables which
>>>> translate gpa to hpa.
>>>>
>>>> But the host is running in primary space mode already, so changing the
>>>> primary ASCE before entering SIE will also affect the host's code and
>>>> data.
>>>>
>>>> To make this switch useful, the host should run in a different address
>>>> space mode. Hence, set up and change to home address space mode before
>>>> installing the guest ASCE.
>>>>
>>>> The home space ASCE is just copied over from the primary space ASCE, so
>>>> no functional change is intended, also for tests that want to use
>>>> MSO/MSL. If a test intends to use a different primary space ASCE, it can
>>>> now just set the guest.asce in the save_area.
>>>>    
>>> [...]
>>>> +     /* 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 */
>>>> +     lowcore.pgm_new_psw.mask |= PSW_MASK_DAT_HOME;
>>>    
>>>> +     lowcore.ext_new_psw.mask |= PSW_MASK_DAT_HOME;
>>>> +     lowcore.io_new_psw.mask |= PSW_MASK_DAT_HOME;
>>> We didn't enable DAT in these two cases as far as I can see so this is
>>> superfluous or we should change the mmu code. Also it's missing the svc
>>> and machine check.
>>
>> Right. Is there a particular reason why we only run DAT on for PGM ints?
> 
> a fixup handler for PGM it might need to run with DAT on (e.g. to
> access data that is not identity mapped), whereas for other interrupts
> it's not needed (at least not yet ;) )

At the time where the mmu code was written, the other handlers were very 
basic or a direct abort (like the IO IRQ that was introduced by Pierre). 
But I'd rather have them all behave the same so they are at least 
consistent.

If we don't introduce a location where we load the PSW from, then add a 
function that sets the masks for all IRQs and also convert the mmu 
enablement to use it.

Something to the likes of:

void irq_new_set_mask(bool dat, uint8_t as)
{
	loop over psws {
		- Remove dat and as bits from new PSW
		- Or in the new dat + as bits
	}
}

Re: [kvm-unit-tests PATCH v1 1/4] s390x: sie: switch to home space mode before entering SIE

[Nico Boehr]

Quoting Claudio Imbrenda (2023-03-29 15:00:32)
> On Wed, 29 Mar 2023 14:50:50 +0200
> Nico Boehr <nrb@linux.ibm.com> wrote:
> 
> > Quoting Janosch Frank (2023-03-28 16:13:04)
> > > On 3/27/23 10:21, Nico Boehr wrote:  
> > > > This is to prepare for running guests without MSO/MSL, which is
> > > > currently not possible.
> > > > 
> > > > We already have code in sie64a to setup a guest primary ASCE before
> > > > entering SIE, so we can in theory switch to the page tables which
> > > > translate gpa to hpa.
> > > > 
> > > > But the host is running in primary space mode already, so changing the
> > > > primary ASCE before entering SIE will also affect the host's code and
> > > > data.
> > > > 
> > > > To make this switch useful, the host should run in a different address
> > > > space mode. Hence, set up and change to home address space mode before
> > > > installing the guest ASCE.
> > > > 
> > > > The home space ASCE is just copied over from the primary space ASCE, so
> > > > no functional change is intended, also for tests that want to use
> > > > MSO/MSL. If a test intends to use a different primary space ASCE, it can
> > > > now just set the guest.asce in the save_area.
> > > >   
> > > [...]  
> > > > +     /* 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 */
> > > > +     lowcore.pgm_new_psw.mask |= PSW_MASK_DAT_HOME;  
> > >   
> > > > +     lowcore.ext_new_psw.mask |= PSW_MASK_DAT_HOME;
> > > > +     lowcore.io_new_psw.mask |= PSW_MASK_DAT_HOME;  
> > > We didn't enable DAT in these two cases as far as I can see so this is 
> > > superfluous or we should change the mmu code. Also it's missing the svc 
> > > and machine check.  
> > 
> > Right. Is there a particular reason why we only run DAT on for PGM ints?
> 
> a fixup handler for PGM it might need to run with DAT on (e.g. to
> access data that is not identity mapped), whereas for other interrupts
> it's not needed (at least not yet ;) )

Makes sense.

Since one can register a cleanup for io and ext, too, I think we need to fix the
mmu init for these cases while at it.

Will do that in v2.

[kvm-unit-tests PATCH v1 2/4] 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 3f993a363ae2..eb3d6a9b701d 100644
--- a/lib/s390x/interrupt.c
+++ b/lib/s390x/interrupt.c
@@ -110,6 +110,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 v1 3/4] 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.

Add a flag PROG_PGM_IN_SIE set by the pgm int fixup which indicates a
program interrupt has occured in SIE. Check for the flag in sie() and if
it's set return from sie() to give the host the ability to react on the
exception. The host may check if a PGM int has occured in the guest
using the new function sie_had_pgm_int().

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

diff --git a/lib/s390x/interrupt.c b/lib/s390x/interrupt.c
index eb3d6a9b701d..9baf7a003f52 100644
--- a/lib/s390x/interrupt.c
+++ b/lib/s390x/interrupt.c
@@ -106,10 +106,16 @@ void register_ext_cleanup_func(void (*f)(struct stack_frame_int *))
 
 static void fixup_pgm_int(struct stack_frame_int *stack)
 {
+	struct kvm_s390_sie_block *sblk;
+
 	/* If we have an error on SIE we directly move to sie_exit */
 	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;
+
+		/* set a marker in sie_block that a PGM int occured */
+		sblk = *((struct kvm_s390_sie_block **)(stack->grs0[13] + __SF_SIE_CONTROL));
+		sblk->prog0c |= PROG_PGM_IN_SIE;
 		return;
 	}
 
diff --git a/lib/s390x/sie.c b/lib/s390x/sie.c
index 22141ded1a90..5e9ae7115c47 100644
--- a/lib/s390x/sie.c
+++ b/lib/s390x/sie.c
@@ -44,6 +44,11 @@ void sie_handle_validity(struct vm *vm)
 	vm->validity_expected = false;
 }
 
+bool sie_had_pgm_int(struct vm *vm)
+{
+	return vm->sblk->prog0c & PROG_PGM_IN_SIE;
+}
+
 void sie(struct vm *vm)
 {
 	uint64_t old_cr13;
@@ -68,7 +73,10 @@ void sie(struct vm *vm)
 	lowcore.io_new_psw.mask |= PSW_MASK_DAT_HOME;
 	mb();
 
-	while (vm->sblk->icptcode == 0) {
+	/* clear PGM int marker, which might still be set */
+	vm->sblk->prog0c &= ~PROG_PGM_IN_SIE;
+
+	while (vm->sblk->icptcode == 0 && !sie_had_pgm_int(vm)) {
 		sie64a(vm->sblk, &vm->save_area);
 		sie_handle_validity(vm);
 	}
diff --git a/lib/s390x/sie.h b/lib/s390x/sie.h
index 0b00fb709776..8ab755dc9456 100644
--- a/lib/s390x/sie.h
+++ b/lib/s390x/sie.h
@@ -37,6 +37,7 @@ struct kvm_s390_sie_block {
 	uint32_t 	ibc : 12;
 	uint8_t		reserved08[4];		/* 0x0008 */
 #define PROG_IN_SIE (1<<0)
+#define PROG_PGM_IN_SIE (1<<1)
 	uint32_t	prog0c;			/* 0x000c */
 union {
 		uint8_t	reserved10[16];		/* 0x0010 */
-- 
2.39.1

Re: [kvm-unit-tests PATCH v1 3/4] s390x: lib: sie: don't reenter SIE on pgm int

[Janosch Frank]

On 3/27/23 10:21, 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.
> 
> Add a flag PROG_PGM_IN_SIE set by the pgm int fixup which indicates a
> program interrupt has occured in SIE. Check for the flag in sie() and if
> it's set return from sie() to give the host the ability to react on the
> exception. The host may check if a PGM int has occured in the guest
> using the new function sie_had_pgm_int().

We could simply check "!lowcore.pgm_int_code" by introducing:
uint16_t read_pgm_int(void)
{
	mb();
	return lowcore.pgm_int_code;
}

into interrupt.c.


Or to be a bit more verbose:
I don't see a reason why we'd want to store a per sblk PGM in SIE bit 
when all we want to know is either: was there a PGM at all (to stop the 
SIE loop) or was there a PGM between the expect and the 
check_pgm_int_code().

Re: [kvm-unit-tests PATCH v1 3/4] s390x: lib: sie: don't reenter SIE on pgm int

[Nico Boehr]

Quoting Janosch Frank (2023-03-28 15:42:26)
> On 3/27/23 10:21, 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.
> > 
> > Add a flag PROG_PGM_IN_SIE set by the pgm int fixup which indicates a
> > program interrupt has occured in SIE. Check for the flag in sie() and if
> > it's set return from sie() to give the host the ability to react on the
> > exception. The host may check if a PGM int has occured in the guest
> > using the new function sie_had_pgm_int().
> 
> We could simply check "!lowcore.pgm_int_code" by introducing:
> uint16_t read_pgm_int(void)
> {
>         mb();
>         return lowcore.pgm_int_code;
> }
> 
> into interrupt.c.
> 
> 
> Or to be a bit more verbose:
> I don't see a reason why we'd want to store a per sblk PGM in SIE bit 
> when all we want to know is either: was there a PGM at all (to stop the 
> SIE loop) or was there a PGM between the expect and the 
> check_pgm_int_code().

Yes, makes perfect sense, I just didn't see this possiblity. Thanks, will change.

Re: [kvm-unit-tests PATCH v1 3/4] s390x: lib: sie: don't reenter SIE on pgm int

[Claudio Imbrenda]

On Mon, 27 Mar 2023 10:21:17 +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.
> 
> Add a flag PROG_PGM_IN_SIE set by the pgm int fixup which indicates a
> program interrupt has occured in SIE. Check for the flag in sie() and if
> it's set return from sie() to give the host the ability to react on the
> exception. The host may check if a PGM int has occured in the guest
> using the new function sie_had_pgm_int().
> 
> Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
> ---
>  lib/s390x/interrupt.c |  6 ++++++
>  lib/s390x/sie.c       | 10 +++++++++-
>  lib/s390x/sie.h       |  1 +
>  3 files changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/s390x/interrupt.c b/lib/s390x/interrupt.c
> index eb3d6a9b701d..9baf7a003f52 100644
> --- a/lib/s390x/interrupt.c
> +++ b/lib/s390x/interrupt.c
> @@ -106,10 +106,16 @@ void register_ext_cleanup_func(void (*f)(struct stack_frame_int *))
>  
>  static void fixup_pgm_int(struct stack_frame_int *stack)
>  {
> +	struct kvm_s390_sie_block *sblk;
> +
>  	/* If we have an error on SIE we directly move to sie_exit */
>  	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;
> +
> +		/* set a marker in sie_block that a PGM int occured */
> +		sblk = *((struct kvm_s390_sie_block **)(stack->grs0[13] + __SF_SIE_CONTROL));
> +		sblk->prog0c |= PROG_PGM_IN_SIE;
>  		return;
>  	}
>  
> diff --git a/lib/s390x/sie.c b/lib/s390x/sie.c
> index 22141ded1a90..5e9ae7115c47 100644
> --- a/lib/s390x/sie.c
> +++ b/lib/s390x/sie.c
> @@ -44,6 +44,11 @@ void sie_handle_validity(struct vm *vm)
>  	vm->validity_expected = false;
>  }
>  
> +bool sie_had_pgm_int(struct vm *vm)
> +{
> +	return vm->sblk->prog0c & PROG_PGM_IN_SIE;
> +}
> +
>  void sie(struct vm *vm)
>  {
>  	uint64_t old_cr13;
> @@ -68,7 +73,10 @@ void sie(struct vm *vm)
>  	lowcore.io_new_psw.mask |= PSW_MASK_DAT_HOME;
>  	mb();
>  
> -	while (vm->sblk->icptcode == 0) {
> +	/* clear PGM int marker, which might still be set */
> +	vm->sblk->prog0c &= ~PROG_PGM_IN_SIE;
> +
> +	while (vm->sblk->icptcode == 0 && !sie_had_pgm_int(vm)) {
>  		sie64a(vm->sblk, &vm->save_area);
>  		sie_handle_validity(vm);
>  	}
> diff --git a/lib/s390x/sie.h b/lib/s390x/sie.h
> index 0b00fb709776..8ab755dc9456 100644
> --- a/lib/s390x/sie.h
> +++ b/lib/s390x/sie.h
> @@ -37,6 +37,7 @@ struct kvm_s390_sie_block {
>  	uint32_t 	ibc : 12;
>  	uint8_t		reserved08[4];		/* 0x0008 */
>  #define PROG_IN_SIE (1<<0)
> +#define PROG_PGM_IN_SIE (1<<1)

please align the body of the macros with tabs, so they are more readable

>  	uint32_t	prog0c;			/* 0x000c */
>  union {
>  		uint8_t	reserved10[16];		/* 0x0010 */

Re: [kvm-unit-tests PATCH v1 3/4] s390x: lib: sie: don't reenter SIE on pgm int

[Nico Boehr]

Quoting Claudio Imbrenda (2023-03-28 19:01:06)
[...]
> > diff --git a/lib/s390x/sie.h b/lib/s390x/sie.h
> > index 0b00fb709776..8ab755dc9456 100644
> > --- a/lib/s390x/sie.h
> > +++ b/lib/s390x/sie.h
> > @@ -37,6 +37,7 @@ struct kvm_s390_sie_block {
> >       uint32_t        ibc : 12;
> >       uint8_t         reserved08[4];          /* 0x0008 */
> >  #define PROG_IN_SIE (1<<0)
> > +#define PROG_PGM_IN_SIE (1<<1)
> 
> please align the body of the macros with tabs, so they are more readable

Thanks, would do, but this is gonna go away anyways in favor of Janoschs suggestion.

[kvm-unit-tests PATCH v1 4/4] 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            | 121 +++++++++++++++++++++++++++++++++++++
 s390x/snippets/c/sie-dat.c |  58 ++++++++++++++++++
 s390x/unittests.cfg        |   3 +
 4 files changed, 184 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 97a616111680..e27650d7811b 100644
--- a/s390x/Makefile
+++ b/s390x/Makefile
@@ -39,6 +39,7 @@ tests += $(TEST_DIR)/panic-loop-extint.elf
 tests += $(TEST_DIR)/panic-loop-pgm.elf
 tests += $(TEST_DIR)/migration-sck.elf
 tests += $(TEST_DIR)/exittime.elf
+tests += $(TEST_DIR)/sie-dat.elf
 
 pv-tests += $(TEST_DIR)/pv-diags.elf
 
@@ -114,6 +115,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..37e46386181c
--- /dev/null
+++ b/s390x/sie-dat.c
@@ -0,0 +1,121 @@
+/* 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 */
+	report_prefix_push("guest write to unmapped");
+	expect_pgm_int();
+	sie(&vm);
+	check_pgm_int_code(PGM_INT_CODE_SEGMENT_TRANSLATION);
+	report(sie_had_pgm_int(&vm), "pgm int occured in sie");
+	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..c9f7af0f3a56
--- /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 d97eb5e943c8..aab0e670f2d4 100644
--- a/s390x/unittests.cfg
+++ b/s390x/unittests.cfg
@@ -215,3 +215,6 @@ file = migration-skey.elf
 smp = 2
 groups = migration
 extra_params = -append '--parallel'
+
+[sie-dat]
+file = sie-dat.elf
-- 
2.39.1

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

[Nina Schoetterl-Glausch]

On Mon, 2023-03-27 at 10:21 +0200, 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            | 121 +++++++++++++++++++++++++++++++++++++
>  s390x/snippets/c/sie-dat.c |  58 ++++++++++++++++++
>  s390x/unittests.cfg        |   3 +
>  4 files changed, 184 insertions(+)
>  create mode 100644 s390x/sie-dat.c
>  create mode 100644 s390x/snippets/c/sie-dat.c
> 

Test looks good to me. Some comments below.
[...]

> diff --git a/s390x/sie-dat.c b/s390x/sie-dat.c
> new file mode 100644
> index 000000000000..37e46386181c
> --- /dev/null
> +++ b/s390x/sie-dat.c
> @@ -0,0 +1,121 @@
> 
[...]
> +
> +/* keep in sync with TOTAL_PAGE_COUNT in s390x/snippets/c/sie-dat.c */
> +#define GUEST_TOTAL_PAGE_COUNT 256

This (1M) is the maximum snippet size (see snippet_setup_guest), is this intentional?
In that case the comment is inaccurate, since you'd want to sync it with the maximum snippet size.
You also know the actual snippet size SNIPPET_LEN(c, sie_dat) so I don't see why you'd need a define
at all.

> +
> +static void test_sie_dat(void)
> +{
> 
[...]
> +
> +	/* the guest will now write to an unmapped address and we check that this causes a segment translation */

I'd prefer "causes a segment translation exception"

[...]


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

Pretty sure the compiler will generate a leading tab, so this will be doubly indented.
> +}
> +
> +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)

Why is the attribute necessary? I know all the snippets have it, but I don't see
why it's necessary.
@Janosch ?

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

Why not just use an address high enough you know it will not be mapped?
-1 should do just fine.

> +	*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 d97eb5e943c8..aab0e670f2d4 100644
> --- a/s390x/unittests.cfg
> +++ b/s390x/unittests.cfg
> @@ -215,3 +215,6 @@ file = migration-skey.elf
>  smp = 2
>  groups = migration
>  extra_params = -append '--parallel'
> +
> +[sie-dat]
> +file = sie-dat.elf

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

[Janosch Frank]

On 4/5/23 21:55, Nina Schoetterl-Glausch wrote:
> On Mon, 2023-03-27 at 10:21 +0200, 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>
>> ---

[...]

>> +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)
> 
> Why is the attribute necessary? I know all the snippets have it, but I don't see
> why it's necessary.
> @Janosch ?

"Historical growth" :)

If it doesn't work without it then we need to find a way to fix it.
If it does work without it then we should remove the attribute from all 
snippets.

But this issue is a low priority for me.

> 
>> +{
>> +	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);
> 
> Why not just use an address high enough you know it will not be mapped?
> -1 should do just fine.
> 
>> +	*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 d97eb5e943c8..aab0e670f2d4 100644
>> --- a/s390x/unittests.cfg
>> +++ b/s390x/unittests.cfg
>> @@ -215,3 +215,6 @@ file = migration-skey.elf
>>   smp = 2
>>   groups = migration
>>   extra_params = -append '--parallel'
>> +
>> +[sie-dat]
>> +file = sie-dat.elf
> 

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

[Nico Boehr]

Quoting Nina Schoetterl-Glausch (2023-04-05 21:55:01)
> On Mon, 2023-03-27 at 10:21 +0200, 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            | 121 +++++++++++++++++++++++++++++++++++++
> >  s390x/snippets/c/sie-dat.c |  58 ++++++++++++++++++
> >  s390x/unittests.cfg        |   3 +
> >  4 files changed, 184 insertions(+)
> >  create mode 100644 s390x/sie-dat.c
> >  create mode 100644 s390x/snippets/c/sie-dat.c
> > 
> 
> Test looks good to me. Some comments below.
> [...]
> 
> > diff --git a/s390x/sie-dat.c b/s390x/sie-dat.c
> > new file mode 100644
> > index 000000000000..37e46386181c
> > --- /dev/null
> > +++ b/s390x/sie-dat.c
> > @@ -0,0 +1,121 @@
> > 
> [...]
> > +
> > +/* keep in sync with TOTAL_PAGE_COUNT in s390x/snippets/c/sie-dat.c */
> > +#define GUEST_TOTAL_PAGE_COUNT 256
> 
> This (1M) is the maximum snippet size (see snippet_setup_guest), is this intentional?

It is by accident the maximum snippet size. It is completely fine to stay at 256 pages when we increase the maximum snippet size.

> In that case the comment is inaccurate, since you'd want to sync it with the maximum snippet size.
> You also know the actual snippet size SNIPPET_LEN(c, sie_dat) so I don't see why you'd need a define
> at all.

The snippet size is not the same as the number of mapped pages in the guest, no?

[...]
> > +
> > +static void test_sie_dat(void)
> > +{
> > 
> [...]
> > +
> > +     /* the guest will now write to an unmapped address and we check that this causes a segment translation */
> 
> I'd prefer "causes a segment translation exception"

OK

[...]
> > diff --git a/s390x/snippets/c/sie-dat.c b/s390x/snippets/c/sie-dat.c
> > new file mode 100644
> > index 000000000000..c9f7af0f3a56
> > --- /dev/null
> > +++ b/s390x/snippets/c/sie-dat.c
[...]
> > +static inline void force_exit(void)
> > +{
> > +     asm volatile("  diag    0,0,0x44\n");
> 
> Pretty sure the compiler will generate a leading tab, so this will be doubly indented.

Copy-paste artifact, I can remove it.

[...]
> > +{
> > +     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);
> 
> Why not just use an address high enough you know it will not be mapped?
> -1 should do just fine.

I wanted to make sure exactly the right amount of pages is mapped and no more.

-1 would defeat that purpose.

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

[Nina Schoetterl-Glausch]

On Thu, 2023-04-13 at 11:43 +0200, Nico Boehr wrote:
> Quoting Nina Schoetterl-Glausch (2023-04-05 21:55:01)
> > On Mon, 2023-03-27 at 10:21 +0200, 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            | 121 +++++++++++++++++++++++++++++++++++++
> > >  s390x/snippets/c/sie-dat.c |  58 ++++++++++++++++++
> > >  s390x/unittests.cfg        |   3 +
> > >  4 files changed, 184 insertions(+)
> > >  create mode 100644 s390x/sie-dat.c
> > >  create mode 100644 s390x/snippets/c/sie-dat.c
> > > 
> > 
> > Test looks good to me. Some comments below.
> > [...]
> > 
> > > diff --git a/s390x/sie-dat.c b/s390x/sie-dat.c
> > > new file mode 100644
> > > index 000000000000..37e46386181c
> > > --- /dev/null
> > > +++ b/s390x/sie-dat.c
> > > @@ -0,0 +1,121 @@
> > > 
> > [...]
> > > +
> > > +/* keep in sync with TOTAL_PAGE_COUNT in s390x/snippets/c/sie-dat.c */
> > > +#define GUEST_TOTAL_PAGE_COUNT 256
> > 
> > This (1M) is the maximum snippet size (see snippet_setup_guest), is this intentional?
> 
> It is by accident the maximum snippet size. It is completely fine to stay at 256 pages when we increase the maximum snippet size.
> 
> > In that case the comment is inaccurate, since you'd want to sync it with the maximum snippet size.
> > You also know the actual snippet size SNIPPET_LEN(c, sie_dat) so I don't see why you'd need a define
> > at all.
> 
> The snippet size is not the same as the number of mapped pages in the guest, no?

No, but one probably wouldn't want to map less that the snippet size.
And there probably isn't a reason to map more either.

[...]

> 
> > > +     /* the first unmapped address */
> > > +     invalid_ptr = (uint8_t *)(TOTAL_PAGE_COUNT * PAGE_SIZE);
> > 
> > Why not just use an address high enough you know it will not be mapped?
> > -1 should do just fine.
> 
> I wanted to make sure exactly the right amount of pages is mapped and no more.
> 
> -1 would defeat that purpose.

You want to touch the first page that isn't mapped?
Do you also want to map a certain number of pages?
I.e fill an entire page table and have the invalid pointer be mapped by another segment table entry?

With a small linker script change the snippet could know it's own length.
Then you could map just the required number of pages and don't need to keep those numbers in sync.

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

[Nico Boehr]

Quoting Nina Schoetterl-Glausch (2023-04-13 18:33:50)
[...]
> With a small linker script change the snippet could know it's own length.
> Then you could map just the required number of pages and don't need to keep those numbers in sync.

Maybe it's because my knowledge about linker scripts is really limited or I
don't get it, but I fail to see the advantage of the additional complexity.

My assumption would be that the number of pages mapped for the guest memory will
never really change. Keeping a define in sync seems more pragmatic than going
through linker script magic.

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

[Nina Schoetterl-Glausch]

On Fri, 2023-04-14 at 12:10 +0200, Nico Boehr wrote:
> Quoting Nina Schoetterl-Glausch (2023-04-13 18:33:50)
> [...]
> > With a small linker script change the snippet could know it's own length.
> > Then you could map just the required number of pages and don't need to keep those numbers in sync.
> 
> Maybe it's because my knowledge about linker scripts is really limited or I
> don't get it, but I fail to see the advantage of the additional complexity.
> 
> My assumption would be that the number of pages mapped for the guest memory will
> never really change. Keeping a define in sync seems more pragmatic than going
> through linker script magic.

I think just

+       . = ALIGN(4K);
+       esnippet = .;
        /* End data */

at the bottom of the snippet linker script would suffice.
Then you could use esnippet as an extern symbol in the C code.
But yeah, might not be worth it unless it would help with other snippets too.