Re: [PATCH V2 1/2] uapi: Define GENMASK_U128

[Yury Norov]

On Wed, Jul 31, 2024 at 09:14:54AM +0530, Anshuman Khandual wrote:
> 
> 
> 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@xxxxxxxxx>
> >> Cc: Rasmus Villemoes <linux@xxxxxxxxxxxxxxxxxx>
> >> Cc: Arnd Bergmann <arnd@xxxxxxxx>>
> >> Cc: linux-kernel@xxxxxxxxxxxxxxx
> >> Cc: linux-arch@xxxxxxxxxxxxxxx
> >> Signed-off-by: Anshuman Khandual <anshuman.khandual@xxxxxxx>
> >> ---
> >>  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@xxxxxxx/
> 
> 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().

OK then, I see. So, is that a GCC bug or intentional behavior? Anyways,
can you put a comment on top of GENMASK_U128 and BIT128 that they wouldn't
work in asm code and why?

Thanks,
Yury