[kvm-unit-tests PATCH v2 0/8] s390x: Cleanup exception register save/restore and implement backtrace

[kvm-unit-tests PATCH v2 0/8] s390x: Cleanup exception register save/restore and implement backtrace

[Janosch Frank]

Having two sets of macros for saving and restoring registers on
exceptions doesn't seem optimal to me. Therefore this patch set
removes the old macros that use the lowcore as storage in favor of the
stack using ones. At the same time we move over to generated offsets
instead of subtracting from the stack piece by piece. Changes to the
stack struct are easier that way.

Additionally let's add backtrace support and print the GRs on
exception so we get a bit more information when something goes wrong.

v2:
	* Added full CR saving to fix diag308 test
	* Added rev-bys

Janosch Frank (8):
  s390x: Fix fpc store address in RESTORE_REGS_STACK
  s390x: Fully commit to stack save area for exceptions
  RFC: s390x: Define STACK_FRAME_INT_SIZE macro
  s390x: Introduce and use CALL_INT_HANDLER macro
  s390x: Provide preliminary backtrace support
  s390x: Print more information on program exceptions
  s390x: Move diag308_load_reset to stack saving
  s390x: Remove SAVE/RESTORE_stack

 lib/s390x/asm-offsets.c   | 15 +++++--
 lib/s390x/asm/arch_def.h  | 29 ++++++++++---
 lib/s390x/asm/interrupt.h |  4 +-
 lib/s390x/interrupt.c     | 43 +++++++++++++++---
 lib/s390x/stack.c         | 20 ++++++---
 s390x/Makefile            |  1 +
 s390x/cpu.S               |  6 ++-
 s390x/cstart64.S          | 25 +++--------
 s390x/macros.S            | 91 +++++++++++++++++++--------------------
 9 files changed, 140 insertions(+), 94 deletions(-)

-- 
2.25.1

[kvm-unit-tests PATCH v2 1/8] s390x: Fix fpc store address in RESTORE_REGS_STACK

[Janosch Frank]

The efpc stores in bits 32-63 of a register and we store a full 8
bytes to have the stack 8 byte aligned. This means that the fpc is
stored at offset 4 but we load it from offset 0. Lets replace efpc
with stfpc and get rid of the stg to store at offset 0.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
Reviewed-by: Pierre Morel <pmorel@linux.ibm.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
---
 s390x/macros.S | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/s390x/macros.S b/s390x/macros.S
index 37a6a63e..e51a557a 100644
--- a/s390x/macros.S
+++ b/s390x/macros.S
@@ -54,8 +54,7 @@
 	.endr
 	/* Save fpc, but keep stack aligned on 64bits */
 	slgfi   %r15, 8
-	efpc	%r0
-	stg	%r0, 0(%r15)
+	stfpc	0(%r15)
 	.endm
 
 /* Restore the register in reverse order */
-- 
2.25.1

[kvm-unit-tests PATCH v2 2/8] s390x: Fully commit to stack save area for exceptions

[Janosch Frank]

Having two sets of macros for saving registers on exceptions makes
maintaining harder. Also we have limited space in the lowcore to save
stuff and by using the stack as a save area, we can stack exceptions.

So let's use the SAVE/RESTORE_REGS_STACK as the default. When we also
move the diag308 macro over we can remove the old SAVE/RESTORE_REGS
macros.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
---
 lib/s390x/asm-offsets.c   | 14 ++++++++----
 lib/s390x/asm/arch_def.h  | 29 ++++++++++++++++++-----
 lib/s390x/asm/interrupt.h |  4 ++--
 lib/s390x/interrupt.c     | 14 ++++++------
 s390x/cstart64.S          | 19 +++++++++-------
 s390x/macros.S            | 48 +++++++++++++++++++++++----------------
 6 files changed, 82 insertions(+), 46 deletions(-)

diff --git a/lib/s390x/asm-offsets.c b/lib/s390x/asm-offsets.c
index a19f14b9..96cb21cf 100644
--- a/lib/s390x/asm-offsets.c
+++ b/lib/s390x/asm-offsets.c
@@ -70,16 +70,22 @@ int main(void)
 	OFFSET(GEN_LC_ARS_SA, lowcore, ars_sa);
 	OFFSET(GEN_LC_CRS_SA, lowcore, crs_sa);
 	OFFSET(GEN_LC_PGM_INT_TDB, lowcore, pgm_int_tdb);
-	OFFSET(__SF_SIE_CONTROL, stack_frame, empty1[0]);
-	OFFSET(__SF_SIE_SAVEAREA, stack_frame, empty1[1]);
-	OFFSET(__SF_SIE_REASON, stack_frame, empty1[2]);
-	OFFSET(__SF_SIE_FLAGS, stack_frame, empty1[3]);
+	OFFSET(__SF_SIE_CONTROL, stack_frame, argument_area[0]);
+	OFFSET(__SF_SIE_SAVEAREA, stack_frame, argument_area[1]);
+	OFFSET(__SF_SIE_REASON, stack_frame, argument_area[2]);
+	OFFSET(__SF_SIE_FLAGS, stack_frame, argument_area[3]);
 	OFFSET(SIE_SAVEAREA_HOST_GRS, vm_save_area, host.grs[0]);
 	OFFSET(SIE_SAVEAREA_HOST_FPRS, vm_save_area, host.fprs[0]);
 	OFFSET(SIE_SAVEAREA_HOST_FPC, vm_save_area, host.fpc);
 	OFFSET(SIE_SAVEAREA_GUEST_GRS, vm_save_area, guest.grs[0]);
 	OFFSET(SIE_SAVEAREA_GUEST_FPRS, vm_save_area, guest.fprs[0]);
 	OFFSET(SIE_SAVEAREA_GUEST_FPC, vm_save_area, guest.fpc);
+	OFFSET(STACK_FRAME_INT_BACKCHAIN, stack_frame_int, back_chain);
+	OFFSET(STACK_FRAME_INT_FPC, stack_frame_int, fpc);
+	OFFSET(STACK_FRAME_INT_FPRS, stack_frame_int, fprs);
+	OFFSET(STACK_FRAME_INT_CRS, stack_frame_int, crs);
+	OFFSET(STACK_FRAME_INT_GRS0, stack_frame_int, grs0);
+	OFFSET(STACK_FRAME_INT_GRS1, stack_frame_int, grs1);
 
 	return 0;
 }
diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
index 9c4e330a..31c2fc66 100644
--- a/lib/s390x/asm/arch_def.h
+++ b/lib/s390x/asm/arch_def.h
@@ -8,13 +8,30 @@
 #ifndef _ASM_S390X_ARCH_DEF_H_
 #define _ASM_S390X_ARCH_DEF_H_
 
-/*
- * We currently only specify the stack frame members needed for the
- * SIE library code.
- */
 struct stack_frame {
-	unsigned long back_chain;
-	unsigned long empty1[5];
+	struct stack_frame *back_chain;
+	u64 reserved;
+	/* GRs 2 - 5 */
+	unsigned long argument_area[4];
+	/* GRs 6 - 15 */
+	unsigned long grs[10];
+	/* FPRs 0, 2, 4, 6 */
+	s64  fprs[4];
+};
+
+struct stack_frame_int {
+	struct stack_frame *back_chain;
+	u64 reserved;
+	/*
+	 * The GRs are offset compatible with struct stack_frame so we
+	 * can easily fetch GR14 for backtraces.
+	 */
+	u64 grs0[14];
+	u64 grs1[2];
+	u32 res;
+	u32 fpc;
+	u64 fprs[16];
+	u64 crs[16];
 };
 
 struct psw {
diff --git a/lib/s390x/asm/interrupt.h b/lib/s390x/asm/interrupt.h
index 1a2e2cd8..31e4766d 100644
--- a/lib/s390x/asm/interrupt.h
+++ b/lib/s390x/asm/interrupt.h
@@ -14,8 +14,8 @@
 #define EXT_IRQ_SERVICE_SIG	0x2401
 
 void register_pgm_cleanup_func(void (*f)(void));
-void handle_pgm_int(void);
-void handle_ext_int(void);
+void handle_pgm_int(struct stack_frame_int *stack);
+void handle_ext_int(struct stack_frame_int *stack);
 void handle_mcck_int(void);
 void handle_io_int(void);
 void handle_svc_int(void);
diff --git a/lib/s390x/interrupt.c b/lib/s390x/interrupt.c
index 1ce36073..a59df80e 100644
--- a/lib/s390x/interrupt.c
+++ b/lib/s390x/interrupt.c
@@ -56,7 +56,7 @@ void register_pgm_cleanup_func(void (*f)(void))
 	pgm_cleanup_func = f;
 }
 
