[PATCH V2 0/2] uapi: Add support for GENMASK_U128()

[PATCH V2 0/2] uapi: Add support for GENMASK_U128()

[Anshuman Khandual]

This adds support for GENMASK_U128() and some corresponding tests as well.
GENMASK_U128() generated 128 bit masks will be required later on the arm64
platform for enabling FEAT_SYSREG128 and FEAT_D128 features.

Because GENMAKS_U128() depends on __int128 data type being supported in the
compiler, its usage needs to be protected with CONFIG_ARCH_SUPPORTS_INT128.

Cc: Andrew Morton <akpm@linux-foundation.org>
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

Changes in V2:

- Wrapped genmask_u128_test() with CONFIG_ARCH_SUPPORTS_INT128
- Defined __BITS_PER_U128 unconditionally as 128
- Defined __GENMASK_U128() via new _BIT128()
- Dropped _U128() and _AC128()

Changes in V1:

https://lore.kernel.org/lkml/20240724103142.165693-1-anshuman.khandual@arm.com/

Anshuman Khandual (2):
  uapi: Define GENMASK_U128
  lib/test_bits.c: Add tests for GENMASK_U128()

 include/linux/bits.h                   |  2 ++
 include/uapi/asm-generic/bitsperlong.h |  2 ++
 include/uapi/linux/bits.h              |  3 +++
 include/uapi/linux/const.h             |  1 +
 lib/test_bits.c                        | 25 +++++++++++++++++++++++++
 5 files changed, 33 insertions(+)

-- 
2.30.2

[PATCH V2 1/2] uapi: Define GENMASK_U128

[Anshuman Khandual]

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
+
 #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))
 
 #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 V2 1/2] uapi: Define GENMASK_U128

[Yury Norov]

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.

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

>  
>  #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 V2 1/2] uapi: Define GENMASK_U128

[Anshuman Khandual]

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

> 
>>  
>>  #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 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@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

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

[Arnd Bergmann]

On Wed, Jul 31, 2024, at 18:38, Yury Norov wrote:

> OK then, I see. So, is that a GCC bug or intentional behavior? 

I just double-checked to see what is in C23 and found that
it indeed defines a 'wb' and 'wbu' suffix for arbitrarily
sized integers. This is supported by gcc-14 and clang-15
or higher on 64-bit architectures.

It's too early to rely on those versions though, as gcc-14
was only just released.

     Arnd

[PATCH V2 2/2] lib/test_bits.c: Add tests for GENMASK_U128()

[Anshuman Khandual]

This adds GENMASK_U128() tests although currently only 64 bit wide masks
are being tested.

Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
---
 lib/test_bits.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/lib/test_bits.c b/lib/test_bits.c
index 01313980f175..f0d1033cf3c9 100644
--- a/lib/test_bits.c
+++ b/lib/test_bits.c
@@ -39,6 +39,26 @@ static void genmask_ull_test(struct kunit *test)
 #endif
 }
 
+#ifdef CONFIG_ARCH_SUPPORTS_INT128
+static void genmask_u128_test(struct kunit *test)
+{
+	/* Tests mask generation only when the mask width is within 64 bits */
+	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);
+
+#ifdef TEST_GENMASK_FAILURES
+	/* these should fail compilation */
+	GENMASK_U128(0, 1);
+	GENMASK_U128(0, 10);
+	GENMASK_U128(9, 10);
+#endif
+}
+#endif
+
 static void genmask_input_check_test(struct kunit *test)
 {
 	unsigned int x, y;
@@ -56,12 +76,17 @@ static void genmask_input_check_test(struct kunit *test)
 	/* Valid input */
 	KUNIT_EXPECT_EQ(test, 0, GENMASK_INPUT_CHECK(1, 1));
 	KUNIT_EXPECT_EQ(test, 0, GENMASK_INPUT_CHECK(39, 21));
+	KUNIT_EXPECT_EQ(test, 0, GENMASK_INPUT_CHECK(100, 80));
+	KUNIT_EXPECT_EQ(test, 0, GENMASK_INPUT_CHECK(110, 65));
 }
 
 
 static struct kunit_case bits_test_cases[] = {
 	KUNIT_CASE(genmask_test),
 	KUNIT_CASE(genmask_ull_test),
+#ifdef CONFIG_ARCH_SUPPORTS_INT128
+	KUNIT_CASE(genmask_u128_test),
+#endif
 	KUNIT_CASE(genmask_input_check_test),
 	{}
 };
