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

Yury Norov
2024-07-31T16:38:45Z

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@gmail.com>
> >> Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> >> Cc: Arnd Bergmann <arnd@arndb.de>>
> >> Cc: linux-kernel@vger.kernel.org
> >> Cc: linux-arch@vger.kernel.org
> >> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.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().

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