s390x: Improve architectural compliance for diag308

PATCH Superseded Janosch Frank 3 participants 10 messages Nov 5, 2019 – Nov 6, 2019

Subsystems (2)

KVM s390

lore

Share & tools

lore

Janosch Frank

author

2019-11-05 16:29 UTC #0 lore raw

cover s390x: Improve architectural compliance for diag308

When testing diag308 subcodes 0/1 on lpar with virtual mem set up, I experienced spec PGMs and addressing PGMs due to the tests not setting short psw bit 12 and leaving the DAT bit on.

The problem was not found under KVM/QEMU, because Qemu just ignores all cpu mask bits... I'm working on a fix for that too.

Janosch Frank (2):
s390x: Add CR save area
s390x: Remove DAT and add short indication psw bits on diag308 reset

 lib/s390x/asm-offsets.c  |  3 ++-
 lib/s390x/asm/arch_def.h |  5 +++--
 lib/s390x/interrupt.c    |  4 ++--
 lib/s390x/smp.c          |  2 +-
 s390x/cstart64.S         | 29 ++++++++++++++++++++---------
 5 files changed, 28 insertions(+), 15 deletions(-)

Signature · 1 line

2.20.1

Janosch Frank

author

2019-11-05 16:29 UTC #1 lore raw

PATCH 1/2 s390x: Add CR save area

If we run with DAT enabled and do a reset, we need to save the CRs to backup our ASCEs on a diag308 for example.

Signed-off-by Janosch Frank <[email protected]>

---
 lib/s390x/asm-offsets.c  | 2 +-
 lib/s390x/asm/arch_def.h | 4 ++--
 lib/s390x/interrupt.c    | 4 ++--
 lib/s390x/smp.c          | 2 +-
 s390x/cstart64.S         | 9 ++++++---
 5 files changed, 12 insertions(+), 9 deletions(-)

lib/s390x/asm-offsets.c +1 -1

index 6e2d259..4b213f8 100644
@@ -57,7 +57,7 @@ int main(void)
 	OFFSET(GEN_LC_SW_INT_GRS, lowcore, sw_int_grs);
 	OFFSET(GEN_LC_SW_INT_FPRS, lowcore, sw_int_fprs);
 	OFFSET(GEN_LC_SW_INT_FPC, lowcore, sw_int_fpc);
-	OFFSET(GEN_LC_SW_INT_CR0, lowcore, sw_int_cr0);
+	OFFSET(GEN_LC_SW_INT_CRS, lowcore, sw_int_crs);
 	OFFSET(GEN_LC_MCCK_EXT_SA_ADDR, lowcore, mcck_ext_sa_addr);
 	OFFSET(GEN_LC_FPRS_SA, lowcore, fprs_sa);
 	OFFSET(GEN_LC_GRS_SA, lowcore, grs_sa);

lib/s390x/asm/arch_def.h +2 -2

index 96cca2e..07d4e5e 100644
@@ -78,8 +78,8 @@ struct lowcore {
 	uint64_t	sw_int_fprs[16];		/* 0x0280 */
 	uint32_t	sw_int_fpc;			/* 0x0300 */
 	uint8_t		pad_0x0304[0x0308 - 0x0304];	/* 0x0304 */
-	uint64_t	sw_int_cr0;			/* 0x0308 */
-	uint8_t		pad_0x0310[0x11b0 - 0x0310];	/* 0x0310 */
+	uint64_t	sw_int_crs[16];			/* 0x0308 */
+	uint8_t		pad_0x0310[0x11b0 - 0x0388];	/* 0x0388 */
 	uint64_t	mcck_ext_sa_addr;		/* 0x11b0 */
 	uint8_t		pad_0x11b8[0x1200 - 0x11b8];	/* 0x11b8 */
 	uint64_t	fprs_sa[16];			/* 0x1200 */

lib/s390x/interrupt.c +2 -2

index 5cade23..c9e2dc6 100644
@@ -124,13 +124,13 @@ void handle_ext_int(void)
 	}
 
 	if (lc->ext_int_code == EXT_IRQ_SERVICE_SIG) {
-		lc->sw_int_cr0 &= ~(1UL << 9);
+		lc->sw_int_crs[0] &= ~(1UL << 9);
 		sclp_handle_ext();
 	} else {
 		ext_int_expected = false;
 	}
 
-	if (!(lc->sw_int_cr0 & CR0_EXTM_MASK))
+	if (!(lc->sw_int_crs[0] & CR0_EXTM_MASK))
 		lc->ext_old_psw.mask &= ~PSW_MASK_EXT;
 }

lib/s390x/smp.c +1 -1

