linux-kernel - Re: [PATCH v5 19/22] powerpc/syscall: Optimise checks in beginning of system_call

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <1612836170.502t0sssvi.astroid@bobo.none>
Date:   Tue, 09 Feb 2021 12:06:05 +1000
From:   Nicholas Piggin <npiggin@...il.com>
To:     Benjamin Herrenschmidt <benh@...nel.crashing.org>,
        Christophe Leroy <christophe.leroy@...roup.eu>,
        Michael Ellerman <mpe@...erman.id.au>, msuchanek@...e.de,
        Paul Mackerras <paulus@...ba.org>
Cc:     linux-kernel@...r.kernel.org, linuxppc-dev@...ts.ozlabs.org
Subject: Re: [PATCH v5 19/22] powerpc/syscall: Optimise checks in beginning of
 system_call_exception()

Excerpts from Christophe Leroy's message of February 9, 2021 1:10 am:
> Combine all tests of regs->msr into a single logical one.

Okay by me unless we choose to do the config option and put these all 
under it. I think I would prefer that because sometimes the registers
are in a state you can't easily see what the values in the expression
were. In this case it doesn't matter so much because they should be in
regs in the interrupt frame.

Thanks,
Nick

> 
> Before the patch:
> 
>    0:	81 6a 00 84 	lwz     r11,132(r10)
>    4:	90 6a 00 88 	stw     r3,136(r10)
>    8:	69 60 00 02 	xori    r0,r11,2
>    c:	54 00 ff fe 	rlwinm  r0,r0,31,31,31
>   10:	0f 00 00 00 	twnei   r0,0
>   14:	69 63 40 00 	xori    r3,r11,16384
>   18:	54 63 97 fe 	rlwinm  r3,r3,18,31,31
>   1c:	0f 03 00 00 	twnei   r3,0
>   20:	69 6b 80 00 	xori    r11,r11,32768
>   24:	55 6b 8f fe 	rlwinm  r11,r11,17,31,31
>   28:	0f 0b 00 00 	twnei   r11,0
> 
> After the patch:
> 
>    0:	81 6a 00 84 	lwz     r11,132(r10)
>    4:	90 6a 00 88 	stw     r3,136(r10)
>    8:	7d 6b 58 f8 	not     r11,r11
>    c:	71 6b c0 02 	andi.   r11,r11,49154
>   10:	0f 0b 00 00 	twnei   r11,0
> 
> 6 cycles less on powerpc 8xx (328 => 322 cycles).
> 
> Signed-off-by: Christophe Leroy <christophe.leroy@...roup.eu>
> ---
>  arch/powerpc/kernel/interrupt.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/interrupt.c b/arch/powerpc/kernel/interrupt.c
> index 55e1aa18cdb9..8c38e8c95be2 100644
> --- a/arch/powerpc/kernel/interrupt.c
> +++ b/arch/powerpc/kernel/interrupt.c
> @@ -28,6 +28,7 @@ notrace long system_call_exception(long r3, long r4, long r5,
>  				   unsigned long r0, struct pt_regs *regs)
>  {
>  	syscall_fn f;
> +	unsigned long expected_msr;
>  
>  	regs->orig_gpr3 = r3;
>  
> @@ -39,10 +40,13 @@ notrace long system_call_exception(long r3, long r4, long r5,
>  
>  	trace_hardirqs_off(); /* finish reconciling */
>  
> +	expected_msr = MSR_PR;
>  	if (!IS_ENABLED(CONFIG_BOOKE) && !IS_ENABLED(CONFIG_40x))
> -		BUG_ON(!(regs->msr & MSR_RI));
> -	BUG_ON(!(regs->msr & MSR_PR));
> -	BUG_ON(arch_irq_disabled_regs(regs));
> +		expected_msr |= MSR_RI;
> +	if (IS_ENABLED(CONFIG_PPC32))
> +		expected_msr |= MSR_EE;
> +	BUG_ON((regs->msr & expected_msr) ^ expected_msr);
> +	BUG_ON(IS_ENABLED(CONFIG_PPC64) && arch_irq_disabled_regs(regs));
>  
>  #ifdef CONFIG_PPC_PKEY
>  	if (mmu_has_feature(MMU_FTR_PKEY)) {
> -- 
> 2.25.0
> 
>