-static void fixup_pgm_int(void)
+static void fixup_pgm_int(struct stack_frame_int *stack)
 {
 	/* If we have an error on SIE we directly move to sie_exit */
 	if (lc->pgm_old_psw.addr >= (uint64_t)&sie_entry &&
@@ -76,7 +76,7 @@ static void fixup_pgm_int(void)
 		/* Handling for iep.c test case. */
 		if (lc->trans_exc_id & 0x80UL && lc->trans_exc_id & 0x04UL &&
 		    !(lc->trans_exc_id & 0x08UL))
-			lc->pgm_old_psw.addr = lc->sw_int_grs[14];
+			lc->pgm_old_psw.addr = stack->grs0[12];
 		break;
 	case PGM_INT_CODE_SEGMENT_TRANSLATION:
 	case PGM_INT_CODE_PAGE_TRANSLATION:
@@ -115,7 +115,7 @@ static void fixup_pgm_int(void)
 	/* suppressed/terminated/completed point already at the next address */
 }
 
-void handle_pgm_int(void)
+void handle_pgm_int(struct stack_frame_int *stack)
 {
 	if (!pgm_int_expected) {
 		/* Force sclp_busy to false, otherwise we will loop forever */
@@ -130,10 +130,10 @@ void handle_pgm_int(void)
 	if (pgm_cleanup_func)
 		(*pgm_cleanup_func)();
 	else
-		fixup_pgm_int();
+		fixup_pgm_int(stack);
 }
 
-void handle_ext_int(void)
+void handle_ext_int(struct stack_frame_int *stack)
 {
 	if (!ext_int_expected &&
 	    lc->ext_int_code != EXT_IRQ_SERVICE_SIG) {
@@ -143,13 +143,13 @@ void handle_ext_int(void)
 	}
 
 	if (lc->ext_int_code == EXT_IRQ_SERVICE_SIG) {
-		lc->sw_int_crs[0] &= ~(1UL << 9);
+		stack->crs[0] &= ~(1UL << 9);
 		sclp_handle_ext();
 	} else {
 		ext_int_expected = false;
 	}
 
-	if (!(lc->sw_int_crs[0] & CR0_EXTM_MASK))
+	if (!(stack->crs[0] & CR0_EXTM_MASK))
 		lc->ext_old_psw.mask &= ~PSW_MASK_EXT;
 }
 
diff --git a/s390x/cstart64.S b/s390x/cstart64.S
index ace0c0d9..35d20293 100644
--- a/s390x/cstart64.S
+++ b/s390x/cstart64.S
@@ -92,33 +92,36 @@ memsetxc:
 
 .section .text
 pgm_int:
-	SAVE_REGS
+	SAVE_REGS_STACK
+	lgr     %r2, %r15
 	brasl	%r14, handle_pgm_int
-	RESTORE_REGS
+	RESTORE_REGS_STACK
 	lpswe	GEN_LC_PGM_OLD_PSW
 
 ext_int:
-	SAVE_REGS
+	SAVE_REGS_STACK
+	lgr     %r2, %r15
 	brasl	%r14, handle_ext_int
-	RESTORE_REGS
+	RESTORE_REGS_STACK
 	lpswe	GEN_LC_EXT_OLD_PSW
 
 mcck_int:
-	SAVE_REGS
+	SAVE_REGS_STACK
 	brasl	%r14, handle_mcck_int
-	RESTORE_REGS
+	RESTORE_REGS_STACK
 	lpswe	GEN_LC_MCCK_OLD_PSW
 
 io_int:
 	SAVE_REGS_STACK
+	lgr     %r2, %r15
 	brasl	%r14, handle_io_int
 	RESTORE_REGS_STACK
 	lpswe	GEN_LC_IO_OLD_PSW
 
 svc_int:
-	SAVE_REGS
+	SAVE_REGS_STACK
 	brasl	%r14, handle_svc_int
-	RESTORE_REGS
+	RESTORE_REGS_STACK
 	lpswe	GEN_LC_SVC_OLD_PSW
 
 	.align	8
diff --git a/s390x/macros.S b/s390x/macros.S
index e51a557a..d7eeeb55 100644
--- a/s390x/macros.S
+++ b/s390x/macros.S
@@ -3,9 +3,10 @@
  * s390x assembly macros
  *
  * Copyright (c) 2017 Red Hat Inc
- * Copyright (c) 2020 IBM Corp.
+ * Copyright (c) 2020, 2021 IBM Corp.
  *
  * Authors:
+ *  Janosch Frank <frankja@linux.ibm.com>
  *  Pierre Morel <pmorel@linux.ibm.com>
  *  David Hildenbrand <david@redhat.com>
  */
@@ -41,36 +42,45 @@
 
 /* Save registers on the stack (r15), so we can have stacked interrupts. */
 	.macro SAVE_REGS_STACK
-	/* Allocate a stack frame for 15 general registers */
-	slgfi   %r15, 15 * 8
+	/* Allocate a full stack frame */
+	slgfi   %r15, 32 * 8 + 4 * 8
 	/* Store registers r0 to r14 on the stack */
-	stmg    %r0, %r14, 0(%r15)
-	/* Allocate a stack frame for 16 floating point registers */
-	/* The size of a FP register is the size of an double word */
-	slgfi   %r15, 16 * 8
+	stmg    %r2, %r15, STACK_FRAME_INT_GRS0(%r15)
+	stg     %r0, STACK_FRAME_INT_GRS1(%r15)
+	stg     %r1, STACK_FRAME_INT_GRS1 + 8(%r15)
+	/* Store the gr15 value before we allocated the new stack */
+	lgr     %r0, %r15
+	algfi   %r0, 32 * 8 + 4 * 8
+	stg     %r0, 13 * 8 + STACK_FRAME_INT_GRS0(%r15)
+	stg     %r0, STACK_FRAME_INT_BACKCHAIN(%r15)
+	/*
+	 * Store CR0 and load initial CR0 so AFP is active and we can
+	 * access all fprs to save them.
+	 */
+	stctg   %c0,%c15,STACK_FRAME_INT_CRS(%r15)
+	larl	%r1, initial_cr0
+	lctlg	%c0, %c0, 0(%r1)
 	/* Save fp register on stack: offset to SP is multiple of reg number */
 	.irp i, 0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15
-	std	\i, \i * 8(%r15)
+	std	\i, \i * 8 + STACK_FRAME_INT_FPRS(%r15)
 	.endr
-	/* Save fpc, but keep stack aligned on 64bits */
-	slgfi   %r15, 8
-	stfpc	0(%r15)
+	/* Save fpc */
+	stfpc	STACK_FRAME_INT_FPC(%r15)
 	.endm
 
 /* Restore the register in reverse order */
 	.macro RESTORE_REGS_STACK
 	/* Restore fpc */
-	lfpc	0(%r15)
-	algfi	%r15, 8
+	lfpc	STACK_FRAME_INT_FPC(%r15)
 	/* Restore fp register from stack: SP still where it was left */
 	/* and offset to SP is a multiple of reg number */
 	.irp i, 0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15
-	ld	\i, \i * 8(%r15)
+	ld	\i, \i * 8 + STACK_FRAME_INT_FPRS(%r15)
 	.endr
-	/* Now that we're done, rewind the stack pointer by 16 double word */
-	algfi   %r15, 16 * 8
+	/* Load CR0 back */
+	lctlg	%c0, %c15, STACK_FRAME_INT_CRS(%r15)
 	/* Load the registers from stack */
-	lmg     %r0, %r14, 0(%r15)
-	/* Rewind the stack by 15 double word */
-	algfi   %r15, 15 * 8
+	lg      %r0, STACK_FRAME_INT_GRS1(%r15)
+	lg      %r1, STACK_FRAME_INT_GRS1 + 8(%r15)
+	lmg     %r2, %r15, STACK_FRAME_INT_GRS0(%r15)
 	.endm
-- 
2.25.1

[kvm-unit-tests PATCH v2 3/8] RFC: s390x: Define STACK_FRAME_INT_SIZE macro

[Janosch Frank]

Using sizeof is safer than using magic constants. However, it doesn't
really fit into asm-offsets.h as it's not an offset so I'm happy to
receive suggestions on where to put it.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 lib/s390x/asm-offsets.c | 1 +
 s390x/macros.S          | 4 ++--
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/lib/s390x/asm-offsets.c b/lib/s390x/asm-offsets.c
index 96cb21cf..2658b59a 100644
--- a/lib/s390x/asm-offsets.c
+++ b/lib/s390x/asm-offsets.c
@@ -86,6 +86,7 @@ int main(void)
 	OFFSET(STACK_FRAME_INT_CRS, stack_frame_int, crs);
 	OFFSET(STACK_FRAME_INT_GRS0, stack_frame_int, grs0);
 	OFFSET(STACK_FRAME_INT_GRS1, stack_frame_int, grs1);
+	DEFINE(STACK_FRAME_INT_SIZE, sizeof(struct stack_frame_int));
 
 	return 0;
 }
diff --git a/s390x/macros.S b/s390x/macros.S
index d7eeeb55..a7d62c6f 100644
--- a/s390x/macros.S
+++ b/s390x/macros.S
@@ -43,14 +43,14 @@
 /* Save registers on the stack (r15), so we can have stacked interrupts. */
 	.macro SAVE_REGS_STACK
 	/* Allocate a full stack frame */
-	slgfi   %r15, 32 * 8 + 4 * 8
+	slgfi   %r15, STACK_FRAME_INT_SIZE
 	/* Store registers r0 to r14 on the stack */
 	stmg    %r2, %r15, STACK_FRAME_INT_GRS0(%r15)
 	stg     %r0, STACK_FRAME_INT_GRS1(%r15)
 	stg     %r1, STACK_FRAME_INT_GRS1 + 8(%r15)
 	/* Store the gr15 value before we allocated the new stack */
 	lgr     %r0, %r15
-	algfi   %r0, 32 * 8 + 4 * 8
+	algfi   %r0, STACK_FRAME_INT_SIZE
 	stg     %r0, 13 * 8 + STACK_FRAME_INT_GRS0(%r15)
 	stg     %r0, STACK_FRAME_INT_BACKCHAIN(%r15)
 	/*
-- 
2.25.1

[kvm-unit-tests PATCH v2 4/8] s390x: Introduce and use CALL_INT_HANDLER macro

[Janosch Frank]

The ELF ABI dictates that we need to allocate 160 bytes of stack space
for the C functions we're calling. Since we would need to do that for
every interruption handler which, combined with the new stack argument
being saved in GR2, makes cstart64.S look a bit messy.

So let's introduce the CALL_INT_HANDLER macro that handles all of
that, calls the C interrupt handler and handles cleanup afterwards.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 s390x/cstart64.S | 28 +++++-----------------------
 s390x/macros.S   | 17 +++++++++++++++++
 2 files changed, 22 insertions(+), 23 deletions(-)

diff --git a/s390x/cstart64.S b/s390x/cstart64.S
index 35d20293..666a9567 100644
--- a/s390x/cstart64.S
+++ b/s390x/cstart64.S
@@ -92,37 +92,19 @@ memsetxc:
 
 .section .text
 pgm_int:
-	SAVE_REGS_STACK
-	lgr     %r2, %r15
-	brasl	%r14, handle_pgm_int
-	RESTORE_REGS_STACK
-	lpswe	GEN_LC_PGM_OLD_PSW
+	CALL_INT_HANDLER handle_pgm_int, GEN_LC_PGM_OLD_PSW
 
 ext_int:
-	SAVE_REGS_STACK
-	lgr     %r2, %r15
-	brasl	%r14, handle_ext_int
-	RESTORE_REGS_STACK
-	lpswe	GEN_LC_EXT_OLD_PSW
+	CALL_INT_HANDLER handle_ext_int, GEN_LC_EXT_OLD_PSW
 
 mcck_int:
-	SAVE_REGS_STACK
-	brasl	%r14, handle_mcck_int
-	RESTORE_REGS_STACK
-	lpswe	GEN_LC_MCCK_OLD_PSW
+	CALL_INT_HANDLER handle_mcck_int, GEN_LC_MCCK_OLD_PSW
 
 io_int:
-	SAVE_REGS_STACK
-	lgr     %r2, %r15
-	brasl	%r14, handle_io_int
-	RESTORE_REGS_STACK
-	lpswe	GEN_LC_IO_OLD_PSW
+	CALL_INT_HANDLER handle_io_int, GEN_LC_IO_OLD_PSW
 
 svc_int:
-	SAVE_REGS_STACK
-	brasl	%r14, handle_svc_int
-	RESTORE_REGS_STACK
-	lpswe	GEN_LC_SVC_OLD_PSW
+	CALL_INT_HANDLER handle_svc_int, GEN_LC_SVC_OLD_PSW
 
 	.align	8
 initial_psw:
diff --git a/s390x/macros.S b/s390x/macros.S
index a7d62c6f..212a3823 100644
--- a/s390x/macros.S
+++ b/s390x/macros.S
@@ -11,6 +11,23 @@
  *  David Hildenbrand <david@redhat.com>
  */
 #include <asm/asm-offsets.h>
+/*
+ * Exception handler macro that saves registers on the stack,
+ * allocates stack space and calls the C handler function. Afterwards
+ * we re-load the registers and load the old PSW.
+ */
+	.macro CALL_INT_HANDLER c_func, old_psw
+	SAVE_REGS_STACK
+	/* Save the stack address in GR2 which is the first function argument */
+	lgr     %r2, %r15
+	/* Allocate stack pace for called C function, as specified in s390 ELF ABI */
+	slgfi   %r15, 160
+	brasl	%r14, \c_func
+	algfi   %r15, 160
+	RESTORE_REGS_STACK
+	lpswe	\old_psw
+	.endm
+
 	.macro SAVE_REGS
 	/* save grs 0-15 */
 	stmg	%r0, %r15, GEN_LC_SW_INT_GRS
-- 
2.25.1

[kvm-unit-tests PATCH v2 5/8] s390x: Provide preliminary backtrace support

[Janosch Frank]

After the stack changes we can finally use -mbackchain and have a
working backtrace.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 lib/s390x/interrupt.c | 12 ++++++++++++
 lib/s390x/stack.c     | 20 ++++++++++++++------
 s390x/Makefile        |  1 +
 3 files changed, 27 insertions(+), 6 deletions(-)

diff --git a/lib/s390x/interrupt.c b/lib/s390x/interrupt.c
index a59df80e..23ad922c 100644
--- a/lib/s390x/interrupt.c
+++ b/lib/s390x/interrupt.c
@@ -115,6 +115,18 @@ static void fixup_pgm_int(struct stack_frame_int *stack)
 	/* suppressed/terminated/completed point already at the next address */
 }
 