index 7602886..f57f420 100644
@@ -189,7 +189,7 @@ int smp_cpu_setup(uint16_t addr, struct psw psw)
 	cpu->lowcore->sw_int_grs[15] = (uint64_t)cpu->stack + (PAGE_SIZE * 4);
 	lc->restart_new_psw.mask = 0x0000000180000000UL;
 	lc->restart_new_psw.addr = (uint64_t)smp_cpu_setup_state;
-	lc->sw_int_cr0 = 0x0000000000040000UL;
+	lc->sw_int_crs[0] = 0x0000000000040000UL;
 
 	/* Start processing */
 	rc = sigp_retry(cpu->addr, SIGP_RESTART, 0, NULL);

s390x/cstart64.S +6 -3

index 8e2b21e..0455591 100644
@@ -93,7 +93,7 @@ memsetxc:
 	/* save grs 0-15 */
 	stmg	%r0, %r15, GEN_LC_SW_INT_GRS
 	/* save cr0 */
-	stctg	%c0, %c0, GEN_LC_SW_INT_CR0
+	stctg	%c0, %c15, GEN_LC_SW_INT_CRS
 	/* load initial cr0 again */
 	larl	%r1, initial_cr0
 	lctlg	%c0, %c0, 0(%r1)

@@ -107,13 +107,16 @@ memsetxc:
 
 	.macro RESTORE_REGS
 	/* restore fprs 0-15 + fpc */
+	/* load initial cr0 again */
+	larl	%r1, initial_cr0
+	lctlg	%c0, %c0, 0(%r1)
 	la	%r1, GEN_LC_SW_INT_FPRS
 	.irp i, 0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15
 	ld	\i, \i * 8(%r1)
 	.endr
 	lfpc	GEN_LC_SW_INT_FPC
 	/* restore cr0 */
-	lctlg	%c0, %c0, GEN_LC_SW_INT_CR0
+	lctlg	%c0, %c15, GEN_LC_SW_INT_CRS
 	/* restore grs 0-15 */
 	lmg	%r0, %r15, GEN_LC_SW_INT_GRS
 	.endm

@@ -150,7 +153,7 @@ diag308_load_reset:
 smp_cpu_setup_state:
 	xgr	%r1, %r1
 	lmg     %r0, %r15, GEN_LC_SW_INT_GRS
-	lctlg   %c0, %c0, GEN_LC_SW_INT_CR0
+	lctlg   %c0, %c0, GEN_LC_SW_INT_CRS
 	br	%r14
 
 pgm_int:

Signature · 1 line

2.20.1

David Hildenbrand

2019-11-05 19:34 UTC #2 lore raw

Re: [kvm-unit-tests PATCH 1/2] s390x: Add CR save area

Quoted 78 lines · Janosch Frank

If we run with DAT enabled and do a reset, we need to save the CRs to backup our ASCEs on a diag308 for example.

Signed-off-by Janosch Frank <[email protected]>

---
  lib/s390x/asm-offsets.c  | 2 +-
  lib/s390x/asm/arch_def.h | 4 ++--
  lib/s390x/interrupt.c    | 4 ++--
  lib/s390x/smp.c          | 2 +-
  s390x/cstart64.S         | 9 ++++++---
  5 files changed, 12 insertions(+), 9 deletions(-)

lib/s390x/asm-offsets.c +1 -1

index 6e2d259..4b213f8 100644
@@ -57,7 +57,7 @@ int main(void)
  	OFFSET(GEN_LC_SW_INT_GRS, lowcore, sw_int_grs);
  	OFFSET(GEN_LC_SW_INT_FPRS, lowcore, sw_int_fprs);
  	OFFSET(GEN_LC_SW_INT_FPC, lowcore, sw_int_fpc);
-	OFFSET(GEN_LC_SW_INT_CR0, lowcore, sw_int_cr0);
+	OFFSET(GEN_LC_SW_INT_CRS, lowcore, sw_int_crs);
  	OFFSET(GEN_LC_MCCK_EXT_SA_ADDR, lowcore, mcck_ext_sa_addr);
  	OFFSET(GEN_LC_FPRS_SA, lowcore, fprs_sa);
  	OFFSET(GEN_LC_GRS_SA, lowcore, grs_sa);

lib/s390x/asm/arch_def.h +2 -2

