[kvm-unit-tests PATCH 0/2] s390x: Improve architectural compliance for diag308

[kvm-unit-tests PATCH 0/2] s390x: Improve architectural compliance for diag308

[Janosch Frank]

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

-- 
2.20.1

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

[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 <frankja@linux.ibm.com>
---
 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(-)

diff --git a/lib/s390x/asm-offsets.c b/lib/s390x/asm-offsets.c
index 6e2d259..4b213f8 100644
--- a/lib/s390x/asm-offsets.c
+++ b/lib/s390x/asm-offsets.c
@@ -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);
diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
index 96cca2e..07d4e5e 100644
--- a/lib/s390x/asm/arch_def.h
+++ b/lib/s390x/asm/arch_def.h
@@ -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 */
diff --git a/lib/s390x/interrupt.c b/lib/s390x/interrupt.c
index 5cade23..c9e2dc6 100644
--- a/lib/s390x/interrupt.c
+++ b/lib/s390x/interrupt.c
@@ -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;
 }
 
diff --git a/lib/s390x/smp.c b/lib/s390x/smp.c
index 7602886..f57f420 100644
--- a/lib/s390x/smp.c
+++ b/lib/s390x/smp.c
@@ -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);
diff --git a/s390x/cstart64.S b/s390x/cstart64.S
index 8e2b21e..0455591 100644
--- a/s390x/cstart64.S
+++ b/s390x/cstart64.S
@@ -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:
-- 
2.20.1

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

[David Hildenbrand]

On 05.11.19 17:28, Janosch Frank wrote:
> 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 <frankja@linux.ibm.com>
> ---
>   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(-)
> 
> diff --git a/lib/s390x/asm-offsets.c b/lib/s390x/asm-offsets.c
> index 6e2d259..4b213f8 100644
> --- a/lib/s390x/asm-offsets.c
> +++ b/lib/s390x/asm-offsets.c
> @@ -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);
> diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
> index 96cca2e..07d4e5e 100644
> --- a/lib/s390x/asm/arch_def.h
> +++ b/lib/s390x/asm/arch_def.h
> @@ -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 */
> diff --git a/lib/s390x/interrupt.c b/lib/s390x/interrupt.c
> index 5cade23..c9e2dc6 100644
> --- a/lib/s390x/interrupt.c
> +++ b/lib/s390x/interrupt.c
> @@ -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;
>   }
>   
> diff --git a/lib/s390x/smp.c b/lib/s390x/smp.c
> index 7602886..f57f420 100644
> --- a/lib/s390x/smp.c
> +++ b/lib/s390x/smp.c
> @@ -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);
> diff --git a/s390x/cstart64.S b/s390x/cstart64.S
> index 8e2b21e..0455591 100644
> --- a/s390x/cstart64.S
> +++ b/s390x/cstart64.S
> @@ -93,7 +93,7 @@ memsetxc:
>   	/* save grs 0-15 */
>   	stmg	%r0, %r15, GEN_LC_SW_INT_GRS
>   	/* save cr0 */

Comment needs an update

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

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

>   	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

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

Apart from that looks good.

-- 

Thanks,

David / dhildenb

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

[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 <frankja@linux.ibm.com>
---
 lib/s390x/asm-offsets.c  |  1 +
 lib/s390x/asm/arch_def.h |  3 ++-
 s390x/cstart64.S         | 20 ++++++++++++++------
 3 files changed, 17 insertions(+), 7 deletions(-)

diff --git a/lib/s390x/asm-offsets.c b/lib/s390x/asm-offsets.c
index 4b213f8..61d2658 100644
--- a/lib/s390x/asm-offsets.c
+++ b/lib/s390x/asm-offsets.c
@@ -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);
diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
index 07d4e5e..7d25e4f 100644
--- a/lib/s390x/asm/arch_def.h
+++ b/lib/s390x/asm/arch_def.h
@@ -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 */
diff --git a/s390x/cstart64.S b/s390x/cstart64.S
index 0455591..2e0dcf5 100644
--- a/s390x/cstart64.S
+++ b/s390x/cstart64.S
@@ -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:
-- 
2.20.1

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

[David Hildenbrand]

On 05.11.19 17:28, Janosch Frank wrote:

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

> 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 <frankja@linux.ibm.com>
> ---
>   lib/s390x/asm-offsets.c  |  1 +
>   lib/s390x/asm/arch_def.h |  3 ++-
>   s390x/cstart64.S         | 20 ++++++++++++++------
>   3 files changed, 17 insertions(+), 7 deletions(-)
> 
> diff --git a/lib/s390x/asm-offsets.c b/lib/s390x/asm-offsets.c
> index 4b213f8..61d2658 100644
> --- a/lib/s390x/asm-offsets.c
> +++ b/lib/s390x/asm-offsets.c
> @@ -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);
> diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
> index 07d4e5e..7d25e4f 100644
> --- a/lib/s390x/asm/arch_def.h
> +++ b/lib/s390x/asm/arch_def.h
> @@ -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 */
> diff --git a/s390x/cstart64.S b/s390x/cstart64.S
> index 0455591..2e0dcf5 100644
> --- a/s390x/cstart64.S
> +++ b/s390x/cstart64.S
> @@ -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.
  */

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