-- 
2.30.2

Re: [PATCH V2 2/2] lib/test_bits.c: Add tests for GENMASK_U128()

[Yury Norov]

On Thu, Jul 25, 2024 at 11:18:08AM +0530, Anshuman Khandual wrote:
> This adds GENMASK_U128() tests although currently only 64 bit wide masks
> are being tested.
> 
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
> ---
>  lib/test_bits.c | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 
> diff --git a/lib/test_bits.c b/lib/test_bits.c
> index 01313980f175..f0d1033cf3c9 100644
> --- a/lib/test_bits.c
> +++ b/lib/test_bits.c
> @@ -39,6 +39,26 @@ static void genmask_ull_test(struct kunit *test)
>  #endif
>  }
>  
> +#ifdef CONFIG_ARCH_SUPPORTS_INT128

Can you move this ifdefery inside the function scope, so that you'll
not have do it below in tests array declarattion?

> +static void genmask_u128_test(struct kunit *test)
> +{
> +	/* Tests mask generation only when the mask width is within 64 bits */
> +	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);
> +
> +#ifdef TEST_GENMASK_FAILURES
> +	/* these should fail compilation */
> +	GENMASK_U128(0, 1);
> +	GENMASK_U128(0, 10);
> +	GENMASK_U128(9, 10);
> +#endif
> +}
> +#endif
> +
>  static void genmask_input_check_test(struct kunit *test)
>  {
>  	unsigned int x, y;
> @@ -56,12 +76,17 @@ static void genmask_input_check_test(struct kunit *test)
>  	/* Valid input */
>  	KUNIT_EXPECT_EQ(test, 0, GENMASK_INPUT_CHECK(1, 1));
>  	KUNIT_EXPECT_EQ(test, 0, GENMASK_INPUT_CHECK(39, 21));
> +	KUNIT_EXPECT_EQ(test, 0, GENMASK_INPUT_CHECK(100, 80));
> +	KUNIT_EXPECT_EQ(test, 0, GENMASK_INPUT_CHECK(110, 65));
>  }
>  
>  
>  static struct kunit_case bits_test_cases[] = {
>  	KUNIT_CASE(genmask_test),
>  	KUNIT_CASE(genmask_ull_test),
> +#ifdef CONFIG_ARCH_SUPPORTS_INT128
> +	KUNIT_CASE(genmask_u128_test),
> +#endif
>  	KUNIT_CASE(genmask_input_check_test),
>  	{}
>  };
> -- 
> 2.30.2

Re: [PATCH V2 2/2] lib/test_bits.c: Add tests for GENMASK_U128()

[Anshuman Khandual]

On 7/30/24 23:50, Yury Norov wrote:
> On Thu, Jul 25, 2024 at 11:18:08AM +0530, Anshuman Khandual wrote:
>> This adds GENMASK_U128() tests although currently only 64 bit wide masks
>> are being tested.
>>
>> Cc: Andrew Morton <akpm@linux-foundation.org>
>> Cc: linux-kernel@vger.kernel.org
>> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
>> ---
>>  lib/test_bits.c | 25 +++++++++++++++++++++++++
>>  1 file changed, 25 insertions(+)
>>
>> diff --git a/lib/test_bits.c b/lib/test_bits.c
>> index 01313980f175..f0d1033cf3c9 100644
>> --- a/lib/test_bits.c
>> +++ b/lib/test_bits.c
>> @@ -39,6 +39,26 @@ static void genmask_ull_test(struct kunit *test)
>>  #endif
>>  }
>>  
>> +#ifdef CONFIG_ARCH_SUPPORTS_INT128
> 
> Can you move this ifdefery inside the function scope, so that you'll
> not have do it below in tests array declarattion?