+static void print_pgm_info(struct stack_frame_int *stack)
+
+{
+	printf("\n");
+	printf("Unexpected program interrupt: %d on cpu %d at %#lx, ilen %d\n",
+	       lc->pgm_int_code, stap(), lc->pgm_old_psw.addr,
+	       lc->pgm_int_id);
+	dump_stack();
+	report_summary();
+	abort();
+}
+
 void handle_pgm_int(struct stack_frame_int *stack)
 {
 	if (!pgm_int_expected) {
diff --git a/lib/s390x/stack.c b/lib/s390x/stack.c
index 0fcd1afb..4cf80dae 100644
--- a/lib/s390x/stack.c
+++ b/lib/s390x/stack.c
@@ -3,24 +3,32 @@
  * s390x stack implementation
  *
  * Copyright (c) 2017 Red Hat Inc
+ * Copyright 2021 IBM Corp
  *
  * Authors:
  *  Thomas Huth <thuth@redhat.com>
  *  David Hildenbrand <david@redhat.com>
+ *  Janosch Frank <frankja@de.ibm.com>
  */
 #include <libcflat.h>
 #include <stack.h>
+#include <asm/arch_def.h>
 
 int backtrace_frame(const void *frame, const void **return_addrs, int max_depth)
 {
-	printf("TODO: Implement backtrace_frame(%p, %p, %d) function!\n",
-	       frame, return_addrs, max_depth);
-	return 0;
+	int depth = 0;
+	struct stack_frame *stack = (struct stack_frame *)frame;
+
+	for (depth = 0; stack && depth < max_depth; depth++) {
+		return_addrs[depth] = (void *)stack->grs[8];
+		stack = stack->back_chain;
+	}
+
+	return depth;
 }
 
 int backtrace(const void **return_addrs, int max_depth)
 {
-	printf("TODO: Implement backtrace(%p, %d) function!\n",
-	       return_addrs, max_depth);
-	return 0;
+	return backtrace_frame(__builtin_frame_address(0),
+			       return_addrs, max_depth);
 }
diff --git a/s390x/Makefile b/s390x/Makefile
index f3b0fccf..20bb5683 100644
--- a/s390x/Makefile
+++ b/s390x/Makefile
@@ -39,6 +39,7 @@ CFLAGS += -ffreestanding
 CFLAGS += -I $(SRCDIR)/lib -I $(SRCDIR)/lib/s390x -I lib
 CFLAGS += -O2
 CFLAGS += -march=zEC12
+CFLAGS += -mbackchain
 CFLAGS += -fno-delete-null-pointer-checks
 LDFLAGS += -nostdlib -Wl,--build-id=none
 
-- 
2.25.1

[kvm-unit-tests PATCH v2 6/8] s390x: Print more information on program exceptions

[Janosch Frank]

Currently we only get a single line of output if a test runs in a
unexpected program exception. Let's also print the general registers
to give soem more context.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
Acked-by: Pierre Morel <pmorel@linux.ibm.com>
---
 lib/s390x/interrupt.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/lib/s390x/interrupt.c b/lib/s390x/interrupt.c
index 23ad922c..22649d04 100644
--- a/lib/s390x/interrupt.c
+++ b/lib/s390x/interrupt.c
@@ -115,6 +115,21 @@ static void fixup_pgm_int(struct stack_frame_int *stack)
 	/* suppressed/terminated/completed point already at the next address */
 }
 
+static void print_int_regs(struct stack_frame_int *stack)
+{
+	printf("\n");
+	printf("GPRS:\n");
+	printf("%016lx %016lx %016lx %016lx\n",
+	       stack->grs1[0], stack->grs1[1], stack->grs0[0], stack->grs0[1]);
+	printf("%016lx %016lx %016lx %016lx\n",
+	       stack->grs0[2], stack->grs0[3], stack->grs0[4], stack->grs0[5]);
+	printf("%016lx %016lx %016lx %016lx\n",
+	       stack->grs0[6], stack->grs0[7], stack->grs0[8], stack->grs0[9]);
+	printf("%016lx %016lx %016lx %016lx\n",
+	       stack->grs0[10], stack->grs0[11], stack->grs0[12], stack->grs0[13]);
+	printf("\n");
+}
+
 static void print_pgm_info(struct stack_frame_int *stack)
 
 {
@@ -122,6 +137,7 @@ static void print_pgm_info(struct stack_frame_int *stack)
 	printf("Unexpected program interrupt: %d on cpu %d at %#lx, ilen %d\n",
 	       lc->pgm_int_code, stap(), lc->pgm_old_psw.addr,
 	       lc->pgm_int_id);
+	print_int_regs(stack);
 	dump_stack();
 	report_summary();
 	abort();
@@ -132,6 +148,7 @@ void handle_pgm_int(struct stack_frame_int *stack)
 	if (!pgm_int_expected) {
 		/* Force sclp_busy to false, otherwise we will loop forever */
 		sclp_handle_ext();
+		print_pgm_info(stack);
 		report_abort("Unexpected program interrupt: %d on cpu %d at %#lx, ilen %d\n",
 			     lc->pgm_int_code, stap(), lc->pgm_old_psw.addr,
 			     lc->pgm_int_id);
-- 
2.25.1

[kvm-unit-tests PATCH v2 7/8] s390x: Move diag308_load_reset to stack saving

[Janosch Frank]

By moving the last user of SAVE/RESTORE_REGS to the macros that use
the stack we can finally remove these macros.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
Reviewed-by: Pierre Morel <pmorel@linux.ibm.com>
---
 s390x/cpu.S | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/s390x/cpu.S b/s390x/cpu.S
index 5267f029..e2ad56c8 100644
--- a/s390x/cpu.S
+++ b/s390x/cpu.S
@@ -18,7 +18,7 @@
  */
 .globl diag308_load_reset
 diag308_load_reset:
-	SAVE_REGS
+	SAVE_REGS_STACK
 	/* Backup current PSW mask, as we have to restore it on success */
 	epsw	%r0, %r1
 	st	%r0, GEN_LC_SW_INT_PSW
@@ -31,6 +31,7 @@ diag308_load_reset:
 	ogr	%r0, %r1
 	/* Store it at the reset PSW location (real 0x0) */
 	stg	%r0, 0
+	stg     %r15, GEN_LC_SW_INT_GRS + 15 * 8
 	/* Do the reset */
 	diag    %r0,%r2,0x308
 	/* Failure path */
@@ -40,7 +41,8 @@ diag308_load_reset:
 	/* load a cr0 that has the AFP control bit which enables all FPRs */
 0:	larl	%r1, initial_cr0
 	lctlg	%c0, %c0, 0(%r1)
-	RESTORE_REGS
+	lg      %r15, GEN_LC_SW_INT_GRS + 15 * 8
+	RESTORE_REGS_STACK
 	lhi	%r2, 1
 	larl	%r0, 1f
 	stg	%r0, GEN_LC_SW_INT_PSW + 8
-- 
2.25.1

[kvm-unit-tests PATCH v2 8/8] s390x: Remove SAVE/RESTORE_stack

[Janosch Frank]

There are no more users.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
Reviewed-by: Pierre Morel <pmorel@linux.ibm.com>
---
 s390x/macros.S | 29 -----------------------------
 1 file changed, 29 deletions(-)

diff --git a/s390x/macros.S b/s390x/macros.S
index 212a3823..399a87c6 100644
--- a/s390x/macros.S
+++ b/s390x/macros.S
@@ -28,35 +28,6 @@
 	lpswe	\old_psw
 	.endm
 
-	.macro SAVE_REGS
-	/* save grs 0-15 */
-	stmg	%r0, %r15, GEN_LC_SW_INT_GRS
-	/* save crs 0-15 */
-	stctg	%c0, %c15, GEN_LC_SW_INT_CRS
-	/* load a cr0 that has the AFP control bit which enables all FPRs */
-	larl	%r1, initial_cr0
-	lctlg	%c0, %c0, 0(%r1)
-	/* save fprs 0-15 + fpc */
-	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
-	std	\i, \i * 8(%r1)
-	.endr
-	stfpc	GEN_LC_SW_INT_FPC
-	.endm
-
-	.macro RESTORE_REGS
-	/* restore fprs 0-15 + fpc */
-	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 crs 0-15 */
-	lctlg	%c0, %c15, GEN_LC_SW_INT_CRS
-	/* restore grs 0-15 */
-	lmg	%r0, %r15, GEN_LC_SW_INT_GRS
-	.endm
-
 /* Save registers on the stack (r15), so we can have stacked interrupts. */
 	.macro SAVE_REGS_STACK
 	/* Allocate a full stack frame */
-- 
2.25.1

Re: [kvm-unit-tests PATCH v2 2/8] s390x: Fully commit to stack save area for exceptions

[Thomas Huth]

On 17/02/2021 15.41, Janosch Frank wrote:
> Having two sets of macros for saving registers on exceptions makes
> maintaining harder. Also we have limited space in the lowcore to save
> stuff and by using the stack as a save area, we can stack exceptions.
> 
> So let's use the SAVE/RESTORE_REGS_STACK as the default. When we also
> move the diag308 macro over we can remove the old SAVE/RESTORE_REGS
> macros.
[...]
> diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
> index 9c4e330a..31c2fc66 100644
> --- a/lib/s390x/asm/arch_def.h
> +++ b/lib/s390x/asm/arch_def.h
> @@ -8,13 +8,30 @@
>   #ifndef _ASM_S390X_ARCH_DEF_H_
>   #define _ASM_S390X_ARCH_DEF_H_
>   
> -/*
> - * We currently only specify the stack frame members needed for the
> - * SIE library code.
> - */
>   struct stack_frame {
> -	unsigned long back_chain;
> -	unsigned long empty1[5];
> +	struct stack_frame *back_chain;
> +	u64 reserved;
> +	/* GRs 2 - 5 */
> +	unsigned long argument_area[4];
> +	/* GRs 6 - 15 */
> +	unsigned long grs[10];
> +	/* FPRs 0, 2, 4, 6 */
> +	s64  fprs[4];
> +};

For consistency, could you please replace the "unsigned long" with u64, or 
even switch to uint64_t completely?

Currently, we have:

$ grep -r u64 lib/s390x/ | wc -l
8
$ grep -r uint64 lib/s390x/ | wc -l
94

... so uint64_t seems to be the better choice.

> +struct stack_frame_int {
> +	struct stack_frame *back_chain;
> +	u64 reserved;
> +	/*
> +	 * The GRs are offset compatible with struct stack_frame so we
> +	 * can easily fetch GR14 for backtraces.
> +	 */
> +	u64 grs0[14];
> +	u64 grs1[2];

Which registers go into grs0 and which ones into grs1? And why is there a 
split at all? A comment would be really helpful!

> +	u32 res;

res = reserved? Please add a comment.

> +	u32 fpc;
> +	u64 fprs[16];
> +	u64 crs[16];
>   };

Similar, switch to uint32_t and uint64_t ?

> diff --git a/s390x/macros.S b/s390x/macros.S
> index e51a557a..d7eeeb55 100644
> --- a/s390x/macros.S
> +++ b/s390x/macros.S
> @@ -3,9 +3,10 @@
>    * s390x assembly macros
>    *
>    * Copyright (c) 2017 Red Hat Inc
> - * Copyright (c) 2020 IBM Corp.
> + * Copyright (c) 2020, 2021 IBM Corp.
>    *
>    * Authors:
> + *  Janosch Frank <frankja@linux.ibm.com>
>    *  Pierre Morel <pmorel@linux.ibm.com>
>    *  David Hildenbrand <david@redhat.com>
>    */
> @@ -41,36 +42,45 @@
>   
>   /* Save registers on the stack (r15), so we can have stacked interrupts. */
>   	.macro SAVE_REGS_STACK
> -	/* Allocate a stack frame for 15 general registers */
> -	slgfi   %r15, 15 * 8
> +	/* Allocate a full stack frame */
> +	slgfi   %r15, 32 * 8 + 4 * 8

How did you come up with that number? That does neither match stack 
stack_frame nor stack_frame_int, if I got this right. Please add a comment 
to the code to explain the numbers.

>   	/* Store registers r0 to r14 on the stack */
> -	stmg    %r0, %r14, 0(%r15)
> -	/* Allocate a stack frame for 16 floating point registers */
> -	/* The size of a FP register is the size of an double word */
> -	slgfi   %r15, 16 * 8
> +	stmg    %r2, %r15, STACK_FRAME_INT_GRS0(%r15)

Storing up to r14 should be sufficent since you store r15 again below?

> +	stg     %r0, STACK_FRAME_INT_GRS1(%r15)
> +	stg     %r1, STACK_FRAME_INT_GRS1 + 8(%r15)
> +	/* Store the gr15 value before we allocated the new stack */
> +	lgr     %r0, %r15
> +	algfi   %r0, 32 * 8 + 4 * 8
> +	stg     %r0, 13 * 8 + STACK_FRAME_INT_GRS0(%r15)
> +	stg     %r0, STACK_FRAME_INT_BACKCHAIN(%r15)
> +	/*
> +	 * Store CR0 and load initial CR0 so AFP is active and we can
> +	 * access all fprs to save them.
> +	 */
> +	stctg   %c0,%c15,STACK_FRAME_INT_CRS(%r15)
> +	larl	%r1, initial_cr0
> +	lctlg	%c0, %c0, 0(%r1)
>   	/* Save fp register on stack: offset to SP is multiple of reg number */
>   	.irp i, 0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15
> -	std	\i, \i * 8(%r15)
> +	std	\i, \i * 8 + STACK_FRAME_INT_FPRS(%r15)
>   	.endr

So you saved 16 GRs, 16 CRs and 16 FPRs onto the stack, that's at least 16 * 
3 * 8 = 48 * 8 bytes ... but you only decreased the stack by 32 * 8 + 4 * 8 
bytes initially ... is this a bug, or do I miss something?

  Thomas

Re: [kvm-unit-tests PATCH v2 3/8] RFC: s390x: Define STACK_FRAME_INT_SIZE macro

[Thomas Huth]

On 17/02/2021 15.41, Janosch Frank wrote:
> Using sizeof is safer than using magic constants. However, it doesn't
> really fit into asm-offsets.h as it's not an offset so I'm happy to
> receive suggestions on where to put it.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>   lib/s390x/asm-offsets.c | 1 +
>   s390x/macros.S          | 4 ++--
>   2 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/s390x/asm-offsets.c b/lib/s390x/asm-offsets.c
> index 96cb21cf..2658b59a 100644
> --- a/lib/s390x/asm-offsets.c
> +++ b/lib/s390x/asm-offsets.c
> @@ -86,6 +86,7 @@ int main(void)
>   	OFFSET(STACK_FRAME_INT_CRS, stack_frame_int, crs);
>   	OFFSET(STACK_FRAME_INT_GRS0, stack_frame_int, grs0);
>   	OFFSET(STACK_FRAME_INT_GRS1, stack_frame_int, grs1);
> +	DEFINE(STACK_FRAME_INT_SIZE, sizeof(struct stack_frame_int));
>   
>   	return 0;
>   }
> diff --git a/s390x/macros.S b/s390x/macros.S
> index d7eeeb55..a7d62c6f 100644
> --- a/s390x/macros.S
> +++ b/s390x/macros.S
> @@ -43,14 +43,14 @@
>   /* Save registers on the stack (r15), so we can have stacked interrupts. */
>   	.macro SAVE_REGS_STACK
>   	/* Allocate a full stack frame */
> -	slgfi   %r15, 32 * 8 + 4 * 8
> +	slgfi   %r15, STACK_FRAME_INT_SIZE
>   	/* Store registers r0 to r14 on the stack */
>   	stmg    %r2, %r15, STACK_FRAME_INT_GRS0(%r15)
>   	stg     %r0, STACK_FRAME_INT_GRS1(%r15)
>   	stg     %r1, STACK_FRAME_INT_GRS1 + 8(%r15)
>   	/* Store the gr15 value before we allocated the new stack */
>   	lgr     %r0, %r15
> -	algfi   %r0, 32 * 8 + 4 * 8
> +	algfi   %r0, STACK_FRAME_INT_SIZE

Ah, well, that of course fixes the problem that I had with the previous 
patch. I'd suggest to merge it into patch 2.

  Thomas

Re: [kvm-unit-tests PATCH v2 4/8] s390x: Introduce and use CALL_INT_HANDLER macro

[Thomas Huth]

On 17/02/2021 15.41, Janosch Frank wrote:
> The ELF ABI dictates that we need to allocate 160 bytes of stack space
> for the C functions we're calling.

So this actually some kind of bug fix, since we should have allocated that 
area before already? Why did we never experienced any issues here so far? 
Are the called functions not using the save area?

> 
> diff --git a/s390x/cstart64.S b/s390x/cstart64.S
> index 35d20293..666a9567 100644
> --- a/s390x/cstart64.S
> +++ b/s390x/cstart64.S
> @@ -92,37 +92,19 @@ memsetxc:
>   
>   .section .text
>   pgm_int:
> -	SAVE_REGS_STACK
> -	lgr     %r2, %r15
> -	brasl	%r14, handle_pgm_int
> -	RESTORE_REGS_STACK
> -	lpswe	GEN_LC_PGM_OLD_PSW
> +	CALL_INT_HANDLER handle_pgm_int, GEN_LC_PGM_OLD_PSW
>   
>   ext_int:
> -	SAVE_REGS_STACK
> -	lgr     %r2, %r15
> -	brasl	%r14, handle_ext_int
> -	RESTORE_REGS_STACK
> -	lpswe	GEN_LC_EXT_OLD_PSW
> +	CALL_INT_HANDLER handle_ext_int, GEN_LC_EXT_OLD_PSW
>   
>   mcck_int:
> -	SAVE_REGS_STACK
> -	brasl	%r14, handle_mcck_int
> -	RESTORE_REGS_STACK
> -	lpswe	GEN_LC_MCCK_OLD_PSW
> +	CALL_INT_HANDLER handle_mcck_int, GEN_LC_MCCK_OLD_PSW
>   
>   io_int:
> -	SAVE_REGS_STACK
> -	lgr     %r2, %r15
> -	brasl	%r14, handle_io_int
> -	RESTORE_REGS_STACK
> -	lpswe	GEN_LC_IO_OLD_PSW
> +	CALL_INT_HANDLER handle_io_int, GEN_LC_IO_OLD_PSW
>   
>   svc_int:
> -	SAVE_REGS_STACK
> -	brasl	%r14, handle_svc_int
> -	RESTORE_REGS_STACK
> -	lpswe	GEN_LC_SVC_OLD_PSW
> +	CALL_INT_HANDLER handle_svc_int, GEN_LC_SVC_OLD_PSW
>   
>   	.align	8
>   initial_psw:
> diff --git a/s390x/macros.S b/s390x/macros.S
> index a7d62c6f..212a3823 100644
> --- a/s390x/macros.S
> +++ b/s390x/macros.S
> @@ -11,6 +11,23 @@
>    *  David Hildenbrand <david@redhat.com>
>    */
>   #include <asm/asm-offsets.h>
> +/*
> + * Exception handler macro that saves registers on the stack,
> + * allocates stack space and calls the C handler function. Afterwards
> + * we re-load the registers and load the old PSW.
> + */
> +	.macro CALL_INT_HANDLER c_func, old_psw
> +	SAVE_REGS_STACK
> +	/* Save the stack address in GR2 which is the first function argument */
> +	lgr     %r2, %r15
> +	/* Allocate stack pace for called C function, as specified in s390 ELF ABI */

s/pace/space/

> +	slgfi   %r15, 160
> +	brasl	%r14, \c_func
> +	algfi   %r15, 160
> +	RESTORE_REGS_STACK
> +	lpswe	\old_psw
> +	.endm

As long as the macro is only used in cstart64.S, I think you could also put 
it there (no strong opinion on this, though).

Anyway, with the typo fixed:
Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [kvm-unit-tests PATCH v2 4/8] s390x: Introduce and use CALL_INT_HANDLER macro

[Thomas Huth]

On 17/02/2021 15.41, Janosch Frank wrote:
> The ELF ABI dictates that we need to allocate 160 bytes of stack space
> for the C functions we're calling. Since we would need to do that for
> every interruption handler which, combined with the new stack argument
> being saved in GR2, makes cstart64.S look a bit messy.
> 
> So let's introduce the CALL_INT_HANDLER macro that handles all of
> that, calls the C interrupt handler and handles cleanup afterwards.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>   s390x/cstart64.S | 28 +++++-----------------------
>   s390x/macros.S   | 17 +++++++++++++++++
>   2 files changed, 22 insertions(+), 23 deletions(-)
> 
> diff --git a/s390x/cstart64.S b/s390x/cstart64.S
> index 35d20293..666a9567 100644
> --- a/s390x/cstart64.S
> +++ b/s390x/cstart64.S
> @@ -92,37 +92,19 @@ memsetxc:
>   
>   .section .text
>   pgm_int:
> -	SAVE_REGS_STACK
> -	lgr     %r2, %r15
> -	brasl	%r14, handle_pgm_int
> -	RESTORE_REGS_STACK
> -	lpswe	GEN_LC_PGM_OLD_PSW
> +	CALL_INT_HANDLER handle_pgm_int, GEN_LC_PGM_OLD_PSW
>   
>   ext_int:
> -	SAVE_REGS_STACK
> -	lgr     %r2, %r15
> -	brasl	%r14, handle_ext_int
> -	RESTORE_REGS_STACK
> -	lpswe	GEN_LC_EXT_OLD_PSW
> +	CALL_INT_HANDLER handle_ext_int, GEN_LC_EXT_OLD_PSW
>   
>   mcck_int:
> -	SAVE_REGS_STACK
> -	brasl	%r14, handle_mcck_int
> -	RESTORE_REGS_STACK
> -	lpswe	GEN_LC_MCCK_OLD_PSW
> +	CALL_INT_HANDLER handle_mcck_int, GEN_LC_MCCK_OLD_PSW
>   
>   io_int:
> -	SAVE_REGS_STACK
> -	lgr     %r2, %r15
> -	brasl	%r14, handle_io_int
> -	RESTORE_REGS_STACK
> -	lpswe	GEN_LC_IO_OLD_PSW
> +	CALL_INT_HANDLER handle_io_int, GEN_LC_IO_OLD_PSW
>   
>   svc_int:
> -	SAVE_REGS_STACK
> -	brasl	%r14, handle_svc_int
> -	RESTORE_REGS_STACK
> -	lpswe	GEN_LC_SVC_OLD_PSW
> +	CALL_INT_HANDLER handle_svc_int, GEN_LC_SVC_OLD_PSW
>   
>   	.align	8
>   initial_psw:
> diff --git a/s390x/macros.S b/s390x/macros.S
> index a7d62c6f..212a3823 100644
> --- a/s390x/macros.S
> +++ b/s390x/macros.S
> @@ -11,6 +11,23 @@
>    *  David Hildenbrand <david@redhat.com>
>    */
>   #include <asm/asm-offsets.h>
> +/*
> + * Exception handler macro that saves registers on the stack,
> + * allocates stack space and calls the C handler function. Afterwards
> + * we re-load the registers and load the old PSW.
> + */
> +	.macro CALL_INT_HANDLER c_func, old_psw
> +	SAVE_REGS_STACK
> +	/* Save the stack address in GR2 which is the first function argument */
> +	lgr     %r2, %r15
> +	/* Allocate stack pace for called C function, as specified in s390 ELF ABI */
> +	slgfi   %r15, 160

By the way, don't you have to store a back chain pointer at the bottom of 
that area, too, if you want to use -mbackchoin in the next patch?

  Thomas


> +	brasl	%r14, \c_func
> +	algfi   %r15, 160
> +	RESTORE_REGS_STACK
> +	lpswe	\old_psw
> +	.endm
> +
>   	.macro SAVE_REGS
>   	/* save grs 0-15 */
>   	stmg	%r0, %r15, GEN_LC_SW_INT_GRS
> 

Re: [kvm-unit-tests PATCH v2 5/8] s390x: Provide preliminary backtrace support

[Thomas Huth]

On 17/02/2021 15.41, Janosch Frank wrote:
> After the stack changes we can finally use -mbackchain and have a
> working backtrace.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>   lib/s390x/interrupt.c | 12 ++++++++++++
>   lib/s390x/stack.c     | 20 ++++++++++++++------
>   s390x/Makefile        |  1 +
>   3 files changed, 27 insertions(+), 6 deletions(-)
> 
> diff --git a/lib/s390x/interrupt.c b/lib/s390x/interrupt.c
> index a59df80e..23ad922c 100644
> --- a/lib/s390x/interrupt.c
> +++ b/lib/s390x/interrupt.c
> @@ -115,6 +115,18 @@ static void fixup_pgm_int(struct stack_frame_int *stack)
>   	/* suppressed/terminated/completed point already at the next address */
>   }
>   
> +static void print_pgm_info(struct stack_frame_int *stack)
> +
> +{
> +	printf("\n");
> +	printf("Unexpected program interrupt: %d on cpu %d at %#lx, ilen %d\n",
> +	       lc->pgm_int_code, stap(), lc->pgm_old_psw.addr,
> +	       lc->pgm_int_id);
> +	dump_stack();
> +	report_summary();
> +	abort();
> +}

I asssume this hunk should go into the next patch instead?
Or should the change to handle_pgm_int() from the next patch go into this 
patch here instead?
Otherwise you have an unused static function here and the compiler might 
complain about it (when bisecting later).

  Thomas

Re: [kvm-unit-tests PATCH v2 3/8] RFC: s390x: Define STACK_FRAME_INT_SIZE macro

[Janosch Frank]

On 2/17/21 4:38 PM, Thomas Huth wrote:
> On 17/02/2021 15.41, Janosch Frank wrote:
>> Using sizeof is safer than using magic constants. However, it doesn't
>> really fit into asm-offsets.h as it's not an offset so I'm happy to
>> receive suggestions on where to put it.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>>   lib/s390x/asm-offsets.c | 1 +
>>   s390x/macros.S          | 4 ++--
>>   2 files changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/lib/s390x/asm-offsets.c b/lib/s390x/asm-offsets.c
>> index 96cb21cf..2658b59a 100644
>> --- a/lib/s390x/asm-offsets.c
>> +++ b/lib/s390x/asm-offsets.c
>> @@ -86,6 +86,7 @@ int main(void)
>>   	OFFSET(STACK_FRAME_INT_CRS, stack_frame_int, crs);
>>   	OFFSET(STACK_FRAME_INT_GRS0, stack_frame_int, grs0);
>>   	OFFSET(STACK_FRAME_INT_GRS1, stack_frame_int, grs1);
>> +	DEFINE(STACK_FRAME_INT_SIZE, sizeof(struct stack_frame_int));
>>   
>>   	return 0;
>>   }
>> diff --git a/s390x/macros.S b/s390x/macros.S
>> index d7eeeb55..a7d62c6f 100644
>> --- a/s390x/macros.S
>> +++ b/s390x/macros.S
>> @@ -43,14 +43,14 @@
>>   /* Save registers on the stack (r15), so we can have stacked interrupts. */
>>   	.macro SAVE_REGS_STACK
>>   	/* Allocate a full stack frame */
>> -	slgfi   %r15, 32 * 8 + 4 * 8
>> +	slgfi   %r15, STACK_FRAME_INT_SIZE
>>   	/* Store registers r0 to r14 on the stack */
>>   	stmg    %r2, %r15, STACK_FRAME_INT_GRS0(%r15)
>>   	stg     %r0, STACK_FRAME_INT_GRS1(%r15)
>>   	stg     %r1, STACK_FRAME_INT_GRS1 + 8(%r15)
>>   	/* Store the gr15 value before we allocated the new stack */
>>   	lgr     %r0, %r15
>> -	algfi   %r0, 32 * 8 + 4 * 8
>> +	algfi   %r0, STACK_FRAME_INT_SIZE
> 
> Ah, well, that of course fixes the problem that I had with the previous 
> patch. I'd suggest to merge it into patch 2.