index 96cca2e..07d4e5e 100644
@@ -78,8 +78,8 @@ struct lowcore {
  	uint64_t	sw_int_fprs[16];		/* 0x0280 */
  	uint32_t	sw_int_fpc;			/* 0x0300 */
  	uint8_t		pad_0x0304[0x0308 - 0x0304];	/* 0x0304 */
-	uint64_t	sw_int_cr0;			/* 0x0308 */
-	uint8_t		pad_0x0310[0x11b0 - 0x0310];	/* 0x0310 */
+	uint64_t	sw_int_crs[16];			/* 0x0308 */
+	uint8_t		pad_0x0310[0x11b0 - 0x0388];	/* 0x0388 */
  	uint64_t	mcck_ext_sa_addr;		/* 0x11b0 */
  	uint8_t		pad_0x11b8[0x1200 - 0x11b8];	/* 0x11b8 */
  	uint64_t	fprs_sa[16];			/* 0x1200 */

lib/s390x/interrupt.c +2 -2

index 5cade23..c9e2dc6 100644
@@ -124,13 +124,13 @@ void handle_ext_int(void)
  	}
 
  	if (lc->ext_int_code == EXT_IRQ_SERVICE_SIG) {
-		lc->sw_int_cr0 &= ~(1UL << 9);
+		lc->sw_int_crs[0] &= ~(1UL << 9);
  		sclp_handle_ext();
  	} else {
  		ext_int_expected = false;
  	}
 
-	if (!(lc->sw_int_cr0 & CR0_EXTM_MASK))
+	if (!(lc->sw_int_crs[0] & CR0_EXTM_MASK))
  		lc->ext_old_psw.mask &= ~PSW_MASK_EXT;
  }

lib/s390x/smp.c +1 -1

index 7602886..f57f420 100644
@@ -189,7 +189,7 @@ int smp_cpu_setup(uint16_t addr, struct psw psw)
  	cpu->lowcore->sw_int_grs[15] = (uint64_t)cpu->stack + (PAGE_SIZE * 4);
  	lc->restart_new_psw.mask = 0x0000000180000000UL;
  	lc->restart_new_psw.addr = (uint64_t)smp_cpu_setup_state;
-	lc->sw_int_cr0 = 0x0000000000040000UL;
+	lc->sw_int_crs[0] = 0x0000000000040000UL;
 
  	/* Start processing */
  	rc = sigp_retry(cpu->addr, SIGP_RESTART, 0, NULL);

s390x/cstart64.S +0 -0

index 8e2b21e..0455591 100644

s390x/cstart64.S memsetxc: · line 95

Quoted 3 lines

s390x/cstart64.S +0 -0

@@ -93,7 +93,7 @@ memsetxc:
  	/* save grs 0-15 */
  	stmg	%r0, %r15, GEN_LC_SW_INT_GRS
  	/* save cr0 */

Comment needs an update

s390x/cstart64.S memsetxc: · line 110

Quoted 10 lines

s390x/cstart64.S +2 -1

-	stctg	%c0, %c0, GEN_LC_SW_INT_CR0
+	stctg	%c0, %c15, GEN_LC_SW_INT_CRS
  	/* load initial cr0 again */
  	larl	%r1, initial_cr0
  	lctlg	%c0, %c0, 0(%r1)

@@ -107,13 +107,16 @@ memsetxc:
 
  	.macro RESTORE_REGS
  	/* restore fprs 0-15 + fpc */
+	/* load initial cr0 again */

Two comments in a wrong look wrong.

s390x/cstart64.S memsetxc: · line 112

Quoted 2 lines

s390x/cstart64.S +2 -0

+	larl	%r1, initial_cr0
+	lctlg	%c0, %c0, 0(%r1)

This hunk does somewhat not fit to this patch description. This needs an explanation or should be moved to a separate patch.

s390x/cstart64.S memsetxc: · line 118

Quoted 6 lines

s390x/cstart64.S +0 -0

  	la	%r1, GEN_LC_SW_INT_FPRS
  	.irp i, 0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15
  	ld	\i, \i * 8(%r1)
  	.endr
  	lfpc	GEN_LC_SW_INT_FPC
  	/* restore cr0 */

Comments needs an update

Quoted 7 lines

s390x/cstart64.S +1 -1

-	lctlg	%c0, %c0, GEN_LC_SW_INT_CR0
+	lctlg	%c0, %c15, GEN_LC_SW_INT_CRS
  	/* restore grs 0-15 */
  	lmg	%r0, %r15, GEN_LC_SW_INT_GRS
  	.endm

s390x/cstart64.S diag308_load_reset: · line 159

Quoted 8 lines

s390x/cstart64.S +1 -1

@@ -150,7 +153,7 @@ diag308_load_reset:
  smp_cpu_setup_state:
  	xgr	%r1, %r1
  	lmg     %r0, %r15, GEN_LC_SW_INT_GRS
-	lctlg   %c0, %c0, GEN_LC_SW_INT_CR0
+	lctlg   %c0, %c0, GEN_LC_SW_INT_CRS
  	br	%r14
 
  pgm_int:

