Re: [patch V2 16/17] lib/vdso: Allow architectures to override the ns shift operation

From: Vincenzo Frascino <vincenzo.frascino@arm.com>
To: Thomas Gleixner <tglx@linutronix.de>,
	LKML <linux-kernel@vger.kernel.org>
Cc: x86@kernel.org, John Stultz <john.stultz@linaro.org>,
	Andy Lutomirski <luto@kernel.org>,
	Christophe Leroy <christophe.leroy@c-s.fr>,
	Paolo Bonzini <pbonzini@redhat.com>,
	Juergen Gross <jgross@suse.com>,
	Michael Kelley <mikelley@microsoft.com>,
	Sasha Levin <sashal@kernel.org>,
	Ralf Baechle <ralf@linux-mips.org>,
	Paul Burton <paulburton@kernel.org>,
	James Hogan <jhogan@kernel.org>,
	Russell King <linux@armlinux.org.uk>,
	Catalin Marinas <catalin.marinas@arm.com>,
	Will Deacon <will@kernel.org>,
	Mark Rutland <mark.rutland@arm.com>,
	Marc Zyngier <maz@kernel.org>, Andrei Vagin <avagin@gmail.com>
Subject: Re: [patch V2 16/17] lib/vdso: Allow architectures to override the ns shift operation
Date: Mon, 17 Feb 2020 11:15:45 +0000	[thread overview]
Message-ID: <b599d1ad-d278-7c5b-fc1d-f5a6b080c16e@arm.com> (raw)
In-Reply-To: <20200207124403.857649978@linutronix.de>

[-- Attachment #1: Type: text/plain, Size: 1727 bytes --]

On 07/02/2020 12:39, Thomas Gleixner wrote:
> From: Christophe Leroy <christophe.leroy@c-s.fr>
> 
> On powerpc/32, GCC (8.1) generates pretty bad code for the ns >>= vd->shift
> operation taking into account that the shift is always <= 32 and the upper
> part of the result is likely to be zero. GCC makes reversed assumptions
> considering the shift to be likely >= 32 and the upper part to be like not
> zero.
> 
> unsigned long long shift(unsigned long long x, unsigned char s)
> {
> 	return x >> s;
> }
> 
> results in:
> 
> 00000018 <shift>:
>   18:	35 25 ff e0 	addic.  r9,r5,-32
>   1c:	41 80 00 10 	blt     2c <shift+0x14>
>   20:	7c 64 4c 30 	srw     r4,r3,r9
>   24:	38 60 00 00 	li      r3,0
>   28:	4e 80 00 20 	blr
>   2c:	54 69 08 3c 	rlwinm  r9,r3,1,0,30
>   30:	21 45 00 1f 	subfic  r10,r5,31
>   34:	7c 84 2c 30 	srw     r4,r4,r5
>   38:	7d 29 50 30 	slw     r9,r9,r10
>   3c:	7c 63 2c 30 	srw     r3,r3,r5
>   40:	7d 24 23 78 	or      r4,r9,r4
>   44:	4e 80 00 20 	blr
> 
> Even when forcing the shift to be smaller than 32 with an &= 31, it still
> considers the shift as likely >= 32.
> 
> Move the default shift implementation into an inline which can be redefined
> in architecture code via a macro.
> 
> [ tglx: Made the shift argument u32 and removed the __arch prefix ]
> 
> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Link: https://lore.kernel.org/r/b3d449de856982ed060a71e6ace8eeca4654e685.1580399657.git.christophe.leroy@c-s.fr
> 


Reviewed-by: Vincenzo Frascino <vincenzo.frascino@arm.com>
Tested-by: Vincenzo Frascino <vincenzo.frascino@arm.com>

[...]
-- 
Regards,
Vincenzo

[-- Attachment #2: pEpkey.asc --]
[-- Type: application/pgp-keys, Size: 14291 bytes --]