That was the plan anyway, I had so much pain until I yanked out the int
offsets in favor of the macros. :)

Did you have time to read the commit message?
I'm not completely convinced that asm-offset.c is the right place for
the DEFINE() so I kept this patch as a discussion starter.

> 
>   Thomas
> 

Re: [kvm-unit-tests PATCH v2 3/8] RFC: s390x: Define STACK_FRAME_INT_SIZE macro

[Thomas Huth]

On 17/02/2021 17.08, Janosch Frank wrote:
> On 2/17/21 4:38 PM, Thomas Huth wrote:
>> On 17/02/2021 15.41, Janosch Frank wrote:
>>> Using sizeof is safer than using magic constants. However, it doesn't
>>> really fit into asm-offsets.h as it's not an offset so I'm happy to
>>> receive suggestions on where to put it.
>>>
>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>>> ---
>>>    lib/s390x/asm-offsets.c | 1 +
>>>    s390x/macros.S          | 4 ++--
>>>    2 files changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/lib/s390x/asm-offsets.c b/lib/s390x/asm-offsets.c
>>> index 96cb21cf..2658b59a 100644
>>> --- a/lib/s390x/asm-offsets.c
>>> +++ b/lib/s390x/asm-offsets.c
>>> @@ -86,6 +86,7 @@ int main(void)
>>>    	OFFSET(STACK_FRAME_INT_CRS, stack_frame_int, crs);
>>>    	OFFSET(STACK_FRAME_INT_GRS0, stack_frame_int, grs0);
>>>    	OFFSET(STACK_FRAME_INT_GRS1, stack_frame_int, grs1);
>>> +	DEFINE(STACK_FRAME_INT_SIZE, sizeof(struct stack_frame_int));
>>>    
>>>    	return 0;
>>>    }
>>> diff --git a/s390x/macros.S b/s390x/macros.S
>>> index d7eeeb55..a7d62c6f 100644
>>> --- a/s390x/macros.S
>>> +++ b/s390x/macros.S
>>> @@ -43,14 +43,14 @@
>>>    /* Save registers on the stack (r15), so we can have stacked interrupts. */
>>>    	.macro SAVE_REGS_STACK
>>>    	/* Allocate a full stack frame */
>>> -	slgfi   %r15, 32 * 8 + 4 * 8
>>> +	slgfi   %r15, STACK_FRAME_INT_SIZE
>>>    	/* Store registers r0 to r14 on the stack */
>>>    	stmg    %r2, %r15, STACK_FRAME_INT_GRS0(%r15)
>>>    	stg     %r0, STACK_FRAME_INT_GRS1(%r15)
>>>    	stg     %r1, STACK_FRAME_INT_GRS1 + 8(%r15)
>>>    	/* Store the gr15 value before we allocated the new stack */
>>>    	lgr     %r0, %r15
>>> -	algfi   %r0, 32 * 8 + 4 * 8
>>> +	algfi   %r0, STACK_FRAME_INT_SIZE
>>
>> Ah, well, that of course fixes the problem that I had with the previous
>> patch. I'd suggest to merge it into patch 2.
> 
> That was the plan anyway, I had so much pain until I yanked out the int
> offsets in favor of the macros. :)
> 
> Did you have time to read the commit message?
> I'm not completely convinced that asm-offset.c is the right place for
> the DEFINE() so I kept this patch as a discussion starter.