Apart from that looks good.

Signature · 3 lines

Thanks,

David / dhildenb

Janosch Frank

author

2019-11-05 16:29 UTC #3 lore raw

PATCH 2/2 s390x: Remove DAT and add short indication psw bits on diag308 reset

In reply to Janosch Frank

On a diag308 subcode 0 CRs will be reset, so we need to mask of PSW DAT indication until we restore our CRs.

Also we need to set the short psw indication to be compliant with the architecture.

Signed-off-by Janosch Frank <[email protected]>

---
 lib/s390x/asm-offsets.c  |  1 +
 lib/s390x/asm/arch_def.h |  3 ++-
 s390x/cstart64.S         | 20 ++++++++++++++------
 3 files changed, 17 insertions(+), 7 deletions(-)

lib/s390x/asm-offsets.c +1 -0

index 4b213f8..61d2658 100644
@@ -58,6 +58,7 @@ int main(void)
 	OFFSET(GEN_LC_SW_INT_FPRS, lowcore, sw_int_fprs);
 	OFFSET(GEN_LC_SW_INT_FPC, lowcore, sw_int_fpc);
 	OFFSET(GEN_LC_SW_INT_CRS, lowcore, sw_int_crs);
+	OFFSET(GEN_LC_SW_INT_PSW, lowcore, sw_int_psw);
 	OFFSET(GEN_LC_MCCK_EXT_SA_ADDR, lowcore, mcck_ext_sa_addr);
 	OFFSET(GEN_LC_FPRS_SA, lowcore, fprs_sa);
 	OFFSET(GEN_LC_GRS_SA, lowcore, grs_sa);

lib/s390x/asm/arch_def.h +2 -1