> +	/* 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.

> +	/* 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:
> 


-- 

Thanks,

David / dhildenb

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

[Janosch Frank]

[-- Attachment #1.1: Type: text/plain, Size: 3621 bytes --]

On 11/5/19 8:53 PM, David Hildenbrand wrote:
> On 05.11.19 17:28, Janosch Frank wrote:
> 
> In the subject "Disable" vs. "Remove" ?
> 
>> 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 <frankja@linux.ibm.com>
>> ---
>>   lib/s390x/asm-offsets.c  |  1 +
>>   lib/s390x/asm/arch_def.h |  3 ++-
>>   s390x/cstart64.S         | 20 ++++++++++++++------
>>   3 files changed, 17 insertions(+), 7 deletions(-)
>>
>> diff --git a/lib/s390x/asm-offsets.c b/lib/s390x/asm-offsets.c
>> index 4b213f8..61d2658 100644
>> --- a/lib/s390x/asm-offsets.c
>> +++ b/lib/s390x/asm-offsets.c
>> @@ -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);
>> diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
>> index 07d4e5e..7d25e4f 100644
>> --- a/lib/s390x/asm/arch_def.h
>> +++ b/lib/s390x/asm/arch_def.h
>> @@ -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 */
>> diff --git a/s390x/cstart64.S b/s390x/cstart64.S
>> index 0455591..2e0dcf5 100644
>> --- a/s390x/cstart64.S
>> +++ b/s390x/cstart64.S
>> @@ -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.
>   */
> 
>>   	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 ...
> 
> 
>> +	/* 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.

Sounds like a good idea, will do

> 
>> +	/* 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:
>>
> 
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Janosch Frank]

[-- Attachment #1.1: Type: text/plain, Size: 2980 bytes --]

On 11/5/19 5:28 PM, Janosch Frank wrote:
> 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 <frankja@linux.ibm.com>
> ---
>  lib/s390x/asm-offsets.c  |  1 +
>  lib/s390x/asm/arch_def.h |  3 ++-
>  s390x/cstart64.S         | 20 ++++++++++++++------
>  3 files changed, 17 insertions(+), 7 deletions(-)
> 
> diff --git a/lib/s390x/asm-offsets.c b/lib/s390x/asm-offsets.c
> index 4b213f8..61d2658 100644
> --- a/lib/s390x/asm-offsets.c
> +++ b/lib/s390x/asm-offsets.c
> @@ -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);
> diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
> index 07d4e5e..7d25e4f 100644
> --- a/lib/s390x/asm/arch_def.h
> +++ b/lib/s390x/asm/arch_def.h
> @@ -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...

>  	uint64_t	mcck_ext_sa_addr;		/* 0x11b0 */
>  	uint8_t		pad_0x11b8[0x1200 - 0x11b8];	/* 0x11b8 */
>  	uint64_t	fprs_sa[16];			/* 0x1200 */
> diff --git a/s390x/cstart64.S b/s390x/cstart64.S
> index 0455591..2e0dcf5 100644
> --- a/s390x/cstart64.S
> +++ b/s390x/cstart64.S
> @@ -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:
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [kvm-unit-tests PATCH 0/2] s390x: Improve architectural compliance for diag308

[David Hildenbrand]

> Am 05.11.2019 um 17:29 schrieb Janosch Frank <frankja@linux.ibm.com>:
> 
> 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.

> 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(-)
> 
> -- 
> 2.20.1
> 

Re: [kvm-unit-tests PATCH 0/2] s390x: Improve architectural compliance for diag308

[Christian Borntraeger]

On 05.11.19 18:34, David Hildenbrand wrote:
> 
> 
>> Am 05.11.2019 um 17:29 schrieb Janosch Frank <frankja@linux.ibm.com>:
>>
>> 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.

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.
> 
>> 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(-)
>>
>> -- 
>> 2.20.1
>>

Re: [kvm-unit-tests PATCH 0/2] s390x: Improve architectural compliance for diag308

[Janosch Frank]

[-- Attachment #1.1: Type: text/plain, Size: 1477 bytes --]

On 11/5/19 7:23 PM, Christian Borntraeger wrote:
> 
> 
> On 05.11.19 18:34, David Hildenbrand wrote:
>>
>>
>>> Am 05.11.2019 um 17:29 schrieb Janosch Frank <frankja@linux.ibm.com>:
>>>
>>> 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.
> 
> 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.

>>
>>> 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(-)
>>>
>>> -- 
>>> 2.20.1
>>>



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]