linux-kernel - Re: [PATCH V2 1/2] uapi: Define GENMASK

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: <b1dd907d-d45b-4602-964e-70654094a315@arm.com>
Date: Wed, 31 Jul 2024 09:14:54 +0530
From: Anshuman Khandual <anshuman.khandual@....com>
To: Yury Norov <yury.norov@...il.com>
Cc: linux-kernel@...r.kernel.org, Andrew Morton <akpm@...ux-foundation.org>,
 Rasmus Villemoes <linux@...musvillemoes.dk>, Arnd Bergmann <arnd@...db.de>,
 linux-arch@...r.kernel.org
Subject: Re: [PATCH V2 1/2] uapi: Define GENMASK_U128



On 7/30/24 23:45, Yury Norov wrote:
> On Thu, Jul 25, 2024 at 11:18:07AM +0530, Anshuman Khandual wrote:
>> This adds GENMASK_U128() and __GENMASK_U128() macros using __BITS_PER_U128
>> and __int128 data types. These macros will be used in providing support for
>> generating 128 bit masks.
>>
>> Cc: Yury Norov <yury.norov@...il.com>
>> Cc: Rasmus Villemoes <linux@...musvillemoes.dk>
>> Cc: Arnd Bergmann <arnd@...db.de>>
>> Cc: linux-kernel@...r.kernel.org
>> Cc: linux-arch@...r.kernel.org
>> Signed-off-by: Anshuman Khandual <anshuman.khandual@....com>
>> ---
>>  include/linux/bits.h                   | 2 ++
>>  include/uapi/asm-generic/bitsperlong.h | 2 ++
>>  include/uapi/linux/bits.h              | 3 +++
>>  include/uapi/linux/const.h             | 1 +
>>  4 files changed, 8 insertions(+)
>>
>> diff --git a/include/linux/bits.h b/include/linux/bits.h
>> index 0eb24d21aac2..0a174cce09d2 100644
>> --- a/include/linux/bits.h
>> +++ b/include/linux/bits.h
>> @@ -35,5 +35,7 @@
>>  	(GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
>>  #define GENMASK_ULL(h, l) \
>>  	(GENMASK_INPUT_CHECK(h, l) + __GENMASK_ULL(h, l))
>> +#define GENMASK_U128(h, l) \
>> +	(GENMASK_INPUT_CHECK(h, l) + __GENMASK_U128(h, l))
>>  
>>  #endif	/* __LINUX_BITS_H */
>> diff --git a/include/uapi/asm-generic/bitsperlong.h b/include/uapi/asm-generic/bitsperlong.h
>> index fadb3f857f28..6275367b17bb 100644
>> --- a/include/uapi/asm-generic/bitsperlong.h
>> +++ b/include/uapi/asm-generic/bitsperlong.h
>> @@ -28,4 +28,6 @@
>>  #define __BITS_PER_LONG_LONG 64
>>  #endif
>>  
>> +#define __BITS_PER_U128 128
> 
> Do we need such a macro for a fixed-width type? Even if we do, I'm not
> sure that a header named bitsperlong.h is a good place to host it.

__BITS_PER_U128 is being used anymore, will drop it.

> 
>> +
>>  #endif /* _UAPI__ASM_GENERIC_BITS_PER_LONG */
>> diff --git a/include/uapi/linux/bits.h b/include/uapi/linux/bits.h
>> index 3c2a101986a3..4d4b7b08003c 100644
>> --- a/include/uapi/linux/bits.h
>> +++ b/include/uapi/linux/bits.h
>> @@ -12,4 +12,7 @@
>>          (((~_ULL(0)) - (_ULL(1) << (l)) + 1) & \
>>           (~_ULL(0) >> (__BITS_PER_LONG_LONG - 1 - (h))))
>>  
>> +#define __GENMASK_U128(h, l) \
>> +	((_BIT128((h) + 1)) - (_BIT128(l)))
>> +
>>  #endif /* _UAPI_LINUX_BITS_H */
>> diff --git a/include/uapi/linux/const.h b/include/uapi/linux/const.h
>> index a429381e7ca5..a0211136dfd8 100644
>> --- a/include/uapi/linux/const.h
>> +++ b/include/uapi/linux/const.h
>> @@ -27,6 +27,7 @@
>>  
>>  #define _BITUL(x)	(_UL(1) << (x))
>>  #define _BITULL(x)	(_ULL(1) << (x))
>> +#define _BIT128(x)	((unsigned __int128)(1) << (x))
> 
> GENMASK() macros may be used in assembly code. This is not the case
> for GENMASK_128 at this time, of course, but I think we'd introduce 
> assembly glue at this point to simplify things in future. Can you
> check the include/uapi/linux/const.h and add something like _U128()
> in there?


https://lore.kernel.org/lkml/20240724103142.165693-1-anshuman.khandual@arm.com/

We had _U128() in the previous version V1 but as Arnd explained earlier
gcc silently truncates the constant passed into that helper. So _U128()
cannot take a real large 128 bit constant as the input.

--- a/include/uapi/linux/const.h
+++ b/include/uapi/linux/const.h
@@ -16,14 +16,17 @@
 #ifdef __ASSEMBLY__
 #define _AC(X,Y)	X
 #define _AT(T,X)	X
+#define _AC128(X)	X
 #else
 #define __AC(X,Y)	(X##Y)
 #define _AC(X,Y)	__AC(X,Y)
 #define _AT(T,X)	((T)(X))
+#define _AC128(X)	((unsigned __int128)(X))
 #endif
 
 #define _UL(x)		(_AC(x, UL))
 #define _ULL(x)		(_AC(x, ULL))
+#define _U128(x)	(_AC128(x))
 
 #define _BITUL(x)	(_UL(1) << (x))
 #define _BITULL(x)	(_ULL(1) << (x))

AFAICS unsigned __int128 based constants can only be formed via shifting
and merging operations involving two distinct user provided 64 bit parts.
Probably something like the following

#define _AC128(h, l)	(((unsigned __int128)h << 64) | (unsigned __int128)l)
#define _U128(h, l)	(_AC128(h, l))

But then carving out h and l components for the required 128 bit constant
needs to be done manually and for assembly the shifting operations has to
be platform specific. Hence just wondering if it is worth adding the macro
_U128().

> 
>>  
>>  #define __ALIGN_KERNEL(x, a)		__ALIGN_KERNEL_MASK(x, (__typeof__(x))(a) - 1)
>>  #define __ALIGN_KERNEL_MASK(x, mask)	(((x) + (mask)) & ~(mask))
>> -- 
>> 2.30.2