index 07d4e5e..7d25e4f 100644
@@ -79,7 +79,8 @@ struct lowcore {
 	uint32_t	sw_int_fpc;			/* 0x0300 */
 	uint8_t		pad_0x0304[0x0308 - 0x0304];	/* 0x0304 */
 	uint64_t	sw_int_crs[16];			/* 0x0308 */
-	uint8_t		pad_0x0310[0x11b0 - 0x0388];	/* 0x0388 */
+	struct psw	sw_int_psw;			/* 0x0388 */
+	uint8_t		pad_0x0310[0x11b0 - 0x0390];	/* 0x0390 */
 	uint64_t	mcck_ext_sa_addr;		/* 0x11b0 */
 	uint8_t		pad_0x11b8[0x1200 - 0x11b8];	/* 0x11b8 */
 	uint64_t	fprs_sa[16];			/* 0x1200 */

s390x/cstart64.S +14 -6

index 0455591..2e0dcf5 100644
@@ -129,8 +129,15 @@ memsetxc:
 .globl diag308_load_reset
 diag308_load_reset:
 	SAVE_REGS
-	/* Save the first PSW word to the IPL PSW */
+	/* Backup current PSW */
 	epsw	%r0, %r1
+	st	%r0, GEN_LC_SW_INT_PSW
+	st	%r1, GEN_LC_SW_INT_PSW + 4
+	/* Disable DAT as the CRs will be reset too */
+	nilh	%r0, 0xfbff
+	/* Add psw bit 12 to indicate short psw */
+	oilh	%r0, 0x0008
+	/* Save the first PSW word to the IPL PSW */
 	st	%r0, 0
 	/* Store the address and the bit for 31 bit addressing */
 	larl    %r0, 0f

@@ -142,12 +149,13 @@ diag308_load_reset:
 	xgr	%r2, %r2
 	br	%r14
 	/* Success path */
-	/* We lost cr0 due to the reset */
-0:	larl	%r1, initial_cr0
-	lctlg	%c0, %c0, 0(%r1)
-	RESTORE_REGS
+	/* Switch to z/Architecture mode and 64-bit */
+0:	RESTORE_REGS
 	lhi	%r2, 1
-	br	%r14
+	larl	%r0, 1f
+	stg	%r0, GEN_LC_SW_INT_PSW + 8
+	lpswe	GEN_LC_SW_INT_PSW
+1:	br	%r14
 
 .globl smp_cpu_setup_state
 smp_cpu_setup_state:

Signature · 1 line

2.20.1

David Hildenbrand

2019-11-05 19:53 UTC #4 lore raw

Re: [kvm-unit-tests PATCH 2/2] s390x: Remove DAT and add short indication psw bits on diag308 reset

On 05.11.19 17:28, Janosch Frank wrote:

In the subject "Disable" vs. "Remove" ?

Quoted 44 lines

On a diag308 subcode 0 CRs will be reset, so we need to mask of PSW DAT indication until we restore our CRs.

Also we need to set the short psw indication to be compliant with the architecture.

Signed-off-by Janosch Frank <[email protected]>

---
  lib/s390x/asm-offsets.c  |  1 +
  lib/s390x/asm/arch_def.h |  3 ++-
  s390x/cstart64.S         | 20 ++++++++++++++------
  3 files changed, 17 insertions(+), 7 deletions(-)

lib/s390x/asm-offsets.c +1 -0

index 4b213f8..61d2658 100644
@@ -58,6 +58,7 @@ int main(void)
  	OFFSET(GEN_LC_SW_INT_FPRS, lowcore, sw_int_fprs);
  	OFFSET(GEN_LC_SW_INT_FPC, lowcore, sw_int_fpc);
  	OFFSET(GEN_LC_SW_INT_CRS, lowcore, sw_int_crs);
+	OFFSET(GEN_LC_SW_INT_PSW, lowcore, sw_int_psw);
  	OFFSET(GEN_LC_MCCK_EXT_SA_ADDR, lowcore, mcck_ext_sa_addr);
  	OFFSET(GEN_LC_FPRS_SA, lowcore, fprs_sa);
  	OFFSET(GEN_LC_GRS_SA, lowcore, grs_sa);

lib/s390x/asm/arch_def.h +2 -1

index 07d4e5e..7d25e4f 100644
@@ -79,7 +79,8 @@ struct lowcore {
  	uint32_t	sw_int_fpc;			/* 0x0300 */
  	uint8_t		pad_0x0304[0x0308 - 0x0304];	/* 0x0304 */
  	uint64_t	sw_int_crs[16];			/* 0x0308 */
-	uint8_t		pad_0x0310[0x11b0 - 0x0388];	/* 0x0388 */
+	struct psw	sw_int_psw;			/* 0x0388 */
+	uint8_t		pad_0x0310[0x11b0 - 0x0390];	/* 0x0390 */
  	uint64_t	mcck_ext_sa_addr;		/* 0x11b0 */
  	uint8_t		pad_0x11b8[0x1200 - 0x11b8];	/* 0x11b8 */
  	uint64_t	fprs_sa[16];			/* 0x1200 */

s390x/cstart64.S +0 -0

index 0455591..2e0dcf5 100644

s390x/cstart64.S memsetxc: · line 132

Quoted 5 lines

s390x/cstart64.S +1 -1

@@ -129,8 +129,15 @@ memsetxc:
  .globl diag308_load_reset
  diag308_load_reset:
  	SAVE_REGS
-	/* Save the first PSW word to the IPL PSW */
+	/* Backup current PSW */

/*
* Backup the current PSW MASK, as we have to restore it on
* success.
*/

s390x/cstart64.S memsetxc: · line 135

Quoted 3 lines

s390x/cstart64.S +2 -0

  	epsw	%r0, %r1
+	st	%r0, GEN_LC_SW_INT_PSW
+	st	%r1, GEN_LC_SW_INT_PSW + 4

I was confused at first, but then I realized that you really only store the PSW mask here and not also the PSW address ...

s390x/cstart64.S memsetxc: · line 139

Quoted 4 lines

s390x/cstart64.S +4 -0

+	/* Disable DAT as the CRs will be reset too */
+	nilh	%r0, 0xfbff
+	/* Add psw bit 12 to indicate short psw */
+	oilh	%r0, 0x0008

Why care about the old PSW mask here at all? Wouldn't it be easier to just construct a new PSW mask from scratch? (64bit, PSW bit 12 set ...)

Save it somewhere and just load it directly from memory.

Quoted 24 lines

s390x/cstart64.S +7 -5

+	/* Save the first PSW word to the IPL PSW */
  	st	%r0, 0
  	/* Store the address and the bit for 31 bit addressing */
  	larl    %r0, 0f

@@ -142,12 +149,13 @@ diag308_load_reset:
  	xgr	%r2, %r2
  	br	%r14
  	/* Success path */
-	/* We lost cr0 due to the reset */
-0:	larl	%r1, initial_cr0
-	lctlg	%c0, %c0, 0(%r1)
-	RESTORE_REGS
+	/* Switch to z/Architecture mode and 64-bit */
+0:	RESTORE_REGS
  	lhi	%r2, 1
-	br	%r14
+	larl	%r0, 1f
+	stg	%r0, GEN_LC_SW_INT_PSW + 8
+	lpswe	GEN_LC_SW_INT_PSW
+1:	br	%r14
 
  .globl smp_cpu_setup_state
  smp_cpu_setup_state:

Signature · 3 lines

Thanks,

David / dhildenb

Janosch Frank

author

2019-11-06 06:58 UTC #5 lore raw

Re: [kvm-unit-tests PATCH 2/2] s390x: Remove DAT and add short indication psw bits on diag308 reset

Quoted 75 lines · David Hildenbrand

On 05.11.19 17:28, Janosch Frank wrote:
In the subject "Disable" vs. "Remove" ?
Quoted 44 lines
On a diag308 subcode 0 CRs will be reset, so we need to mask of PSW DAT indication until we restore our CRs.
Also we need to set the short psw indication to be compliant with the architecture.
Signed-off-by Janosch Frank <[email protected]>
---
  lib/s390x/asm-offsets.c  |  1 +
  lib/s390x/asm/arch_def.h |  3 ++-
  s390x/cstart64.S         | 20 ++++++++++++++------
  3 files changed, 17 insertions(+), 7 deletions(-)
lib/s390x/asm-offsets.c +1 -0
index 4b213f8..61d2658 100644
@@ -58,6 +58,7 @@ int main(void)
  	OFFSET(GEN_LC_SW_INT_FPRS, lowcore, sw_int_fprs);
  	OFFSET(GEN_LC_SW_INT_FPC, lowcore, sw_int_fpc);
  	OFFSET(GEN_LC_SW_INT_CRS, lowcore, sw_int_crs);
+	OFFSET(GEN_LC_SW_INT_PSW, lowcore, sw_int_psw);
  	OFFSET(GEN_LC_MCCK_EXT_SA_ADDR, lowcore, mcck_ext_sa_addr);
  	OFFSET(GEN_LC_FPRS_SA, lowcore, fprs_sa);
  	OFFSET(GEN_LC_GRS_SA, lowcore, grs_sa);
lib/s390x/asm/arch_def.h +2 -1
index 07d4e5e..7d25e4f 100644
@@ -79,7 +79,8 @@ struct lowcore {
  	uint32_t	sw_int_fpc;			/* 0x0300 */
  	uint8_t		pad_0x0304[0x0308 - 0x0304];	/* 0x0304 */
  	uint64_t	sw_int_crs[16];			/* 0x0308 */
-	uint8_t		pad_0x0310[0x11b0 - 0x0388];	/* 0x0388 */
+	struct psw	sw_int_psw;			/* 0x0388 */
+	uint8_t		pad_0x0310[0x11b0 - 0x0390];	/* 0x0390 */
  	uint64_t	mcck_ext_sa_addr;		/* 0x11b0 */
  	uint8_t		pad_0x11b8[0x1200 - 0x11b8];	/* 0x11b8 */
  	uint64_t	fprs_sa[16];			/* 0x1200 */
s390x/cstart64.S +0 -0
index 0455591..2e0dcf5 100644
s390x/cstart64.S memsetxc: · line 132
Quoted 5 lines
s390x/cstart64.S +1 -1
@@ -129,8 +129,15 @@ memsetxc:
  .globl diag308_load_reset
  diag308_load_reset:
  	SAVE_REGS
-	/* Save the first PSW word to the IPL PSW */
+	/* Backup current PSW */
/*
* Backup the current PSW MASK, as we have to restore it on
* success.
*/
s390x/cstart64.S memsetxc: · line 135
Quoted 3 lines
s390x/cstart64.S +2 -0
  	epsw	%r0, %r1
+	st	%r0, GEN_LC_SW_INT_PSW
+	st	%r1, GEN_LC_SW_INT_PSW + 4
I was confused at first, but then I realized that you really only store the PSW mask here and not also the PSW address ...
s390x/cstart64.S memsetxc: · line 139
Quoted 4 lines
s390x/cstart64.S +4 -0
+	/* Disable DAT as the CRs will be reset too */
+	nilh	%r0, 0xfbff
+	/* Add psw bit 12 to indicate short psw */
+	oilh	%r0, 0x0008
Why care about the old PSW mask here at all? Wouldn't it be easier to just construct a new PSW mask from scratch? (64bit, PSW bit 12 set ...)

Quoted 1 line

Save it somewhere and just load it directly from memory.

Sounds like a good idea, will do

Quoted 27 lines

Quoted 25 lines

s390x/cstart64.S +7 -5

+	/* Save the first PSW word to the IPL PSW */
  	st	%r0, 0
  	/* Store the address and the bit for 31 bit addressing */
  	larl    %r0, 0f

@@ -142,12 +149,13 @@ diag308_load_reset:
  	xgr	%r2, %r2
  	br	%r14
  	/* Success path */
-	/* We lost cr0 due to the reset */
-0:	larl	%r1, initial_cr0
-	lctlg	%c0, %c0, 0(%r1)
-	RESTORE_REGS
+	/* Switch to z/Architecture mode and 64-bit */
+0:	RESTORE_REGS
  	lhi	%r2, 1
-	br	%r14
+	larl	%r0, 1f
+	stg	%r0, GEN_LC_SW_INT_PSW + 8
+	lpswe	GEN_LC_SW_INT_PSW
+1:	br	%r14
 
  .globl smp_cpu_setup_state
  smp_cpu_setup_state:

Janosch Frank

author

2019-11-06 10:19 UTC #6 lore raw

Re: [kvm-unit-tests PATCH 2/2] s390x: Remove DAT and add short indication psw bits on diag308 reset

In reply to Janosch Frank

Quoted 30 lines · Janosch Frank

On a diag308 subcode 0 CRs will be reset, so we need to mask of PSW DAT indication until we restore our CRs.
Also we need to set the short psw indication to be compliant with the architecture.
Signed-off-by Janosch Frank <[email protected]>
---
 lib/s390x/asm-offsets.c  |  1 +
 lib/s390x/asm/arch_def.h |  3 ++-
 s390x/cstart64.S         | 20 ++++++++++++++------
 3 files changed, 17 insertions(+), 7 deletions(-)
lib/s390x/asm-offsets.c +1 -0
index 4b213f8..61d2658 100644
@@ -58,6 +58,7 @@ int main(void)
 	OFFSET(GEN_LC_SW_INT_FPRS, lowcore, sw_int_fprs);
 	OFFSET(GEN_LC_SW_INT_FPC, lowcore, sw_int_fpc);
 	OFFSET(GEN_LC_SW_INT_CRS, lowcore, sw_int_crs);
+	OFFSET(GEN_LC_SW_INT_PSW, lowcore, sw_int_psw);
 	OFFSET(GEN_LC_MCCK_EXT_SA_ADDR, lowcore, mcck_ext_sa_addr);
 	OFFSET(GEN_LC_FPRS_SA, lowcore, fprs_sa);
 	OFFSET(GEN_LC_GRS_SA, lowcore, grs_sa);
lib/s390x/asm/arch_def.h +0 -0
index 07d4e5e..7d25e4f 100644

lib/s390x/asm/arch_def.h struct lowcore { · line 83

Quoted 6 lines

lib/s390x/asm/arch_def.h +2 -1

@@ -79,7 +79,8 @@ struct lowcore {
 	uint32_t	sw_int_fpc;			/* 0x0300 */
 	uint8_t		pad_0x0304[0x0308 - 0x0304];	/* 0x0304 */
 	uint64_t	sw_int_crs[16];			/* 0x0308 */
-	uint8_t		pad_0x0310[0x11b0 - 0x0388];	/* 0x0388 */
+	struct psw	sw_int_psw;			/* 0x0388 */
+	uint8_t		pad_0x0310[0x11b0 - 0x0390];	/* 0x0390 */

That's obviously not correct, the psw is two DWORDS long, not one...

Quoted 44 lines

lib/s390x/asm/arch_def.h +0 -0

 	uint64_t	mcck_ext_sa_addr;		/* 0x11b0 */
 	uint8_t		pad_0x11b8[0x1200 - 0x11b8];	/* 0x11b8 */
 	uint64_t	fprs_sa[16];			/* 0x1200 */

s390x/cstart64.S +14 -6

index 0455591..2e0dcf5 100644
@@ -129,8 +129,15 @@ memsetxc:
 .globl diag308_load_reset
 diag308_load_reset:
 	SAVE_REGS
-	/* Save the first PSW word to the IPL PSW */
+	/* Backup current PSW */
 	epsw	%r0, %r1
+	st	%r0, GEN_LC_SW_INT_PSW
+	st	%r1, GEN_LC_SW_INT_PSW + 4
+	/* Disable DAT as the CRs will be reset too */
+	nilh	%r0, 0xfbff
+	/* Add psw bit 12 to indicate short psw */
+	oilh	%r0, 0x0008
+	/* Save the first PSW word to the IPL PSW */
 	st	%r0, 0
 	/* Store the address and the bit for 31 bit addressing */
 	larl    %r0, 0f

@@ -142,12 +149,13 @@ diag308_load_reset:
 	xgr	%r2, %r2
 	br	%r14
 	/* Success path */
-	/* We lost cr0 due to the reset */
-0:	larl	%r1, initial_cr0
-	lctlg	%c0, %c0, 0(%r1)
-	RESTORE_REGS
+	/* Switch to z/Architecture mode and 64-bit */
+0:	RESTORE_REGS
 	lhi	%r2, 1
-	br	%r14
+	larl	%r0, 1f
+	stg	%r0, GEN_LC_SW_INT_PSW + 8
+	lpswe	GEN_LC_SW_INT_PSW
+1:	br	%r14
 
 .globl smp_cpu_setup_state
 smp_cpu_setup_state:

David Hildenbrand

2019-11-05 17:34 UTC #7 lore raw

In reply to Janosch Frank

Quoted 11 lines

Am 05.11.2019 um 17:29 schrieb Janosch Frank <[email protected]>:
When testing diag308 subcodes 0/1 on lpar with virtual mem set up, I experienced spec PGMs and addressing PGMs due to the tests not setting short psw bit 12 and leaving the DAT bit on.
The problem was not found under KVM/QEMU, because Qemu just ignores all cpu mask bits... I'm working on a fix for that too.

I don‘t have access to documentation. Is what LPAR does documented behavior or is this completely undocumented and therefore undefined behavior? Then we should remove these test cases completely instead.

Quoted 14 lines

Janosch Frank (2):
s390x: Add CR save area
s390x: Remove DAT and add short indication psw bits on diag308 reset
lib/s390x/asm-offsets.c  |  3 ++-
lib/s390x/asm/arch_def.h |  5 +++--
lib/s390x/interrupt.c    |  4 ++--
lib/s390x/smp.c          |  2 +-
s390x/cstart64.S         | 29 ++++++++++++++++++++---------
5 files changed, 28 insertions(+), 15 deletions(-)
Signature · 1 line
2.20.1

Christian Borntraeger

2019-11-05 18:23 UTC #8 lore raw

Quoted 8 lines · David Hildenbrand

Quoted 8 lines
Am 05.11.2019 um 17:29 schrieb Janosch Frank <[email protected]>:
When testing diag308 subcodes 0/1 on lpar with virtual mem set up, I experienced spec PGMs and addressing PGMs due to the tests not setting short psw bit 12 and leaving the DAT bit on.

Quoted 5 lines

Quoted 3 lines
The problem was not found under KVM/QEMU, because Qemu just ignores all cpu mask bits... I'm working on a fix for that too.
I don‘t have access to documentation. Is what LPAR does documented behavior or is this completely undocumented and therefore undefined behavior? Then we should remove these test cases completely instead.

Yes. It was just that KVM/QEMU never looked at the mask and just used a default one. The short PSW on address 0 clearly contains a mask and we should better set it.

Quoted 15 lines

Quoted 15 lines
Janosch Frank (2):
s390x: Add CR save area
s390x: Remove DAT and add short indication psw bits on diag308 reset
lib/s390x/asm-offsets.c  |  3 ++-
lib/s390x/asm/arch_def.h |  5 +++--
lib/s390x/interrupt.c    |  4 ++--
lib/s390x/smp.c          |  2 +-
s390x/cstart64.S         | 29 ++++++++++++++++++++---------
5 files changed, 28 insertions(+), 15 deletions(-)
Signature · 1 line
2.20.1

Janosch Frank

author

2019-11-05 18:38 UTC #9 lore raw

Quoted 15 lines · Christian Borntraeger

Quoted 12 lines · David Hildenbrand
Quoted 11 lines
Am 05.11.2019 um 17:29 schrieb Janosch Frank <[email protected]>:
When testing diag308 subcodes 0/1 on lpar with virtual mem set up, I experienced spec PGMs and addressing PGMs due to the tests not setting short psw bit 12 and leaving the DAT bit on.
The problem was not found under KVM/QEMU, because Qemu just ignores all cpu mask bits... I'm working on a fix for that too.

Quoted 5 lines

Quoted 1 line
I don‘t have access to documentation. Is what LPAR does documented behavior or is this completely undocumented and therefore undefined behavior? Then we should remove these test cases completely instead.
Yes. It was just that KVM/QEMU never looked at the mask and just used a default one. The short PSW on address 0 clearly contains a mask and we should better set it.

Yeah, we're currently reviewing the QEMU patch to fix this, I'll send it out tomorrow.

Quoted 15 lines

Quoted 15 lines
Quoted 15 lines
Janosch Frank (2):
s390x: Add CR save area
s390x: Remove DAT and add short indication psw bits on diag308 reset
lib/s390x/asm-offsets.c  |  3 ++-
lib/s390x/asm/arch_def.h |  5 +++--
lib/s390x/interrupt.c    |  4 ++--
lib/s390x/smp.c          |  2 +-
s390x/cstart64.S         | 29 ++++++++++++++++++++---------
5 files changed, 28 insertions(+), 15 deletions(-)
Signature · 1 line
2.20.1