Sure, will fold in the following changes in here.

diff --git a/lib/test_bits.c b/lib/test_bits.c
index f0d1033cf3c9..f8b058974d07 100644
--- a/lib/test_bits.c
+++ b/lib/test_bits.c
@@ -39,9 +39,9 @@ static void genmask_ull_test(struct kunit *test)
 #endif
 }
 
-#ifdef CONFIG_ARCH_SUPPORTS_INT128
 static void genmask_u128_test(struct kunit *test)
 {
+#ifdef CONFIG_ARCH_SUPPORTS_INT128
        /* Tests mask generation only when the mask width is within 64 bits */
        KUNIT_EXPECT_EQ(test, 0x0000000000ff0000ULL, GENMASK_U128(87, 80) >> 64);
        KUNIT_EXPECT_EQ(test, 0x0000000000ffffffULL, GENMASK_U128(87, 64) >> 64);
@@ -56,8 +56,8 @@ static void genmask_u128_test(struct kunit *test)
        GENMASK_U128(0, 10);
        GENMASK_U128(9, 10);
 #endif
-}
 #endif
+}
 
 static void genmask_input_check_test(struct kunit *test)
 {
@@ -84,9 +84,7 @@ static void genmask_input_check_test(struct kunit *test)
 static struct kunit_case bits_test_cases[] = {
        KUNIT_CASE(genmask_test),
        KUNIT_CASE(genmask_ull_test),
-#ifdef CONFIG_ARCH_SUPPORTS_INT128
        KUNIT_CASE(genmask_u128_test),
-#endif
        KUNIT_CASE(genmask_input_check_test),
        {}
 };


> 
>> +static void genmask_u128_test(struct kunit *test)
>> +{
>> +	/* Tests mask generation only when the mask width is within 64 bits */
>> +	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);
>> +
>> +#ifdef TEST_GENMASK_FAILURES
>> +	/* these should fail compilation */
>> +	GENMASK_U128(0, 1);
>> +	GENMASK_U128(0, 10);
>> +	GENMASK_U128(9, 10);
>> +#endif
>> +}
>> +#endif
>> +
>>  static void genmask_input_check_test(struct kunit *test)
>>  {
>>  	unsigned int x, y;
>> @@ -56,12 +76,17 @@ static void genmask_input_check_test(struct kunit *test)
>>  	/* Valid input */
>>  	KUNIT_EXPECT_EQ(test, 0, GENMASK_INPUT_CHECK(1, 1));
>>  	KUNIT_EXPECT_EQ(test, 0, GENMASK_INPUT_CHECK(39, 21));
>> +	KUNIT_EXPECT_EQ(test, 0, GENMASK_INPUT_CHECK(100, 80));
>> +	KUNIT_EXPECT_EQ(test, 0, GENMASK_INPUT_CHECK(110, 65));
>>  }
>>  
>>  
>>  static struct kunit_case bits_test_cases[] = {
>>  	KUNIT_CASE(genmask_test),
>>  	KUNIT_CASE(genmask_ull_test),
>> +#ifdef CONFIG_ARCH_SUPPORTS_INT128
>> +	KUNIT_CASE(genmask_u128_test),
>> +#endif
>>  	KUNIT_CASE(genmask_input_check_test),
>>  	{}
>>  };
>> -- 
>> 2.30.2

Re: [PATCH V2 0/2] uapi: Add support for GENMASK_U128()

[Andrew Morton]

On Thu, 25 Jul 2024 11:18:06 +0530 Anshuman Khandual <anshuman.khandual@arm.com> wrote:

> This adds support for GENMASK_U128() and some corresponding tests as well.
> GENMASK_U128() generated 128 bit masks will be required later on the arm64
> platform for enabling FEAT_SYSREG128 and FEAT_D128 features.

