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