Yes, I read the commit message, and I'm fine with the DEFINE in that file. 
But if you feel uncomfortable, maybe add a comment in front of that line?

  Thomas

Re: [kvm-unit-tests PATCH v2 5/8] s390x: Provide preliminary backtrace support

[Janosch Frank]

On 2/17/21 5:01 PM, Thomas Huth wrote:
> On 17/02/2021 15.41, Janosch Frank wrote:
>> After the stack changes we can finally use -mbackchain and have a
>> working backtrace.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>>   lib/s390x/interrupt.c | 12 ++++++++++++
>>   lib/s390x/stack.c     | 20 ++++++++++++++------
>>   s390x/Makefile        |  1 +
>>   3 files changed, 27 insertions(+), 6 deletions(-)
>>
>> diff --git a/lib/s390x/interrupt.c b/lib/s390x/interrupt.c
>> index a59df80e..23ad922c 100644
>> --- a/lib/s390x/interrupt.c
>> +++ b/lib/s390x/interrupt.c
>> @@ -115,6 +115,18 @@ static void fixup_pgm_int(struct stack_frame_int *stack)
>>   	/* suppressed/terminated/completed point already at the next address */
>>   }
>>   
>> +static void print_pgm_info(struct stack_frame_int *stack)
>> +
>> +{
>> +	printf("\n");
>> +	printf("Unexpected program interrupt: %d on cpu %d at %#lx, ilen %d\n",
>> +	       lc->pgm_int_code, stap(), lc->pgm_old_psw.addr,
>> +	       lc->pgm_int_id);
>> +	dump_stack();
>> +	report_summary();
>> +	abort();
>> +}
> 
> I asssume this hunk should go into the next patch instead?
> Or should the change to handle_pgm_int() from the next patch go into this 
> patch here instead?
> Otherwise you have an unused static function here and the compiler might 
> complain about it (when bisecting later).

