Re: [PATCH v1 1/4] s390x: saving regs for interrupts

[Janosch Frank <frankja@linux.ibm.com>]

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

On 11/13/19 1:23 PM, Pierre Morel wrote:
> If we use multiple source of interrupts, for exemple, using SCLP console
> to print information while using I/O interrupts or during exceptions, we
> need to have a re-entrant register saving interruption handling.
> 
> Instead of saving at a static place, let's save the base registers on
> the stack.
> 
> Note that we keep the static register saving that we need for the RESET
> tests.
> 
> We also care to give the handlers a pointer to the save registers in
> case the handler needs it (fixup_pgm_int needs the old psw address).

So you're still ignoring the FPRs...
I disassembled a test and looked at all stds and it looks like printf
and related functions use them. Wouldn't we overwrite test FPRs if
printing in a handler?

> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  lib/s390x/asm/interrupt.h | 15 ++++++++++-----
>  lib/s390x/interrupt.c     | 16 ++++++++--------
>  s390x/cstart64.S          | 17 ++++++++++++++---
>  3 files changed, 32 insertions(+), 16 deletions(-)
> 
> diff --git a/lib/s390x/asm/interrupt.h b/lib/s390x/asm/interrupt.h
> index 4cfade9..a39a3a3 100644
> --- a/lib/s390x/asm/interrupt.h
> +++ b/lib/s390x/asm/interrupt.h
> @@ -15,11 +15,16 @@
>  #define EXT_IRQ_EXTERNAL_CALL	0x1202
>  #define EXT_IRQ_SERVICE_SIG	0x2401
>  
> -void handle_pgm_int(void);
> -void handle_ext_int(void);
> -void handle_mcck_int(void);
> -void handle_io_int(void);
> -void handle_svc_int(void);
> +typedef struct saved_registers {
> +        unsigned long regs[15];
> +} sregs_t;
> +
> +void handle_pgm_int(sregs_t *regs);
> +void handle_ext_int(sregs_t *regs);
> +void handle_mcck_int(sregs_t *regs);
> +void handle_io_int(sregs_t *regs);
> +void handle_svc_int(sregs_t *regs);
> +
>  void expect_pgm_int(void);
>  void expect_ext_int(void);
>  uint16_t clear_pgm_int(void);
> diff --git a/lib/s390x/interrupt.c b/lib/s390x/interrupt.c
> index 5cade23..7aecfc5 100644
> --- a/lib/s390x/interrupt.c
> +++ b/lib/s390x/interrupt.c
> @@ -50,7 +50,7 @@ void check_pgm_int_code(uint16_t code)
>  	       code == lc->pgm_int_code, code, lc->pgm_int_code);
>  }
>  
> -static void fixup_pgm_int(void)
> +static void fixup_pgm_int(sregs_t *regs)
>  {
>  	switch (lc->pgm_int_code) {
>  	case PGM_INT_CODE_PRIVILEGED_OPERATION:
> @@ -64,7 +64,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 = regs->regs[14];
>  		break;
>  	case PGM_INT_CODE_SEGMENT_TRANSLATION:
>  	case PGM_INT_CODE_PAGE_TRANSLATION:
> @@ -103,7 +103,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(sregs_t *regs)
>  {
>  	if (!pgm_int_expected)
>  		report_abort("Unexpected program interrupt: %d at %#lx, ilen %d\n",
> @@ -111,10 +111,10 @@ void handle_pgm_int(void)
>  			     lc->pgm_int_id);
>  
>  	pgm_int_expected = false;
> -	fixup_pgm_int();
> +	fixup_pgm_int(regs);
>  }
>  
> -void handle_ext_int(void)
> +void handle_ext_int(sregs_t *regs)
>  {
>  	if (!ext_int_expected &&
>  	    lc->ext_int_code != EXT_IRQ_SERVICE_SIG) {
> @@ -134,19 +134,19 @@ void handle_ext_int(void)
>  		lc->ext_old_psw.mask &= ~PSW_MASK_EXT;
>  }
>  
> -void handle_mcck_int(void)
> +void handle_mcck_int(sregs_t *regs)
>  {
>  	report_abort("Unexpected machine check interrupt: at %#lx",
>  		     lc->mcck_old_psw.addr);
>  }
>  
> -void handle_io_int(void)
> +void handle_io_int(sregs_t *regs)
>  {
>  	report_abort("Unexpected io interrupt: at %#lx",
>  		     lc->io_old_psw.addr);
>  }
>  
> -void handle_svc_int(void)
> +void handle_svc_int(sregs_t *regs)
>  {
>  	report_abort("Unexpected supervisor call interrupt: at %#lx",
>  		     lc->svc_old_psw.addr);
> diff --git a/s390x/cstart64.S b/s390x/cstart64.S
> index 8e2b21e..eaff481 100644
> --- a/s390x/cstart64.S
> +++ b/s390x/cstart64.S
> @@ -90,6 +90,17 @@ memsetxc:
>  	xc 0(1,%r1),0(%r1)
>  
>  	.macro SAVE_REGS
> +	slgfi	%r15, 15 * 8
> +	stmg	%r0, %r14, 0(%r15)
> +	lgr	%r2, %r15
> +	.endm
> +
> +	.macro RESTORE_REGS
> +	lmg     %r0, %r14, 0(%r15)
> +	algfi   %r15, 15 * 8
> +	.endm
> +
> +	.macro SAVE_REGS_RESET
>  	/* save grs 0-15 */
>  	stmg	%r0, %r15, GEN_LC_SW_INT_GRS
>  	/* save cr0 */
> @@ -105,7 +116,7 @@ memsetxc:
>  	stfpc	GEN_LC_SW_INT_FPC
>  	.endm
>  
> -	.macro RESTORE_REGS
> +	.macro RESTORE_REGS_RESET
>  	/* 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
> @@ -125,7 +136,7 @@ memsetxc:
>   */
>  .globl diag308_load_reset
>  diag308_load_reset:
> -	SAVE_REGS
> +	SAVE_REGS_RESET
>  	/* Save the first PSW word to the IPL PSW */
>  	epsw	%r0, %r1
>  	st	%r0, 0
> @@ -142,7 +153,7 @@ diag308_load_reset:
>  	/* We lost cr0 due to the reset */
>  0:	larl	%r1, initial_cr0
>  	lctlg	%c0, %c0, 0(%r1)
> -	RESTORE_REGS
> +	RESTORE_REGS_RESET
>  	lhi	%r2, 1
>  	br	%r14
>  
> 



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