Re: [PATCH v4 15/20] powerpc/64s: Use {ZEROIZE,SAVE,REST}_GPRS macros in sc, scv 0 handlers

From: "Nicholas Piggin" <npiggin@gmail.com>
To: "Rohan McLure" <rmclure@linux.ibm.com>, <linuxppc-dev@lists.ozlabs.org>
Subject: Re: [PATCH v4 15/20] powerpc/64s: Use {ZEROIZE,SAVE,REST}_GPRS macros in sc, scv 0 handlers
Date: Mon, 12 Sep 2022 21:49:18 +1000	[thread overview]
Message-ID: <CMUEEG8WEDIM.1GHQCY11V524N@bobo> (raw)
In-Reply-To: <20220824020548.62625-16-rmclure@linux.ibm.com>

On Wed Aug 24, 2022 at 12:05 PM AEST, Rohan McLure wrote:
> Use the convenience macros for saving/clearing/restoring gprs in keeping
> with syscall calling conventions. The plural variants of these macros
> can store a range of registers for concision.
>
> This works well when the user gpr value we are hoping to save is still
> live. In the syscall interrupt handlers, user register state is
> sometimes juggled between registers. Hold-off from issuing the SAVE_GPR
> macro for applicable neighbouring lines to highlight the delicate
> register save logic.
>

Reviewed-by: Nicholas Piggin <npiggin@gmail.com>

> Signed-off-by: Rohan McLure <rmclure@linux.ibm.com>
> ---
> V1 -> V2: Update summary
> V2 -> V3: Update summary regarding exclusions for the SAVE_GPR marco.
> Acknowledge new name for ZEROIZE_GPR{,S} macros.
> ---
>  arch/powerpc/kernel/interrupt_64.S | 43 ++++++----------------------
>  1 file changed, 9 insertions(+), 34 deletions(-)
>
> diff --git a/arch/powerpc/kernel/interrupt_64.S b/arch/powerpc/kernel/interrupt_64.S
> index a8065209dd8c..ad302ad93433 100644
> --- a/arch/powerpc/kernel/interrupt_64.S
> +++ b/arch/powerpc/kernel/interrupt_64.S
> @@ -71,12 +71,7 @@ _ASM_NOKPROBE_SYMBOL(system_call_vectored_\name)
>  	mfcr	r12
>  	li	r11,0
>  	/* Save syscall parameters in r3-r8 */
> -	std	r3,GPR3(r1)
> -	std	r4,GPR4(r1)
> -	std	r5,GPR5(r1)
> -	std	r6,GPR6(r1)
> -	std	r7,GPR7(r1)
> -	std	r8,GPR8(r1)
> +	SAVE_GPRS(3, 8, r1)
>  	/* Zero r9-r12, this should only be required when restoring all GPRs */
>  	std	r11,GPR9(r1)
>  	std	r11,GPR10(r1)
> @@ -157,17 +152,10 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
>  	/* Could zero these as per ABI, but we may consider a stricter ABI
>  	 * which preserves these if libc implementations can benefit, so
>  	 * restore them for now until further measurement is done. */
> -	ld	r0,GPR0(r1)
> -	ld	r4,GPR4(r1)
> -	ld	r5,GPR5(r1)
> -	ld	r6,GPR6(r1)
> -	ld	r7,GPR7(r1)
> -	ld	r8,GPR8(r1)
> +	REST_GPR(0, r1)
> +	REST_GPRS(4, 8, r1)
>  	/* Zero volatile regs that may contain sensitive kernel data */
> -	li	r9,0
> -	li	r10,0
> -	li	r11,0
> -	li	r12,0
> +	ZEROIZE_GPRS(9, 12)
>  	mtspr	SPRN_XER,r0
>  
>  	/*
> @@ -189,7 +177,7 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
>  	ld	r4,_LINK(r1)
>  	ld	r5,_XER(r1)
>  
> -	ld	r0,GPR0(r1)
> +	REST_GPR(0, r1)
>  	mtcr	r2
>  	mtctr	r3
>  	mtlr	r4
> @@ -257,12 +245,7 @@ END_BTB_FLUSH_SECTION
>  	mfcr	r12
>  	li	r11,0
>  	/* Save syscall parameters in r3-r8 */
> -	std	r3,GPR3(r1)
> -	std	r4,GPR4(r1)
> -	std	r5,GPR5(r1)
> -	std	r6,GPR6(r1)
> -	std	r7,GPR7(r1)
> -	std	r8,GPR8(r1)
> +	SAVE_GPRS(3, 8, r1)
>  	/* Zero r9-r12, this should only be required when restoring all GPRs */
>  	std	r11,GPR9(r1)
>  	std	r11,GPR10(r1)
> @@ -360,16 +343,8 @@ END_FTR_SECTION_IFCLR(CPU_FTR_STCX_CHECKS_ADDRESS)
>  	cmpdi	r3,0
>  	bne	.Lsyscall_restore_regs
>  	/* Zero volatile regs that may contain sensitive kernel data */
> -	li	r0,0
> -	li	r4,0
> -	li	r5,0
> -	li	r6,0
> -	li	r7,0
> -	li	r8,0
> -	li	r9,0
> -	li	r10,0
> -	li	r11,0
> -	li	r12,0
> +	ZEROIZE_GPR(0)
> +	ZEROIZE_GPRS(4, 12)
>  	mtctr	r0
>  	mtspr	SPRN_XER,r0
>  .Lsyscall_restore_regs_cont:
> @@ -394,7 +369,7 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
>  	ld	r4,_XER(r1)
>  	mtctr	r3
>  	mtspr	SPRN_XER,r4
> -	ld	r0,GPR0(r1)
> +	REST_GPR(0, r1)
>  	REST_GPRS(4, 12, r1)
>  	b	.Lsyscall_restore_regs_cont
>  .Lsyscall_rst_end:
> -- 
> 2.34.1