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

[Yury Norov <yury.norov@gmail.com>]

On Fri, Aug 16, 2024 at 11:58:04AM +0530, Anshuman Khandual wrote:
> 
> 
> On 8/1/24 12:46, 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
> > Reviewed-by: Arnd Bergmann <arnd@arndb.de>
> > Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
> > ---
> >  include/linux/bits.h       | 13 +++++++++++++
> >  include/uapi/linux/bits.h  |  3 +++
> >  include/uapi/linux/const.h | 15 +++++++++++++++
> >  3 files changed, 31 insertions(+)
> > 
> > diff --git a/include/linux/bits.h b/include/linux/bits.h
> > index 0eb24d21aac2..bf99feb5570e 100644
> > --- a/include/linux/bits.h
> > +++ b/include/linux/bits.h
> > @@ -36,4 +36,17 @@
> >  #define GENMASK_ULL(h, l) \
> >  	(GENMASK_INPUT_CHECK(h, l) + __GENMASK_ULL(h, l))
> >  
> > +/*
> > + * Missing asm support
> > + *
> > + * __GENMASK_U128() depends on _BIT128() which would not work
> > + * in the asm code, as it shifts an 'unsigned __init128' data
> > + * type instead of direct representation of 128 bit constants
> > + * such as long and unsigned long. The fundamental problem is
> > + * that a 128 bit constant will get silently truncated by the
> > + * gcc compiler.
> > + */
> > +#define GENMASK_U128(h, l) \
> > +	(GENMASK_INPUT_CHECK(h, l) + __GENMASK_U128(h, l))
> > +
> >  #endif	/* __LINUX_BITS_H */
> > 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..5be12e8f8f9c 100644
> > --- a/include/uapi/linux/const.h
> > +++ b/include/uapi/linux/const.h
> > @@ -28,6 +28,21 @@
> >  #define _BITUL(x)	(_UL(1) << (x))
> >  #define _BITULL(x)	(_ULL(1) << (x))
> >  
> > +/*
> > + * Missing asm support
> > + *
> > + * __BIT128() would not work in the asm code, as it shifts an
> > + * 'unsigned __init128' data type as direct representation of
> > + * 128 bit constants is not supported in the gcc compiler, as
> > + * they get silently truncated.
> > + *
> > + * TODO: Please revisit this implementation when gcc compiler
> > + * starts representing 128 bit constants directly like long
> > + * and unsigned long etc. Subsequently drop the comment for
> > + * GENMASK_U128() which would then start supporting asm code.
> > + */
> > +#define _BIT128(x)	((unsigned __int128)(1) << (x))
> > +
> >  #define __ALIGN_KERNEL(x, a)		__ALIGN_KERNEL_MASK(x, (__typeof__(x))(a) - 1)
> >  #define __ALIGN_KERNEL_MASK(x, mask)	(((x) + (mask)) & ~(mask))
> >  
> 
> Hello Yuri/Arnd,
> 
> This proposed GENMASK_U128(h, l) warns during build when the higher end
> bit is 127 (which in itself is a valid input).
> 
> ./include/uapi/linux/const.h:45:44: warning: left shift count >= width of type [-Wshift-count-overflow]
>    45 | #define _BIT128(x) ((unsigned __int128)(1) << (x))
>       |                                            ^~
> ./include/asm-generic/bug.h:123:25: note: in definition of macro ‘WARN_ON’
>   123 |  int __ret_warn_on = !!(condition);    \
>       |                         ^~~~~~~~~
> ./include/uapi/linux/bits.h:16:4: note: in expansion of macro ‘_BIT128’
>    16 |  ((_BIT128((h) + 1)) - (_BIT128(l)))
>       |    ^~~~~~~
> ./include/linux/bits.h:51:31: note: in expansion of macro ‘__GENMASK_U128’
>    51 |  (GENMASK_INPUT_CHECK(h, l) + __GENMASK_U128(h, l))
>       |                               ^~~~~~~~~~~~~~
> 
> This is caused by ((unsigned __int128)(1) << (128)) which is generated
> via (h + 1) element in __GENMASK_U128().
> 
> #define _BIT128(x)	((unsigned __int128)(1) << (x))
> #define __GENMASK_U128(h, l) \
> 	((_BIT128((h) + 1)) - (_BIT128(l)))
> 
> Adding some extra tests in lib/test_bits.c exposes this build problem,
> although it does not fail these new tests.
> 
> [    1.719221]     # Subtest: bits-test
> [    1.719291]     # module: test_bits
> [    1.720522]     ok 1 genmask_test
> [    1.721570]     ok 2 genmask_ull_test
> [    1.722668]     ok 3 genmask_u128_test
> [    1.723760]     ok 4 genmask_input_check_test
> [    1.723909] # bits-test: pass:4 fail:0 skip:0 total:4
> [    1.724101] ok 1 bits-test
> 
> diff --git a/lib/test_bits.c b/lib/test_bits.c
> index d3d858b24e02..7a972edc7122 100644
> --- a/lib/test_bits.c
> +++ b/lib/test_bits.c
> @@ -49,6 +49,8 @@ static void genmask_u128_test(struct kunit *test)
>         KUNIT_EXPECT_EQ(test, 0xffffffffffffffffULL, GENMASK_U128(63, 0));
>         KUNIT_EXPECT_EQ(test, 0xffffffffffffffffULL, GENMASK_U128(64, 0) >> 1);
>         KUNIT_EXPECT_EQ(test, 0x00000000ffffffffULL, GENMASK_U128(81, 50) >> 50);
> +       KUNIT_EXPECT_EQ(test, 0x0000000000000003ULL, GENMASK_U128(127, 126) >> 126);
> +       KUNIT_EXPECT_EQ(test, 0x0000000000000001ULL, GENMASK_U128(127, 127) >> 127);
> 
> The most significant bit in the generate mask can be added separately
> , thus voiding that extra shift. The following patch solves the build
> problem.
> 
> diff --git a/include/uapi/linux/bits.h b/include/uapi/linux/bits.h
> index 4d4b7b08003c..4e50f635c6d9 100644
> --- a/include/uapi/linux/bits.h
> +++ b/include/uapi/linux/bits.h
> @@ -13,6 +13,6 @@
>           (~_ULL(0) >> (__BITS_PER_LONG_LONG - 1 - (h))))
>  
>  #define __GENMASK_U128(h, l) \
> -       ((_BIT128((h) + 1)) - (_BIT128(l)))
> +       (((_BIT128(h)) - (_BIT128(l))) | (_BIT128(h)))

Can you send v3 with the fix? I will drop this series from bitmap-for-next
meanwhile.

Can you also extend the test for more? I'd like to check for example
the (127, 0) range. Also, can you please check both HI and LO parts 
the mask?

For the v3, please share the link to the following series that
actually uses new API.

Thanks,
Yury