I'll move it to the next patch

> 
>   Thomas
> 

Re: [kvm-unit-tests PATCH v2 8/8] s390x: Remove SAVE/RESTORE_stack

[Thomas Huth]

On 17/02/2021 15.41, Janosch Frank wrote:
> There are no more users.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> Reviewed-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>   s390x/macros.S | 29 -----------------------------
>   1 file changed, 29 deletions(-)
> 
> diff --git a/s390x/macros.S b/s390x/macros.S
> index 212a3823..399a87c6 100644
> --- a/s390x/macros.S
> +++ b/s390x/macros.S
> @@ -28,35 +28,6 @@
>   	lpswe	\old_psw
>   	.endm
>   
> -	.macro SAVE_REGS
> -	/* save grs 0-15 */
> -	stmg	%r0, %r15, GEN_LC_SW_INT_GRS
> -	/* save crs 0-15 */
> -	stctg	%c0, %c15, GEN_LC_SW_INT_CRS
> -	/* load a cr0 that has the AFP control bit which enables all FPRs */
> -	larl	%r1, initial_cr0
> -	lctlg	%c0, %c0, 0(%r1)
> -	/* save fprs 0-15 + fpc */
> -	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
> -	std	\i, \i * 8(%r1)
> -	.endr
> -	stfpc	GEN_LC_SW_INT_FPC
> -	.endm
> -
> -	.macro RESTORE_REGS
> -	/* restore fprs 0-15 + fpc */
> -	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

Could we now also remove the sw_int_fprs and sw_int_fpc from the lowcore?

  Thomas

Re: [kvm-unit-tests PATCH v2 4/8] s390x: Introduce and use CALL_INT_HANDLER macro

[Janosch Frank]

On 2/17/21 4:55 PM, Thomas Huth wrote:
> On 17/02/2021 15.41, Janosch Frank wrote:
>> The ELF ABI dictates that we need to allocate 160 bytes of stack space
>> for the C functions we're calling. Since we would need to do that for
>> every interruption handler which, combined with the new stack argument
>> being saved in GR2, makes cstart64.S look a bit messy.
>>
>> So let's introduce the CALL_INT_HANDLER macro that handles all of
>> that, calls the C interrupt handler and handles cleanup afterwards.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>>   s390x/cstart64.S | 28 +++++-----------------------
>>   s390x/macros.S   | 17 +++++++++++++++++
>>   2 files changed, 22 insertions(+), 23 deletions(-)
>>
>> diff --git a/s390x/cstart64.S b/s390x/cstart64.S
>> index 35d20293..666a9567 100644
>> --- a/s390x/cstart64.S
>> +++ b/s390x/cstart64.S
>> @@ -92,37 +92,19 @@ memsetxc:
>>   
>>   .section .text
>>   pgm_int:
>> -	SAVE_REGS_STACK
>> -	lgr     %r2, %r15
>> -	brasl	%r14, handle_pgm_int
>> -	RESTORE_REGS_STACK
>> -	lpswe	GEN_LC_PGM_OLD_PSW
>> +	CALL_INT_HANDLER handle_pgm_int, GEN_LC_PGM_OLD_PSW
>>   
>>   ext_int:
>> -	SAVE_REGS_STACK
>> -	lgr     %r2, %r15
>> -	brasl	%r14, handle_ext_int
>> -	RESTORE_REGS_STACK
>> -	lpswe	GEN_LC_EXT_OLD_PSW
>> +	CALL_INT_HANDLER handle_ext_int, GEN_LC_EXT_OLD_PSW
>>   
>>   mcck_int:
>> -	SAVE_REGS_STACK
>> -	brasl	%r14, handle_mcck_int
>> -	RESTORE_REGS_STACK
>> -	lpswe	GEN_LC_MCCK_OLD_PSW
>> +	CALL_INT_HANDLER handle_mcck_int, GEN_LC_MCCK_OLD_PSW
>>   
>>   io_int:
>> -	SAVE_REGS_STACK
>> -	lgr     %r2, %r15
>> -	brasl	%r14, handle_io_int
>> -	RESTORE_REGS_STACK
>> -	lpswe	GEN_LC_IO_OLD_PSW
>> +	CALL_INT_HANDLER handle_io_int, GEN_LC_IO_OLD_PSW
>>   
>>   svc_int:
>> -	SAVE_REGS_STACK
>> -	brasl	%r14, handle_svc_int
>> -	RESTORE_REGS_STACK
>> -	lpswe	GEN_LC_SVC_OLD_PSW
>> +	CALL_INT_HANDLER handle_svc_int, GEN_LC_SVC_OLD_PSW
>>   
>>   	.align	8
>>   initial_psw:
>> diff --git a/s390x/macros.S b/s390x/macros.S
>> index a7d62c6f..212a3823 100644
>> --- a/s390x/macros.S
>> +++ b/s390x/macros.S
>> @@ -11,6 +11,23 @@
>>    *  David Hildenbrand <david@redhat.com>
>>    */
>>   #include <asm/asm-offsets.h>
>> +/*
>> + * Exception handler macro that saves registers on the stack,
>> + * allocates stack space and calls the C handler function. Afterwards
>> + * we re-load the registers and load the old PSW.
>> + */
>> +	.macro CALL_INT_HANDLER c_func, old_psw
>> +	SAVE_REGS_STACK
>> +	/* Save the stack address in GR2 which is the first function argument */
>> +	lgr     %r2, %r15
>> +	/* Allocate stack pace for called C function, as specified in s390 ELF ABI */
>> +	slgfi   %r15, 160
> 
> By the way, don't you have to store a back chain pointer at the bottom of 
> that area, too, if you want to use -mbackchoin in the next patch?

Don't I already do that in #2?

+       /* Store the gr15 value before we allocated the new stack */



+       lgr     %r0, %r15



+       algfi   %r0, 32 * 8 + 4 * 8



+       stg     %r0, 13 * 8 + STACK_FRAME_INT_GRS0(%r15)



+       stg     %r0, STACK_FRAME_INT_BACKCHAIN(%r15)