If this will be required for ongoing ARM development prior to the 6.11
release then it would make sense for these changes to be carried in the
relevant ARM tree.

Re: [PATCH V2 0/2] uapi: Add support for GENMASK_U128()

[Anshuman Khandual]

On 7/26/24 02:35, Andrew Morton wrote:
> On Thu, 25 Jul 2024 11:18:06 +0530 Anshuman Khandual <anshuman.khandual@arm.com> wrote:
> 
>> This adds support for GENMASK_U128() and some corresponding tests as well.
>> GENMASK_U128() generated 128 bit masks will be required later on the arm64
>> platform for enabling FEAT_SYSREG128 and FEAT_D128 features.
> 
> If this will be required for ongoing ARM development prior to the 6.11
> release then it would make sense for these changes to be carried in the
> relevant ARM tree.

Hello Andrew,

These changes are not required prior to 6.11, hence being taken via the mm
tree is fine.

- Anshuman

Re: [PATCH V2 0/2] uapi: Add support for GENMASK_U128()

[Anshuman Khandual]

On 7/25/24 11:18, Anshuman Khandual wrote:
> This adds support for GENMASK_U128() and some corresponding tests as well.
> GENMASK_U128() generated 128 bit masks will be required later on the arm64
> platform for enabling FEAT_SYSREG128 and FEAT_D128 features.
> 
> Because GENMAKS_U128() depends on __int128 data type being supported in the
> compiler, its usage needs to be protected with CONFIG_ARCH_SUPPORTS_INT128.
> 
> Cc: Andrew Morton <akpm@linux-foundation.org>
> 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
> 
> Changes in V2:
> 
> - Wrapped genmask_u128_test() with CONFIG_ARCH_SUPPORTS_INT128
> - Defined __BITS_PER_U128 unconditionally as 128
> - Defined __GENMASK_U128() via new _BIT128()
> - Dropped _U128() and _AC128()

Hello Arnd,

Does the changed series look good ? Please do let me know if something
further needs to be changed. Thank you.

- Anshuman

Re: [PATCH V2 0/2] uapi: Add support for GENMASK_U128()

[Arnd Bergmann]

On Tue, Jul 30, 2024, at 06:29, Anshuman Khandual wrote:
> On 7/25/24 11:18, Anshuman Khandual wrote:
>> 
>> - Wrapped genmask_u128_test() with CONFIG_ARCH_SUPPORTS_INT128
>> - Defined __BITS_PER_U128 unconditionally as 128
>> - Defined __GENMASK_U128() via new _BIT128()
>> - Dropped _U128() and _AC128()
>
> Does the changed series look good ? Please do let me know if something
> further needs to be changed. Thank you.

Yes, these look fine to me, please add

Reviewed-by: Arnd Bergmann <arnd@arndb.de>

One detail: You are not actually using __BITS_PER_U128 at
all now, so I think it would be better to not add it at all.

Re: [PATCH V2 0/2] uapi: Add support for GENMASK_U128()

[Anshuman Khandual]

On 7/30/24 17:59, Arnd Bergmann wrote:
> On Tue, Jul 30, 2024, at 06:29, Anshuman Khandual wrote:
>> On 7/25/24 11:18, Anshuman Khandual wrote:
>>>
>>> - Wrapped genmask_u128_test() with CONFIG_ARCH_SUPPORTS_INT128
>>> - Defined __BITS_PER_U128 unconditionally as 128
>>> - Defined __GENMASK_U128() via new _BIT128()
>>> - Dropped _U128() and _AC128()
>>
>> Does the changed series look good ? Please do let me know if something
>> further needs to be changed. Thank you.
> 
> Yes, these look fine to me, please add
> 
> Reviewed-by: Arnd Bergmann <arnd@arndb.de>

Thanks Arnd.

> 
> One detail: You are not actually using __BITS_PER_U128 at
> all now, so I think it would be better to not add it at all.

Sure, will do, missed that some how.