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

[Thomas Huth <thuth@redhat.com>]

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>