1. Patchew
  2. linux
  3. [PATCH V3 0/2] uapi: Add support for GENMASK_U128()
  4. View patch

[PATCH V3 1/2] uapi: Define GENMASK_U128

Anshuman Khandual posted 2 patches 1 year, 6 months ago
There is a newer version of this series
[PATCH V3 1/2] uapi: Define GENMASK_U128
Posted by Anshuman Khandual 1 year, 6 months ago 
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))
 
-- 
2.30.2
Re: [PATCH V3 1/2] uapi: Define GENMASK_U128
Posted by Anshuman Khandual 1 year, 5 months ago 

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)))
Re: [PATCH V3 1/2] uapi: Define GENMASK_U128
Posted by Arnd Bergmann 1 year, 5 months ago 
On Fri, Aug 16, 2024, at 08:28, Anshuman Khandual wrote:
>
> 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)))

Right, makes sense.

>
> 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)))

This could probably use a comment then, as it's less intuitive.

Another solution might be to use a double shift, as in

#define __GENMASK_U128(h, l) \
       ((_BIT128((h)) << 1) - (_BIT128(l)))

but I have not checked if this is correct for all inputs
or if it avoids the warning. Your version looks fine to
me otherwise.

    Arnd
Re: [PATCH V3 1/2] uapi: Define GENMASK_U128
Posted by Anshuman Khandual 1 year, 5 months ago 

On 8/19/24 12:43, Arnd Bergmann wrote:
> On Fri, Aug 16, 2024, at 08:28, Anshuman Khandual wrote:
>>
>> 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)))
> 
> Right, makes sense.
> 
>>
>> 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)))
> 
> This could probably use a comment then, as it's less intuitive.

Right, a comment explaining the need for this additional bit to
cover the corner 127 bit case could be added for reference.

> 
> Another solution might be to use a double shift, as in
> 
> #define __GENMASK_U128(h, l) \
>        ((_BIT128((h)) << 1) - (_BIT128(l)))

This looks much cleaner, passed all the tests without warning.
But for the above 127 bit case, wondering how the bit position
is managed after the second shift operation because it still
goes beyond __int128 element's 128 bit representation.

(_BIT128((127)) << 1)
(((unsigned __int128)(1) << (127)) << 1)

Should not the second shift operation warn about the possible
overflow scenario ? But actually it does not. Or the compiler
is too smart in detecting what's happening next in the overall
equation and do the needful while creating the mask below the
highest bit.

> 
> but I have not checked if this is correct for all inputs
> or if it avoids the warning. Your version looks fine to
> me otherwise.

This approach is much cleaner, passes all tests without warning,
unless something else shows up, will fold this in instead.
Re: [PATCH V3 1/2] uapi: Define GENMASK_U128
Posted by Arnd Bergmann 1 year, 5 months ago 
On Tue, Aug 20, 2024, at 03:25, Anshuman Khandual wrote:
> On 8/19/24 12:43, Arnd Bergmann wrote:
>
> Should not the second shift operation warn about the possible
> overflow scenario ? But actually it does not. Or the compiler
> is too smart in detecting what's happening next in the overall
> equation and do the needful while creating the mask below the
> highest bit.

Not sure about the reasoning behind the compiler warning for
one but not the other, but I know that we rely on similar
behavior in places like:

#define upper_32_bits(n) ((u32)(((n) >> 16) >> 16))

which is intended to return a zero without a compiler
warning when passing an 'unsigned long' input on 32-bit
architectures.

      Arnd
Re: [PATCH V3 1/2] uapi: Define GENMASK_U128
Posted by Yury Norov 1 year, 5 months ago 
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
Re: [PATCH V3 1/2] uapi: Define GENMASK_U128
Posted by Anshuman Khandual 1 year, 5 months ago 

On 8/17/24 19:27, Yury Norov wrote:
> 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.

Sure, will send out V4 (current series being V3).

> 
> 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?

Following tests form the complete set on GENMASK_U128(). The last four tests
here will be added in V4. Please also note that only 64 bit mask portion can
be tested at once.

        KUNIT_EXPECT_EQ(test, 0x0000000000ff0000ULL, GENMASK_U128(87, 80) >> 64);
        KUNIT_EXPECT_EQ(test, 0x0000000000ffffffULL, GENMASK_U128(87, 64) >> 64);
        KUNIT_EXPECT_EQ(test, 0x0000000000000001ULL, GENMASK_U128(0, 0));
        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);
        KUNIT_EXPECT_EQ(test, 0xffffffffffffffffULL, GENMASK_U128(127, 0) >> 64);
        KUNIT_EXPECT_EQ(test, 0xffffffffffffffffULL, GENMASK_U128(127, 0) & ~GENMASK_U128(127, 64));

Although, please do let me know if you would like to add some more tests.

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

Sure, will add the following link pointing to the work in progress on arm64.

https://lore.kernel.org/all/20240801054436.612024-1-anshuman.khandual@arm.com/

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.