I can vertainly move the hunk here and improve the comment.


> 
>   Thomas
> 
> 
>> +	brasl	%r14, \c_func
>> +	algfi   %r15, 160
>> +	RESTORE_REGS_STACK
>> +	lpswe	\old_psw
>> +	.endm
>> +
>>   	.macro SAVE_REGS
>>   	/* save grs 0-15 */
>>   	stmg	%r0, %r15, GEN_LC_SW_INT_GRS
>>
> 

Re: [kvm-unit-tests PATCH v2 8/8] s390x: Remove SAVE/RESTORE_stack

[Janosch Frank]

On 2/17/21 5:18 PM, Thomas Huth wrote:
> On 17/02/2021 15.41, Janosch Frank wrote:
>> There are no more users.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
>> Reviewed-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   s390x/macros.S | 29 -----------------------------
>>   1 file changed, 29 deletions(-)
>>
>> diff --git a/s390x/macros.S b/s390x/macros.S
>> index 212a3823..399a87c6 100644
>> --- a/s390x/macros.S
>> +++ b/s390x/macros.S
>> @@ -28,35 +28,6 @@
>>   	lpswe	\old_psw
>>   	.endm
>>   
>> -	.macro SAVE_REGS
>> -	/* save grs 0-15 */
>> -	stmg	%r0, %r15, GEN_LC_SW_INT_GRS
>> -	/* save crs 0-15 */
>> -	stctg	%c0, %c15, GEN_LC_SW_INT_CRS
>> -	/* load a cr0 that has the AFP control bit which enables all FPRs */
>> -	larl	%r1, initial_cr0
>> -	lctlg	%c0, %c0, 0(%r1)
>> -	/* save fprs 0-15 + fpc */
>> -	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
>> -	std	\i, \i * 8(%r1)
>> -	.endr
>> -	stfpc	GEN_LC_SW_INT_FPC
>> -	.endm
>> -
>> -	.macro RESTORE_REGS
>> -	/* restore fprs 0-15 + fpc */
>> -	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
> 
> Could we now also remove the sw_int_fprs and sw_int_fpc from the lowcore?
> 
>   Thomas
> 

git grep tells me that we can.
Do you want to have both the offset macro and the struct member removed
or only the macro?

We'll still need the grs and crs for the cpu setup in lib/s390x/smp.c

Re: [kvm-unit-tests PATCH v2 8/8] s390x: Remove SAVE/RESTORE_stack

[Thomas Huth]

On 17/02/2021 17.46, Janosch Frank wrote:
> On 2/17/21 5:18 PM, Thomas Huth wrote:
>> On 17/02/2021 15.41, Janosch Frank wrote:
>>> There are no more users.
>>>
>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>>> Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
>>> Reviewed-by: Pierre Morel <pmorel@linux.ibm.com>
>>> ---
>>>    s390x/macros.S | 29 -----------------------------
>>>    1 file changed, 29 deletions(-)
>>>
>>> diff --git a/s390x/macros.S b/s390x/macros.S
>>> index 212a3823..399a87c6 100644
>>> --- a/s390x/macros.S
>>> +++ b/s390x/macros.S
>>> @@ -28,35 +28,6 @@
>>>    	lpswe	\old_psw
>>>    	.endm
>>>    
>>> -	.macro SAVE_REGS
>>> -	/* save grs 0-15 */
>>> -	stmg	%r0, %r15, GEN_LC_SW_INT_GRS
>>> -	/* save crs 0-15 */
>>> -	stctg	%c0, %c15, GEN_LC_SW_INT_CRS
>>> -	/* load a cr0 that has the AFP control bit which enables all FPRs */
>>> -	larl	%r1, initial_cr0
>>> -	lctlg	%c0, %c0, 0(%r1)
>>> -	/* save fprs 0-15 + fpc */
>>> -	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
>>> -	std	\i, \i * 8(%r1)
>>> -	.endr
>>> -	stfpc	GEN_LC_SW_INT_FPC
>>> -	.endm
>>> -
>>> -	.macro RESTORE_REGS
>>> -	/* restore fprs 0-15 + fpc */
>>> -	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
>>
>> Could we now also remove the sw_int_fprs and sw_int_fpc from the lowcore?
>>
>>    Thomas
>>
> 
> git grep tells me that we can.
> Do you want to have both the offset macro and the struct member removed
> or only the macro?

I'd remove both.

  Thomas

Re: [kvm-unit-tests PATCH v2 2/8] s390x: Fully commit to stack save area for exceptions

[Janosch Frank]

On 2/17/21 4:35 PM, Thomas Huth wrote:
> On 17/02/2021 15.41, Janosch Frank wrote:
>> Having two sets of macros for saving registers on exceptions makes
>> maintaining harder. Also we have limited space in the lowcore to save
>> stuff and by using the stack as a save area, we can stack exceptions.
>>
>> So let's use the SAVE/RESTORE_REGS_STACK as the default. When we also
>> move the diag308 macro over we can remove the old SAVE/RESTORE_REGS
>> macros.
> [...]
>> diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
>> index 9c4e330a..31c2fc66 100644
>> --- a/lib/s390x/asm/arch_def.h
>> +++ b/lib/s390x/asm/arch_def.h
>> @@ -8,13 +8,30 @@
>>   #ifndef _ASM_S390X_ARCH_DEF_H_
>>   #define _ASM_S390X_ARCH_DEF_H_
>>   
>> -/*
>> - * We currently only specify the stack frame members needed for the
>> - * SIE library code.
>> - */
>>   struct stack_frame {
>> -	unsigned long back_chain;
>> -	unsigned long empty1[5];
>> +	struct stack_frame *back_chain;
>> +	u64 reserved;
>> +	/* GRs 2 - 5 */
>> +	unsigned long argument_area[4];
>> +	/* GRs 6 - 15 */
>> +	unsigned long grs[10];
>> +	/* FPRs 0, 2, 4, 6 */
>> +	s64  fprs[4];
>> +};
> 
> For consistency, could you please replace the "unsigned long" with u64, or 
> even switch to uint64_t completely?
> 
> Currently, we have:
> 
> $ grep -r u64 lib/s390x/ | wc -l
> 8
> $ grep -r uint64 lib/s390x/ | wc -l
> 94
> 
> ... so uint64_t seems to be the better choice.

Hmm, I like the short kernel types, but okay I'll bow to the majority. :)

> 
>> +struct stack_frame_int {
>> +	struct stack_frame *back_chain;
>> +	u64 reserved;
>> +	/*
>> +	 * The GRs are offset compatible with struct stack_frame so we
>> +	 * can easily fetch GR14 for backtraces.
>> +	 */
>> +	u64 grs0[14];
>> +	u64 grs1[2];
> 
> Which registers go into grs0 and which ones into grs1? And why is there a 
> split at all? A comment would be really helpful!

I've added two comments one for each struct member.

> 
>> +	u32 res;
> 
> res = reserved? Please add a comment.

Yes
It's now 'reserved1'

> 
>> +	u32 fpc;
>> +	u64 fprs[16];
>> +	u64 crs[16];
>>   };
> 
> Similar, switch to uint32_t and uint64_t ?

Will do

> 
>> diff --git a/s390x/macros.S b/s390x/macros.S
>> index e51a557a..d7eeeb55 100644
>> --- a/s390x/macros.S
>> +++ b/s390x/macros.S
>> @@ -3,9 +3,10 @@
>>    * s390x assembly macros
>>    *
>>    * Copyright (c) 2017 Red Hat Inc
>> - * Copyright (c) 2020 IBM Corp.
>> + * Copyright (c) 2020, 2021 IBM Corp.
>>    *
>>    * Authors:
>> + *  Janosch Frank <frankja@linux.ibm.com>
>>    *  Pierre Morel <pmorel@linux.ibm.com>
>>    *  David Hildenbrand <david@redhat.com>
>>    */
>> @@ -41,36 +42,45 @@
>>   
>>   /* Save registers on the stack (r15), so we can have stacked interrupts. */
>>   	.macro SAVE_REGS_STACK
>> -	/* Allocate a stack frame for 15 general registers */
>> -	slgfi   %r15, 15 * 8
>> +	/* Allocate a full stack frame */
>> +	slgfi   %r15, 32 * 8 + 4 * 8
> 
> How did you come up with that number? That does neither match stack 
> stack_frame nor stack_frame_int, if I got this right. Please add a comment 
> to the code to explain the numbers.
> 
>>   	/* Store registers r0 to r14 on the stack */
>> -	stmg    %r0, %r14, 0(%r15)
>> -	/* Allocate a stack frame for 16 floating point registers */
>> -	/* The size of a FP register is the size of an double word */
>> -	slgfi   %r15, 16 * 8
>> +	stmg    %r2, %r15, STACK_FRAME_INT_GRS0(%r15)
> 
> Storing up to r14 should be sufficent since you store r15 again below?

Yes, but it also doesn't hurt.

> 
>> +	stg     %r0, STACK_FRAME_INT_GRS1(%r15)
>> +	stg     %r1, STACK_FRAME_INT_GRS1 + 8(%r15)
>> +	/* Store the gr15 value before we allocated the new stack */
>> +	lgr     %r0, %r15
>> +	algfi   %r0, 32 * 8 + 4 * 8
>> +	stg     %r0, 13 * 8 + STACK_FRAME_INT_GRS0(%r15)
>> +	stg     %r0, STACK_FRAME_INT_BACKCHAIN(%r15)
>> +	/*
>> +	 * Store CR0 and load initial CR0 so AFP is active and we can
>> +	 * access all fprs to save them.
>> +	 */
>> +	stctg   %c0,%c15,STACK_FRAME_INT_CRS(%r15)
>> +	larl	%r1, initial_cr0
>> +	lctlg	%c0, %c0, 0(%r1)
>>   	/* Save fp register on stack: offset to SP is multiple of reg number */
>>   	.irp i, 0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15
>> -	std	\i, \i * 8(%r15)
>> +	std	\i, \i * 8 + STACK_FRAME_INT_FPRS(%r15)
>>   	.endr
> 
> So you saved 16 GRs, 16 CRs and 16 FPRs onto the stack, that's at least 16 * 
> 3 * 8 = 48 * 8 bytes ... but you only decreased the stack by 32 * 8 + 4 * 8 
> bytes initially ... is this a bug, or do I miss something?
> 
>   Thomas
> 

After I fixed the CR problem I didn't touch this anymore and the offset
macro overwrote it anyway and fixed the problem so it still worked on tests.


