linux-kernel - Re: [PATCH] LoongArch: Provide __lshrti3, __ashrti3, and _

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: <56414029e179d219442bde9b8eae81fa3e3ceec4.camel@xry111.site>
Date: Sat, 27 Apr 2024 12:13:25 +0800
From: Xi Ruoyao <xry111@...111.site>
To: Huacai Chen <chenhuacai@...nel.org>
Cc: WANG Xuerui <kernel@...0n.name>, loongarch@...ts.linux.dev, 
	linux-kernel@...r.kernel.org, Tiezhu Yang <yangtiezhu@...ngson.cn>
Subject: Re: [PATCH] LoongArch: Provide __lshrti3, __ashrti3, and __ashrti3

On Sat, 2024-04-27 at 12:00 +0800, Xi Ruoyao wrote:
> On Sat, 2024-04-27 at 10:50 +0800, Huacai Chen wrote:
> > Hi, Ruoyao,
> > 
> > I don't think #ifdef CONFIG_ARCH_SUPPORTS_INT128 is needed here.
> > S390/ARM64/RISCV all built it unconditionally.
> 
> The problem here is RISCV and ARM64 are using an incorrect prototype for
> these functions in asm-prototypes.h:
> 
> long long __lshrti3(long long a, int b); 
> long long __ashrti3(long long a, int b); 
> long long __ashlti3(long long a, int b); 
> 
> where "long long" is not 128-bit.  Despite this seems working for RISC-V
> and ARM64 I really dislike it.
> 
> S390 seems assuming CONFIG_ARCH_SUPPORTS_INT128 is always true, but I
> don't think we can assume it too (at least it'll likely to be false for
> LA32, so doing so will cause trouble when we add LA32 support).
> 
> So if we don't want to check CONFIG_ARCH_SUPPORTS_INT128 and still use a
> correct prototype, we'll do:
> 
> diff --git a/arch/loongarch/include/asm/asm-prototypes.h b/arch/loongarch/include/asm/asm-prototypes.h
> index 51f224bcfc65..0a57db01116d 100644
> --- a/arch/loongarch/include/asm/asm-prototypes.h
> +++ b/arch/loongarch/include/asm/asm-prototypes.h
> @@ -7,8 +7,6 @@
>  #include <asm/ftrace.h>
>  #include <asm-generic/asm-prototypes.h>
>  
> -#ifdef CONFIG_ARCH_SUPPORTS_INT128
> -__int128_t __ashlti3(__int128_t a, int b);
> -__int128_t __ashrti3(__int128_t a, int b);
> -__int128_t __lshrti3(__int128_t a, int b);
> -#endif
> +struct { u64 lo, hi; } __ashlti3(u64 lo, u64 hi, int b);
> +struct { u64 lo, hi; } __ashrti3(u64 lo, u64 hi, int b);
> +struct { u64 lo, hi; } __lshrti3(u64 lo, u64 hi, int b);

Whoops.  This is still incorrect for LA32.  On LA32 an "int128" (if it
ever exists) should be passed as a pointer, but this is passing it in 4
GPRs.  So if we want to keep the prototype correct we need to either use
"struct { u64 lo, hi; }" in the parameter list too, or guard it with
#ifdef CONFIG_64BIT.

So to me checking CONFIG_ARCH_SUPPORTS_INT128 is just easier.

If you insists on not checking CONFIG_ARCH_SUPPORTS_INT128 I'll just use
an incorrect prototype like RISC-V but put a comment here, like:

/* The prototypes are incorrect but this file is only used by
   modpost which does not care.  */
long long __ashlti3(long long a, int b);
long long __ashrti3(long long a, int b);
long long __lshrti3(long long a, int b);

How do you think?

-- 
Xi Ruoyao <xry111@...111.site>
School of Aerospace Science and Technology, Xidian University