I've squashed the next patch into this one so we should be fine.

Re: [kvm-unit-tests PATCH v2 4/8] s390x: Introduce and use CALL_INT_HANDLER macro

[Thomas Huth]

On 17/02/2021 17.22, Janosch Frank wrote:
> On 2/17/21 4:55 PM, Thomas Huth wrote:
>> On 17/02/2021 15.41, Janosch Frank wrote:
>>> The ELF ABI dictates that we need to allocate 160 bytes of stack space
>>> for the C functions we're calling. Since we would need to do that for
>>> every interruption handler which, combined with the new stack argument
>>> being saved in GR2, makes cstart64.S look a bit messy.
>>>
>>> So let's introduce the CALL_INT_HANDLER macro that handles all of
>>> that, calls the C interrupt handler and handles cleanup afterwards.
>>>
>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>>> ---
>>>    s390x/cstart64.S | 28 +++++-----------------------
>>>    s390x/macros.S   | 17 +++++++++++++++++
>>>    2 files changed, 22 insertions(+), 23 deletions(-)
>>>
>>> diff --git a/s390x/cstart64.S b/s390x/cstart64.S
>>> index 35d20293..666a9567 100644
>>> --- a/s390x/cstart64.S
>>> +++ b/s390x/cstart64.S
>>> @@ -92,37 +92,19 @@ memsetxc:
>>>    
>>>    .section .text
>>>    pgm_int:
>>> -	SAVE_REGS_STACK
>>> -	lgr     %r2, %r15
>>> -	brasl	%r14, handle_pgm_int
>>> -	RESTORE_REGS_STACK
>>> -	lpswe	GEN_LC_PGM_OLD_PSW
>>> +	CALL_INT_HANDLER handle_pgm_int, GEN_LC_PGM_OLD_PSW
>>>    
>>>    ext_int:
>>> -	SAVE_REGS_STACK
>>> -	lgr     %r2, %r15
>>> -	brasl	%r14, handle_ext_int
>>> -	RESTORE_REGS_STACK
>>> -	lpswe	GEN_LC_EXT_OLD_PSW
>>> +	CALL_INT_HANDLER handle_ext_int, GEN_LC_EXT_OLD_PSW
>>>    
>>>    mcck_int:
>>> -	SAVE_REGS_STACK
>>> -	brasl	%r14, handle_mcck_int
>>> -	RESTORE_REGS_STACK
>>> -	lpswe	GEN_LC_MCCK_OLD_PSW
>>> +	CALL_INT_HANDLER handle_mcck_int, GEN_LC_MCCK_OLD_PSW
>>>    
>>>    io_int:
>>> -	SAVE_REGS_STACK
>>> -	lgr     %r2, %r15
>>> -	brasl	%r14, handle_io_int
>>> -	RESTORE_REGS_STACK
>>> -	lpswe	GEN_LC_IO_OLD_PSW
>>> +	CALL_INT_HANDLER handle_io_int, GEN_LC_IO_OLD_PSW
>>>    
>>>    svc_int:
>>> -	SAVE_REGS_STACK
>>> -	brasl	%r14, handle_svc_int
>>> -	RESTORE_REGS_STACK
>>> -	lpswe	GEN_LC_SVC_OLD_PSW
>>> +	CALL_INT_HANDLER handle_svc_int, GEN_LC_SVC_OLD_PSW
>>>    
>>>    	.align	8
>>>    initial_psw:
>>> diff --git a/s390x/macros.S b/s390x/macros.S
>>> index a7d62c6f..212a3823 100644
>>> --- a/s390x/macros.S
>>> +++ b/s390x/macros.S
>>> @@ -11,6 +11,23 @@
>>>     *  David Hildenbrand <david@redhat.com>
>>>     */
>>>    #include <asm/asm-offsets.h>
>>> +/*
>>> + * Exception handler macro that saves registers on the stack,
>>> + * allocates stack space and calls the C handler function. Afterwards
>>> + * we re-load the registers and load the old PSW.
>>> + */
>>> +	.macro CALL_INT_HANDLER c_func, old_psw
>>> +	SAVE_REGS_STACK
>>> +	/* Save the stack address in GR2 which is the first function argument */
>>> +	lgr     %r2, %r15
>>> +	/* Allocate stack pace for called C function, as specified in s390 ELF ABI */
>>> +	slgfi   %r15, 160
>>
>> By the way, don't you have to store a back chain pointer at the bottom of
>> that area, too, if you want to use -mbackchoin in the next patch?
> 
> Don't I already do that in #2?

You do it in the SAVE_REGS_STACK patch, yes. But not on the bottom of the 
new 160 bytes stack frame that you've added here. But I guess it doesn't 
really matter for your back traces, since you load %r2 with %r15 before 
decrementing the stack by 160, so this new stack frame simply gets ignored 
anyway.

  Thomas

Re: [kvm-unit-tests PATCH v2 4/8] s390x: Introduce and use CALL_INT_HANDLER macro

[Janosch Frank]

On 2/17/21 6:03 PM, Thomas Huth wrote:
> On 17/02/2021 17.22, Janosch Frank wrote:
>> On 2/17/21 4:55 PM, Thomas Huth wrote:
>>> On 17/02/2021 15.41, Janosch Frank wrote:
>>>> The ELF ABI dictates that we need to allocate 160 bytes of stack space
>>>> for the C functions we're calling. Since we would need to do that for
>>>> every interruption handler which, combined with the new stack argument
>>>> being saved in GR2, makes cstart64.S look a bit messy.
>>>>
>>>> So let's introduce the CALL_INT_HANDLER macro that handles all of
>>>> that, calls the C interrupt handler and handles cleanup afterwards.
>>>>
>>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>>>> ---
>>>>    s390x/cstart64.S | 28 +++++-----------------------
>>>>    s390x/macros.S   | 17 +++++++++++++++++
>>>>    2 files changed, 22 insertions(+), 23 deletions(-)
>>>>
>>>> diff --git a/s390x/cstart64.S b/s390x/cstart64.S
>>>> index 35d20293..666a9567 100644
>>>> --- a/s390x/cstart64.S
>>>> +++ b/s390x/cstart64.S
>>>> @@ -92,37 +92,19 @@ memsetxc:
>>>>    
>>>>    .section .text
>>>>    pgm_int:
>>>> -	SAVE_REGS_STACK
>>>> -	lgr     %r2, %r15
>>>> -	brasl	%r14, handle_pgm_int
>>>> -	RESTORE_REGS_STACK
>>>> -	lpswe	GEN_LC_PGM_OLD_PSW
>>>> +	CALL_INT_HANDLER handle_pgm_int, GEN_LC_PGM_OLD_PSW
>>>>    
>>>>    ext_int:
>>>> -	SAVE_REGS_STACK
>>>> -	lgr     %r2, %r15
>>>> -	brasl	%r14, handle_ext_int
>>>> -	RESTORE_REGS_STACK
>>>> -	lpswe	GEN_LC_EXT_OLD_PSW
>>>> +	CALL_INT_HANDLER handle_ext_int, GEN_LC_EXT_OLD_PSW
>>>>    
>>>>    mcck_int:
>>>> -	SAVE_REGS_STACK
>>>> -	brasl	%r14, handle_mcck_int
>>>> -	RESTORE_REGS_STACK
>>>> -	lpswe	GEN_LC_MCCK_OLD_PSW
>>>> +	CALL_INT_HANDLER handle_mcck_int, GEN_LC_MCCK_OLD_PSW
>>>>    
>>>>    io_int:
>>>> -	SAVE_REGS_STACK
>>>> -	lgr     %r2, %r15
>>>> -	brasl	%r14, handle_io_int
>>>> -	RESTORE_REGS_STACK
>>>> -	lpswe	GEN_LC_IO_OLD_PSW
>>>> +	CALL_INT_HANDLER handle_io_int, GEN_LC_IO_OLD_PSW
>>>>    
>>>>    svc_int:
>>>> -	SAVE_REGS_STACK
>>>> -	brasl	%r14, handle_svc_int
>>>> -	RESTORE_REGS_STACK
>>>> -	lpswe	GEN_LC_SVC_OLD_PSW
>>>> +	CALL_INT_HANDLER handle_svc_int, GEN_LC_SVC_OLD_PSW
>>>>    
>>>>    	.align	8
>>>>    initial_psw:
>>>> diff --git a/s390x/macros.S b/s390x/macros.S
>>>> index a7d62c6f..212a3823 100644
>>>> --- a/s390x/macros.S
>>>> +++ b/s390x/macros.S
>>>> @@ -11,6 +11,23 @@
>>>>     *  David Hildenbrand <david@redhat.com>
>>>>     */
>>>>    #include <asm/asm-offsets.h>
>>>> +/*
>>>> + * Exception handler macro that saves registers on the stack,
>>>> + * allocates stack space and calls the C handler function. Afterwards
>>>> + * we re-load the registers and load the old PSW.
>>>> + */
>>>> +	.macro CALL_INT_HANDLER c_func, old_psw
>>>> +	SAVE_REGS_STACK
>>>> +	/* Save the stack address in GR2 which is the first function argument */
>>>> +	lgr     %r2, %r15
>>>> +	/* Allocate stack pace for called C function, as specified in s390 ELF ABI */
>>>> +	slgfi   %r15, 160
>>>
>>> By the way, don't you have to store a back chain pointer at the bottom of
>>> that area, too, if you want to use -mbackchoin in the next patch?
>>
>> Don't I already do that in #2?
> 
> You do it in the SAVE_REGS_STACK patch, yes. But not on the bottom of the 
> new 160 bytes stack frame that you've added here. But I guess it doesn't 
> really matter for your back traces, since you load %r2 with %r15 before 
> decrementing the stack by 160, so this new stack frame simply gets ignored 
> anyway.
> 
>   Thomas
> 

Right, I'm currently fixing that up for the next version.

diff --git i/s390x/macros.S w/s390x/macros.S
index b4275c77..13cff299 100644
--- i/s390x/macros.S
+++ w/s390x/macros.S
@@ -22,6 +22,11 @@
        lgr     %r2, %r15
        /* Allocate stack space for called C function, as specified in
s390 ELF ABI */
        slgfi   %r15, 160
+       /*
+        * Save the address of the interrupt stack into the back chain
+        * of the called function.
+        */
+       stg     %r2, STACK_FRAME_INT_BACKCHAIN(%r15)
        brasl   %r14, \c_func
        algfi   %r15, 160
        RESTORE_REGS_STACK