[PATCH v4 0/8] bits: Fixed-type GENMASK()/BIT()

[PATCH v4 0/8] bits: Fixed-type GENMASK()/BIT()

[Vincent Mailhol via B4 Relay]

Introduce some fixed width variant of the GENMASK() and the BIT()
macros in bits.h. Note that the main goal is not to get the correct
type, but rather to enforce more checks at compile time. For example:

  GENMASK_U16(16, 0)

will raise a build bug.

This series is a continuation of:

  https://lore.kernel.org/intel-xe/20240208074521.577076-1-lucas.demarchi@intel.com

from Lucas De Marchi. Above series is one year old. I really think
that this was a good idea and I do not want this series to die. So I
am volunteering to revive it.

Meanwhile, many changes occurred in bits.h. The most significant
change is that __GENMASK() was moved to the uapi headers.

In this v4, I introduce one big change: split the definition of the
asm and non-asm GENMASK(). I think this is controversial. Especially,
Yuri commented that he did not want such split. So I initially
implemented a first draft in which both the asm and non-asm version
would rely on the same helper macro, i.e. adding this:

  #define __GENMASK_t(t, w, h, l)			\
  	(((t)~_ULL(0) - ((t)1 << (l)) + 1) &		\
  	 ((t)~_ULL(0) >> (w - 1 - (h))))
    
to uapi/bits.h. And then, the different GENMASK()s would look like
this:

  #define __GENMASK(h, l) __GENMASK_t(unsigned long, __BITS_PER_LONG, h, l)
    
and so on.
    
I implemented it, and the final result looks quite ugly. Not only do
we need to manually provide the width each time, the biggest concern
is that adding this to the uapi is asking for trouble. Who knows how
people are going to use this? And once it is in the uapi, there is
virtually no way back.

Finally, I do not think it makes sense to expose the fixed width
variants to the asm. The fixed width integers type are a C
concept. For asm, the long and long long variants seems sufficient.

And so, after implementing both, the asm and non-asm split seems way
more clean and I think this is the best compromise. Let me know what
you think :)

Changes from v3:

        - Rebase on v6.14-rc5

        - Fix a typo in GENMASK_U128() comment.

        - Split the asm and non-asm definition of 

        - Replace ~0ULL by ~ULL(0)

        - Since v3, __GENMASK() was moved to the uapi and people
          started using directly. Introduce GENMASK_t() instead.

v3: https://lore.kernel.org/intel-xe/20240208074521.577076-1-lucas.demarchi@intel.com

Changes from v2:

	- Document both in commit message and code about the strict type
	  checking and give examples how it´d break with invalid params.

v2: https://lore.kernel.org/intel-xe/20240124050205.3646390-1-lucas.demarchi@intel.com
v1: https://lore.kernel.org/intel-xe/20230509051403.2748545-1-lucas.demarchi@intel.com
--
2.43.0

---
Lucas De Marchi (3):
      bits: introduce fixed-type BIT
      drm/i915: Convert REG_GENMASK* to fixed-width GENMASK_*
      test_bits: add tests for fixed-type genmasks

Vincent Mailhol (4):
      bits: fix typo 'unsigned __init128' -> 'unsigned __int128'
      bits: split the definition of the asm and non-asm GENMASK()
      test_bits: add tests for __GENMASK() and __GENMASK_ULL()
      test_bits: add tests for fixed-type BIT

Yury Norov (1):
      bits: introduce fixed-type genmasks

 drivers/gpu/drm/i915/i915_reg_defs.h | 108 ++++-------------------------------
 include/linux/bitops.h               |   1 -
 include/linux/bits.h                 |  65 +++++++++++++++++----
 lib/test_bits.c                      |  47 +++++++++++++++
 4 files changed, 111 insertions(+), 110 deletions(-)
---
base-commit: 7eb172143d5508b4da468ed59ee857c6e5e01da6
change-id: 20250228-fixed-type-genmasks-8d1a555f34e8

Best regards,
-- 
Vincent Mailhol <mailhol.vincent@wanadoo.fr>

[PATCH v4 1/8] bits: fix typo 'unsigned __init128' -> 'unsigned __int128'

[Vincent Mailhol via B4 Relay]

From: Vincent Mailhol <mailhol.vincent@wanadoo.fr>

"int" was misspelled as "init" in GENMASK_U128() comments. Fix the typo.

Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
 include/linux/bits.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/bits.h b/include/linux/bits.h
index 61a75d3f294bfa96267383b5e2fd2a5d4593fcee..14fd0ca9a6cd17339dd2f69e449558312a8a001b 100644
--- a/include/linux/bits.h
+++ b/include/linux/bits.h
@@ -40,7 +40,7 @@
  * Missing asm support
  *
  * __GENMASK_U128() depends on _BIT128() which would not work
- * in the asm code, as it shifts an 'unsigned __init128' data
+ * in the asm code, as it shifts an 'unsigned __int128' 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

-- 
2.45.3

Re: [PATCH v4 1/8] bits: fix typo 'unsigned __init128' -> 'unsigned __int128'

[Yury Norov]

Hi Vincent!

On Wed, Mar 05, 2025 at 10:00:13PM +0900, Vincent Mailhol via B4 Relay wrote:
> From: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> 
> "int" was misspelled as "init" in GENMASK_U128() comments. Fix the typo.
> 
> Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>

Thanks for respinning the series. I'll take this fix in bitmap-for-next, so
if you need v2, you'll not have to bear this thing too.

Thanks,
Yury

> ---
>  include/linux/bits.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/linux/bits.h b/include/linux/bits.h
> index 61a75d3f294bfa96267383b5e2fd2a5d4593fcee..14fd0ca9a6cd17339dd2f69e449558312a8a001b 100644
> --- a/include/linux/bits.h
> +++ b/include/linux/bits.h
> @@ -40,7 +40,7 @@
>   * Missing asm support
>   *
>   * __GENMASK_U128() depends on _BIT128() which would not work
> - * in the asm code, as it shifts an 'unsigned __init128' data
> + * in the asm code, as it shifts an 'unsigned __int128' 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
> 
> -- 
> 2.45.3
> 

Re: [PATCH v4 1/8] bits: fix typo 'unsigned __init128' -> 'unsigned __int128'

[Andy Shevchenko]

On Wed, Mar 05, 2025 at 09:30:20AM -0500, Yury Norov wrote:
> On Wed, Mar 05, 2025 at 10:00:13PM +0900, Vincent Mailhol via B4 Relay wrote:
> > From: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> > 
> > "int" was misspelled as "init" in GENMASK_U128() comments. Fix the typo.
> 
> Thanks for respinning the series. I'll take this fix in bitmap-for-next, so
> if you need v2, you'll not have to bear this thing too.

Before doing that, please read my comment first.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 1/8] bits: fix typo 'unsigned __init128' -> 'unsigned __int128'

[Yury Norov]

On Wed, Mar 05, 2025 at 04:36:12PM +0200, Andy Shevchenko wrote:
> On Wed, Mar 05, 2025 at 09:30:20AM -0500, Yury Norov wrote:
> > On Wed, Mar 05, 2025 at 10:00:13PM +0900, Vincent Mailhol via B4 Relay wrote:
> > > From: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> > > 
> > > "int" was misspelled as "init" in GENMASK_U128() comments. Fix the typo.
> > 
> > Thanks for respinning the series. I'll take this fix in bitmap-for-next, so
> > if you need v2, you'll not have to bear this thing too.
> 
> Before doing that, please read my comment first.

Already did. Yes, you're right.

Vincent, can you send the fix separately, so I'll move it in the
upcoming merge window?

Re: [PATCH v4 1/8] bits: fix typo 'unsigned __init128' -> 'unsigned __int128'

[Vincent Mailhol]

On 05/03/2025 at 23:38, Yury Norov wrote:
> On Wed, Mar 05, 2025 at 04:36:12PM +0200, Andy Shevchenko wrote:
>> On Wed, Mar 05, 2025 at 09:30:20AM -0500, Yury Norov wrote:
>>> On Wed, Mar 05, 2025 at 10:00:13PM +0900, Vincent Mailhol via B4 Relay wrote:
>>>> From: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
>>>>
>>>> "int" was misspelled as "init" in GENMASK_U128() comments. Fix the typo.
>>>
>>> Thanks for respinning the series. I'll take this fix in bitmap-for-next, so
>>> if you need v2, you'll not have to bear this thing too.
>>
>> Before doing that, please read my comment first.
> 
> Already did. Yes, you're right.
> 
> Vincent, can you send the fix separately, so I'll move it in the
> upcoming merge window?

Here it is:
https://lore.kernel.org/all/20250305-fix_init128_typo-v1-1-cbe5b8e54e7d@wanadoo.fr/

As requested, I will exclude this from the v5.


Yours sincerely,
Vincent Mailhol

Re: [PATCH v4 1/8] bits: fix typo 'unsigned __init128' -> 'unsigned __int128'

[Andy Shevchenko]

On Wed, Mar 05, 2025 at 10:00:13PM +0900, Vincent Mailhol via B4 Relay wrote:
> From: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> 
> "int" was misspelled as "init" in GENMASK_U128() comments. Fix the typo.

Please, fix it everywhere:

$ git grep -lw __init128
include/linux/bits.h
include/uapi/linux/const.h
tools/include/linux/bits.h
tools/include/uapi/linux/const.h

-- 
With Best Regards,
Andy Shevchenko

[PATCH v4 2/8] bits: split the definition of the asm and non-asm GENMASK()

[Vincent Mailhol via B4 Relay]

From: Vincent Mailhol <mailhol.vincent@wanadoo.fr>

In an upcoming change, GENMASK() and its friends will indirectly
depend on sizeof() which is not available in asm.

Instead of adding further complexity to __GENMASK() to make it work
for both asm and non asm, just split the definition of the two
variants.

Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
 include/linux/bits.h | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/include/linux/bits.h b/include/linux/bits.h
index 14fd0ca9a6cd17339dd2f69e449558312a8a001b..5f68980a1b98d771426872c74d7b5c0f79e5e802 100644
--- a/include/linux/bits.h
+++ b/include/linux/bits.h
@@ -19,23 +19,17 @@
  * GENMASK_ULL(39, 21) gives us the 64bit vector 0x000000ffffe00000.
  */
 #if !defined(__ASSEMBLY__)
+
 #include <linux/build_bug.h>
 #include <linux/compiler.h>
+
 #define GENMASK_INPUT_CHECK(h, l) BUILD_BUG_ON_ZERO(const_true((l) > (h)))
-#else
-/*
- * BUILD_BUG_ON_ZERO is not available in h files included from asm files,
- * disable the input check if that is the case.
- */
-#define GENMASK_INPUT_CHECK(h, l) 0
-#endif
 
 #define GENMASK(h, l) \
 	(GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
 #define GENMASK_ULL(h, l) \
 	(GENMASK_INPUT_CHECK(h, l) + __GENMASK_ULL(h, l))
 
-#if !defined(__ASSEMBLY__)
 /*
  * Missing asm support
  *
@@ -48,6 +42,12 @@
  */
 #define GENMASK_U128(h, l) \
 	(GENMASK_INPUT_CHECK(h, l) + __GENMASK_U128(h, l))
-#endif
+
+#else /* defined(__ASSEMBLY__) */
+
+#define GENMASK(h, l) __GENMASK(h, l)
+#define GENMASK_ULL(h, l) __GENMASK_ULL(h, l)
+
+#endif /* !defined(__ASSEMBLY__) */
 
 #endif	/* __LINUX_BITS_H */

-- 
2.45.3

[PATCH v4 3/8] bits: introduce fixed-type genmasks

[Vincent Mailhol via B4 Relay]

From: Yury Norov <yury.norov@gmail.com>

Add __GENMASK_t() which generalizes __GENMASK() to support different
types, and implement fixed-types versions of GENMASK() based on it.
The fixed-type version allows more strict checks to the min/max values
accepted, which is useful for defining registers like implemented by
i915 and xe drivers with their REG_GENMASK*() macros.

The strict checks rely on shift-count-overflow compiler check to fail
the build if a number outside of the range allowed is passed.
Example:

	#define FOO_MASK GENMASK_U32(33, 4)

will generate a warning like:

	../include/linux/bits.h:41:31: error: left shift count >= width of type [-Werror=shift-count-overflow]
	   41 |          (((t)~0ULL - ((t)(1) << (l)) + 1) & \
	      |                               ^~

Signed-off-by: Yury Norov <yury.norov@gmail.com>
Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
Acked-by: Jani Nikula <jani.nikula@intel.com>
Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
Changelog:

  v3 -> v4:

    - The v3 is one year old. Meanwhile people started using
      __GENMASK() directly. So instead of generalizing __GENMASK() to
      support different types, add a new GENMASK_t().

    - replace ~0ULL by ~_ULL(0). Otherwise, __GENMASK_t() would fail
      in asm code.

    - Make GENMASK_U8() and GENMASK_U16() return an unsigned int. In
      v3, due to the integer promotion rules, these were returning a
      signed integer. By casting these to unsigned int, at least the
      signedness is kept.
---
 include/linux/bitops.h |  1 -
 include/linux/bits.h   | 33 +++++++++++++++++++++++++++++----
 2 files changed, 29 insertions(+), 5 deletions(-)

diff --git a/include/linux/bitops.h b/include/linux/bitops.h
index c1cb53cf2f0f8662ed3e324578f74330e63f935d..9be2d50da09a417966b3d11c84092bb2f4cd0bef 100644
--- a/include/linux/bitops.h
+++ b/include/linux/bitops.h
@@ -8,7 +8,6 @@
 
 #include <uapi/linux/kernel.h>
 
-#define BITS_PER_TYPE(type)	(sizeof(type) * BITS_PER_BYTE)
 #define BITS_TO_LONGS(nr)	__KERNEL_DIV_ROUND_UP(nr, BITS_PER_TYPE(long))
 #define BITS_TO_U64(nr)		__KERNEL_DIV_ROUND_UP(nr, BITS_PER_TYPE(u64))
 #define BITS_TO_U32(nr)		__KERNEL_DIV_ROUND_UP(nr, BITS_PER_TYPE(u32))
diff --git a/include/linux/bits.h b/include/linux/bits.h
index 5f68980a1b98d771426872c74d7b5c0f79e5e802..f202e46d2f4b7899c16d975120f3fa3ae41556ae 100644
--- a/include/linux/bits.h
+++ b/include/linux/bits.h
@@ -12,6 +12,7 @@
 #define BIT_ULL_MASK(nr)	(ULL(1) << ((nr) % BITS_PER_LONG_LONG))
 #define BIT_ULL_WORD(nr)	((nr) / BITS_PER_LONG_LONG)
 #define BITS_PER_BYTE		8
+#define BITS_PER_TYPE(type)	(sizeof(type) * BITS_PER_BYTE)
 
 /*
  * Create a contiguous bitmask starting at bit position @l and ending at
@@ -25,14 +26,38 @@
 
 #define GENMASK_INPUT_CHECK(h, l) BUILD_BUG_ON_ZERO(const_true((l) > (h)))
 
-#define GENMASK(h, l) \
-	(GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
-#define GENMASK_ULL(h, l) \
-	(GENMASK_INPUT_CHECK(h, l) + __GENMASK_ULL(h, l))
+/*
+ * Generate a mask for the specified type @t. Additional checks are made to
+ * guarantee the value returned fits in that type, relying on
+ * shift-count-overflow compiler check to detect incompatible arguments.
+ * For example, all these create build errors or warnings:
+ *
+ * - GENMASK(15, 20): wrong argument order
+ * - GENMASK(72, 15): doesn't fit unsigned long
+ * - GENMASK_U32(33, 15): doesn't fit in a u32
+ */
+#define GENMASK_t(t, h, l)				\
+	(GENMASK_INPUT_CHECK(h, l) +			\
+	 (((t)~ULL(0) - ((t)1 << (l)) + 1) &		\
+	  ((t)~ULL(0) >> (BITS_PER_TYPE(t) - 1 - (h)))))
+
+#define GENMASK(h, l) GENMASK_t(unsigned long,  h, l)
+#define GENMASK_ULL(h, l) GENMASK_t(unsigned long long, h, l)
 
 /*
  * Missing asm support
  *
+ * __GENMASK_U*() depends on BITS_PER_TYPE() which would not work in the asm
+ * code as BITS_PER_TYPE() relies on sizeof(), something not available in
+ * asm. Nethertheless, the concept of fixed width integers is a C thing which
+ * does not apply to assembly code.
+ */
+#define GENMASK_U8(h, l) ((unsigned int)GENMASK_t(u8,  h, l))
+#define GENMASK_U16(h, l) ((unsigned int)GENMASK_t(u16, h, l))
+#define GENMASK_U32(h, l) GENMASK_t(u32, h, l)
+#define GENMASK_U64(h, l) GENMASK_t(u64, h, l)
+
+/*
  * __GENMASK_U128() depends on _BIT128() which would not work
  * in the asm code, as it shifts an 'unsigned __int128' data
  * type instead of direct representation of 128 bit constants

-- 
2.45.3

Re: [PATCH v4 3/8] bits: introduce fixed-type genmasks

[Andy Shevchenko]

On Wed, Mar 05, 2025 at 10:00:15PM +0900, Vincent Mailhol via B4 Relay wrote:
> From: Yury Norov <yury.norov@gmail.com>
> 
> Add __GENMASK_t() which generalizes __GENMASK() to support different

Is it with double underscore? I do not see it.

_t is used for typedef simple types. It's unfortunate to have it
in such a macro. Perhaps T or TYPE will suffice. Or perhaps we want
__GENMASK_Uxx() here?

> types, and implement fixed-types versions of GENMASK() based on it.
> The fixed-type version allows more strict checks to the min/max values
> accepted, which is useful for defining registers like implemented by
> i915 and xe drivers with their REG_GENMASK*() macros.
> 
> The strict checks rely on shift-count-overflow compiler check to fail
> the build if a number outside of the range allowed is passed.
> Example:
> 
> 	#define FOO_MASK GENMASK_U32(33, 4)
> 
> will generate a warning like:
> 
> 	../include/linux/bits.h:41:31: error: left shift count >= width of type [-Werror=shift-count-overflow]
> 	   41 |          (((t)~0ULL - ((t)(1) << (l)) + 1) & \
> 	      |                               ^~

...

> + * __GENMASK_U*() depends on BITS_PER_TYPE() which would not work in the asm

Where are the double underscore variants? I see it only for U128.

> + * code as BITS_PER_TYPE() relies on sizeof(), something not available in
> + * asm. Nethertheless, the concept of fixed width integers is a C thing which
> + * does not apply to assembly code.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 3/8] bits: introduce fixed-type genmasks

[Vincent Mailhol]

On 05/03/2025 at 23:30, Andy Shevchenko wrote:
> On Wed, Mar 05, 2025 at 10:00:15PM +0900, Vincent Mailhol via B4 Relay wrote:
>> From: Yury Norov <yury.norov@gmail.com>
>>
>> Add __GENMASK_t() which generalizes __GENMASK() to support different
> 
> Is it with double underscore? I do not see it.

This is my mistake. In an earlier draft, it was __GENMASK_t(),
meanwhile, I dropped the __ prefix but forget to update the patch
description.

> _t is used for typedef simple types. It's unfortunate to have it
> in such a macro.

Ack.

> Perhaps T or TYPE will suffice. Or perhaps we want
> __GENMASK_Uxx() here?

If no objection, I have a preference for GENMASK_TYPE().

>> types, and implement fixed-types versions of GENMASK() based on it.
>> The fixed-type version allows more strict checks to the min/max values
>> accepted, which is useful for defining registers like implemented by
>> i915 and xe drivers with their REG_GENMASK*() macros.
>>
>> The strict checks rely on shift-count-overflow compiler check to fail
>> the build if a number outside of the range allowed is passed.
>> Example:
>>
>> 	#define FOO_MASK GENMASK_U32(33, 4)
>>
>> will generate a warning like:
>>
>> 	../include/linux/bits.h:41:31: error: left shift count >= width of type [-Werror=shift-count-overflow]
>> 	   41 |          (((t)~0ULL - ((t)(1) << (l)) + 1) & \
>> 	      |                               ^~
> 
> ...
> 
>> + * __GENMASK_U*() depends on BITS_PER_TYPE() which would not work in the asm
> 
> Where are the double underscore variants? I see it only for U128.

Same as above. The description is incorrect. I will fix this in v5.

>> + * code as BITS_PER_TYPE() relies on sizeof(), something not available in
>> + * asm. Nethertheless, the concept of fixed width integers is a C thing which
>> + * does not apply to assembly code.

Yours sincerely,
Vincent Mailhol

Re: [PATCH v4 3/8] bits: introduce fixed-type genmasks

[Andy Shevchenko]

On Wed, Mar 05, 2025 at 11:38:19PM +0900, Vincent Mailhol wrote:
> On 05/03/2025 at 23:30, Andy Shevchenko wrote:
> > On Wed, Mar 05, 2025 at 10:00:15PM +0900, Vincent Mailhol via B4 Relay wrote:

...

> > Perhaps T or TYPE will suffice. Or perhaps we want
> > __GENMASK_Uxx() here?
> 
> If no objection, I have a preference for GENMASK_TYPE().

No objection from me :-)

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 3/8] bits: introduce fixed-type genmasks

[Lucas De Marchi]

On Wed, Mar 05, 2025 at 11:38:19PM +0900, Vincent Mailhol wrote:
>On 05/03/2025 at 23:30, Andy Shevchenko wrote:
>> On Wed, Mar 05, 2025 at 10:00:15PM +0900, Vincent Mailhol via B4 Relay wrote:
>>> From: Yury Norov <yury.norov@gmail.com>
>>>
>>> Add __GENMASK_t() which generalizes __GENMASK() to support different
>>
>> Is it with double underscore? I do not see it.
>
>This is my mistake. In an earlier draft, it was __GENMASK_t(),
>meanwhile, I dropped the __ prefix but forget to update the patch
>description.
>
>> _t is used for typedef simple types. It's unfortunate to have it
>> in such a macro.
>
>Ack.
>
>> Perhaps T or TYPE will suffice. Or perhaps we want
>> __GENMASK_Uxx() here?
>
>If no objection, I have a preference for GENMASK_TYPE().

ack, GENMASK_TYPE() seems better.

thanks
Lucas De Marchi

Re: [PATCH v4 3/8] bits: introduce fixed-type genmasks

[Yury Norov]

+ Anshuman Khandual <anshuman.khandual@arm.com>

Anshuman,

I merged your GENMASK_U128() because you said it's important for your
projects, and that it will get used in the kernel soon.

Now it's in the kernel for more than 6 month, but no users were added.
Can you clarify if you still need it, and if so why it's not used?

As you see, people add another fixed-types GENMASK() macros, and their
implementation differ from GENMASK_U128().

My second concern is that __GENMASK_U128() is declared in uapi, while
the general understanding for other fixed-type genmasks is that they
are not exported to users. Do you need this macro to be exported to
userspace? Can you show how and where it is used there?

Thanks,
Yury


On Wed, Mar 05, 2025 at 10:00:15PM +0900, Vincent Mailhol via B4 Relay wrote:
> From: Yury Norov <yury.norov@gmail.com>
> 
> Add __GENMASK_t() which generalizes __GENMASK() to support different
> types, and implement fixed-types versions of GENMASK() based on it.
> The fixed-type version allows more strict checks to the min/max values
> accepted, which is useful for defining registers like implemented by
> i915 and xe drivers with their REG_GENMASK*() macros.
> 
> The strict checks rely on shift-count-overflow compiler check to fail
> the build if a number outside of the range allowed is passed.
> Example:
> 
> 	#define FOO_MASK GENMASK_U32(33, 4)
> 
> will generate a warning like:
> 
> 	../include/linux/bits.h:41:31: error: left shift count >= width of type [-Werror=shift-count-overflow]
> 	   41 |          (((t)~0ULL - ((t)(1) << (l)) + 1) & \
> 	      |                               ^~
> 
> Signed-off-by: Yury Norov <yury.norov@gmail.com>
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> Acked-by: Jani Nikula <jani.nikula@intel.com>
> Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> ---
> Changelog:
> 
>   v3 -> v4:
> 
>     - The v3 is one year old. Meanwhile people started using
>       __GENMASK() directly. So instead of generalizing __GENMASK() to
>       support different types, add a new GENMASK_t().
> 
>     - replace ~0ULL by ~_ULL(0). Otherwise, __GENMASK_t() would fail
>       in asm code.
> 
>     - Make GENMASK_U8() and GENMASK_U16() return an unsigned int. In
>       v3, due to the integer promotion rules, these were returning a
>       signed integer. By casting these to unsigned int, at least the
>       signedness is kept.
> ---
>  include/linux/bitops.h |  1 -
>  include/linux/bits.h   | 33 +++++++++++++++++++++++++++++----
>  2 files changed, 29 insertions(+), 5 deletions(-)
> 
> diff --git a/include/linux/bitops.h b/include/linux/bitops.h
> index c1cb53cf2f0f8662ed3e324578f74330e63f935d..9be2d50da09a417966b3d11c84092bb2f4cd0bef 100644
> --- a/include/linux/bitops.h
> +++ b/include/linux/bitops.h
> @@ -8,7 +8,6 @@
>  
>  #include <uapi/linux/kernel.h>
>  
> -#define BITS_PER_TYPE(type)	(sizeof(type) * BITS_PER_BYTE)
>  #define BITS_TO_LONGS(nr)	__KERNEL_DIV_ROUND_UP(nr, BITS_PER_TYPE(long))
>  #define BITS_TO_U64(nr)		__KERNEL_DIV_ROUND_UP(nr, BITS_PER_TYPE(u64))
>  #define BITS_TO_U32(nr)		__KERNEL_DIV_ROUND_UP(nr, BITS_PER_TYPE(u32))
> diff --git a/include/linux/bits.h b/include/linux/bits.h
> index 5f68980a1b98d771426872c74d7b5c0f79e5e802..f202e46d2f4b7899c16d975120f3fa3ae41556ae 100644
> --- a/include/linux/bits.h
> +++ b/include/linux/bits.h
> @@ -12,6 +12,7 @@
>  #define BIT_ULL_MASK(nr)	(ULL(1) << ((nr) % BITS_PER_LONG_LONG))
>  #define BIT_ULL_WORD(nr)	((nr) / BITS_PER_LONG_LONG)
>  #define BITS_PER_BYTE		8
> +#define BITS_PER_TYPE(type)	(sizeof(type) * BITS_PER_BYTE)
>  
>  /*
>   * Create a contiguous bitmask starting at bit position @l and ending at
> @@ -25,14 +26,38 @@
>  
>  #define GENMASK_INPUT_CHECK(h, l) BUILD_BUG_ON_ZERO(const_true((l) > (h)))
>  
> -#define GENMASK(h, l) \
> -	(GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
> -#define GENMASK_ULL(h, l) \
> -	(GENMASK_INPUT_CHECK(h, l) + __GENMASK_ULL(h, l))
> +/*
> + * Generate a mask for the specified type @t. Additional checks are made to
> + * guarantee the value returned fits in that type, relying on
> + * shift-count-overflow compiler check to detect incompatible arguments.
> + * For example, all these create build errors or warnings:
> + *
> + * - GENMASK(15, 20): wrong argument order
> + * - GENMASK(72, 15): doesn't fit unsigned long
> + * - GENMASK_U32(33, 15): doesn't fit in a u32
> + */
> +#define GENMASK_t(t, h, l)				\
> +	(GENMASK_INPUT_CHECK(h, l) +			\
> +	 (((t)~ULL(0) - ((t)1 << (l)) + 1) &		\
> +	  ((t)~ULL(0) >> (BITS_PER_TYPE(t) - 1 - (h)))))
> +
> +#define GENMASK(h, l) GENMASK_t(unsigned long,  h, l)
> +#define GENMASK_ULL(h, l) GENMASK_t(unsigned long long, h, l)
>  
>  /*
>   * Missing asm support
>   *
> + * __GENMASK_U*() depends on BITS_PER_TYPE() which would not work in the asm
> + * code as BITS_PER_TYPE() relies on sizeof(), something not available in
> + * asm. Nethertheless, the concept of fixed width integers is a C thing which
> + * does not apply to assembly code.
> + */
> +#define GENMASK_U8(h, l) ((unsigned int)GENMASK_t(u8,  h, l))
> +#define GENMASK_U16(h, l) ((unsigned int)GENMASK_t(u16, h, l))
> +#define GENMASK_U32(h, l) GENMASK_t(u32, h, l)
> +#define GENMASK_U64(h, l) GENMASK_t(u64, h, l)
> +
> +/*
>   * __GENMASK_U128() depends on _BIT128() which would not work
>   * in the asm code, as it shifts an 'unsigned __int128' data
>   * type instead of direct representation of 128 bit constants
> 
> -- 
> 2.45.3
> 

Re: [PATCH v4 3/8] bits: introduce fixed-type genmasks

[Andy Shevchenko]

On Wed, Mar 05, 2025 at 10:22:44AM -0500, Yury Norov wrote:
> + Anshuman Khandual <anshuman.khandual@arm.com>
> 
> Anshuman,
> 
> I merged your GENMASK_U128() because you said it's important for your
> projects, and that it will get used in the kernel soon.
> 
> Now it's in the kernel for more than 6 month, but no users were added.
> Can you clarify if you still need it, and if so why it's not used?
> 
> As you see, people add another fixed-types GENMASK() macros, and their
> implementation differ from GENMASK_U128().
> 
> My second concern is that __GENMASK_U128() is declared in uapi, while
> the general understanding for other fixed-type genmasks is that they
> are not exported to users. Do you need this macro to be exported to
> userspace? Can you show how and where it is used there?

FWIW, have you browsed via Debian source code browser? If you can't find it
there, you may remove from uAPI with a little chance of the ABI regression.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 3/8] bits: introduce fixed-type genmasks

[Yury Norov]

+ Catalin Marinas, ARM maillist

Hi Catalin and everyone,

Anshuman Khandual asked me to merge GENMASK_U128() saying it's
important for ARM to stabilize API. While it's a dead code, I
accepted his patch as he promised to add users shortly.

Now it's more than half a year since that. There's no users,
and no feedback from Anshuman.

Can you please tell if you still need the macro? I don't want to
undercut your development, but if you don't need 128-bit genmasks
there's no reason to have a dead code in the uapi.

Thanks,
Yury

On Wed, Mar 05, 2025 at 10:22:47AM -0500, Yury Norov wrote:
> + Anshuman Khandual <anshuman.khandual@arm.com>
> 
> Anshuman,
> 
> I merged your GENMASK_U128() because you said it's important for your
> projects, and that it will get used in the kernel soon.
> 
> Now it's in the kernel for more than 6 month, but no users were added.
> Can you clarify if you still need it, and if so why it's not used?
> 
> As you see, people add another fixed-types GENMASK() macros, and their
> implementation differ from GENMASK_U128().
> 
> My second concern is that __GENMASK_U128() is declared in uapi, while
> the general understanding for other fixed-type genmasks is that they
> are not exported to users. Do you need this macro to be exported to
> userspace? Can you show how and where it is used there?
> 
> Thanks,
> Yury
> 
> 
> On Wed, Mar 05, 2025 at 10:00:15PM +0900, Vincent Mailhol via B4 Relay wrote:
> > From: Yury Norov <yury.norov@gmail.com>
> > 
> > Add __GENMASK_t() which generalizes __GENMASK() to support different
> > types, and implement fixed-types versions of GENMASK() based on it.
> > The fixed-type version allows more strict checks to the min/max values
> > accepted, which is useful for defining registers like implemented by
> > i915 and xe drivers with their REG_GENMASK*() macros.
> > 
> > The strict checks rely on shift-count-overflow compiler check to fail
> > the build if a number outside of the range allowed is passed.
> > Example:
> > 
> > 	#define FOO_MASK GENMASK_U32(33, 4)
> > 
> > will generate a warning like:
> > 
> > 	../include/linux/bits.h:41:31: error: left shift count >= width of type [-Werror=shift-count-overflow]
> > 	   41 |          (((t)~0ULL - ((t)(1) << (l)) + 1) & \
> > 	      |                               ^~
> > 
> > Signed-off-by: Yury Norov <yury.norov@gmail.com>
> > Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> > Acked-by: Jani Nikula <jani.nikula@intel.com>
> > Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> > ---
> > Changelog:
> > 
> >   v3 -> v4:
> > 
> >     - The v3 is one year old. Meanwhile people started using
> >       __GENMASK() directly. So instead of generalizing __GENMASK() to
> >       support different types, add a new GENMASK_t().
> > 
> >     - replace ~0ULL by ~_ULL(0). Otherwise, __GENMASK_t() would fail
> >       in asm code.
> > 
> >     - Make GENMASK_U8() and GENMASK_U16() return an unsigned int. In
> >       v3, due to the integer promotion rules, these were returning a
> >       signed integer. By casting these to unsigned int, at least the
> >       signedness is kept.
> > ---
> >  include/linux/bitops.h |  1 -
> >  include/linux/bits.h   | 33 +++++++++++++++++++++++++++++----
> >  2 files changed, 29 insertions(+), 5 deletions(-)
> > 
> > diff --git a/include/linux/bitops.h b/include/linux/bitops.h
> > index c1cb53cf2f0f8662ed3e324578f74330e63f935d..9be2d50da09a417966b3d11c84092bb2f4cd0bef 100644
> > --- a/include/linux/bitops.h
> > +++ b/include/linux/bitops.h
> > @@ -8,7 +8,6 @@
> >  
> >  #include <uapi/linux/kernel.h>
> >  
> > -#define BITS_PER_TYPE(type)	(sizeof(type) * BITS_PER_BYTE)
> >  #define BITS_TO_LONGS(nr)	__KERNEL_DIV_ROUND_UP(nr, BITS_PER_TYPE(long))
> >  #define BITS_TO_U64(nr)		__KERNEL_DIV_ROUND_UP(nr, BITS_PER_TYPE(u64))
> >  #define BITS_TO_U32(nr)		__KERNEL_DIV_ROUND_UP(nr, BITS_PER_TYPE(u32))
> > diff --git a/include/linux/bits.h b/include/linux/bits.h
> > index 5f68980a1b98d771426872c74d7b5c0f79e5e802..f202e46d2f4b7899c16d975120f3fa3ae41556ae 100644
> > --- a/include/linux/bits.h
> > +++ b/include/linux/bits.h
> > @@ -12,6 +12,7 @@
> >  #define BIT_ULL_MASK(nr)	(ULL(1) << ((nr) % BITS_PER_LONG_LONG))
> >  #define BIT_ULL_WORD(nr)	((nr) / BITS_PER_LONG_LONG)
> >  #define BITS_PER_BYTE		8
> > +#define BITS_PER_TYPE(type)	(sizeof(type) * BITS_PER_BYTE)
> >  
> >  /*
> >   * Create a contiguous bitmask starting at bit position @l and ending at
> > @@ -25,14 +26,38 @@
> >  
> >  #define GENMASK_INPUT_CHECK(h, l) BUILD_BUG_ON_ZERO(const_true((l) > (h)))
> >  
> > -#define GENMASK(h, l) \
> > -	(GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
> > -#define GENMASK_ULL(h, l) \
> > -	(GENMASK_INPUT_CHECK(h, l) + __GENMASK_ULL(h, l))
> > +/*
> > + * Generate a mask for the specified type @t. Additional checks are made to
> > + * guarantee the value returned fits in that type, relying on
> > + * shift-count-overflow compiler check to detect incompatible arguments.
> > + * For example, all these create build errors or warnings:
> > + *
> > + * - GENMASK(15, 20): wrong argument order
> > + * - GENMASK(72, 15): doesn't fit unsigned long
> > + * - GENMASK_U32(33, 15): doesn't fit in a u32
> > + */
> > +#define GENMASK_t(t, h, l)				\
> > +	(GENMASK_INPUT_CHECK(h, l) +			\
> > +	 (((t)~ULL(0) - ((t)1 << (l)) + 1) &		\
> > +	  ((t)~ULL(0) >> (BITS_PER_TYPE(t) - 1 - (h)))))
> > +
> > +#define GENMASK(h, l) GENMASK_t(unsigned long,  h, l)
> > +#define GENMASK_ULL(h, l) GENMASK_t(unsigned long long, h, l)
> >  
> >  /*
> >   * Missing asm support
> >   *
> > + * __GENMASK_U*() depends on BITS_PER_TYPE() which would not work in the asm
> > + * code as BITS_PER_TYPE() relies on sizeof(), something not available in
> > + * asm. Nethertheless, the concept of fixed width integers is a C thing which
> > + * does not apply to assembly code.
> > + */
> > +#define GENMASK_U8(h, l) ((unsigned int)GENMASK_t(u8,  h, l))
> > +#define GENMASK_U16(h, l) ((unsigned int)GENMASK_t(u16, h, l))
> > +#define GENMASK_U32(h, l) GENMASK_t(u32, h, l)
> > +#define GENMASK_U64(h, l) GENMASK_t(u64, h, l)
> > +
> > +/*
> >   * __GENMASK_U128() depends on _BIT128() which would not work
> >   * in the asm code, as it shifts an 'unsigned __int128' data
> >   * type instead of direct representation of 128 bit constants
> > 
> > -- 
> > 2.45.3
> > 

Re: [PATCH v4 3/8] bits: introduce fixed-type genmasks

[Anshuman Khandual]

On 3/19/25 07:16, Yury Norov wrote:
> + Catalin Marinas, ARM maillist
> 
> Hi Catalin and everyone,

Hello Yury,

> 
> Anshuman Khandual asked me to merge GENMASK_U128() saying it's
> important for ARM to stabilize API. While it's a dead code, I
> accepted his patch as he promised to add users shortly.
> 
> Now it's more than half a year since that. There's no users,
> and no feedback from Anshuman.

My apologies to have missed your email earlier. Please find response
for the earlier email below as well.

> 
> Can you please tell if you still need the macro? I don't want to
> undercut your development, but if you don't need 128-bit genmasks
> there's no reason to have a dead code in the uapi.

The code base specifically using GENMASK_U128() has not been posted
upstream (probably in next couple of months or so) till now, except
the following patch which has been not been merged and still under
review and development.

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

> 
> Thanks,
> Yury
> 
> On Wed, Mar 05, 2025 at 10:22:47AM -0500, Yury Norov wrote:
>> + Anshuman Khandual <anshuman.khandual@arm.com>
>>
>> Anshuman,
>>
>> I merged your GENMASK_U128() because you said it's important for your
>> projects, and that it will get used in the kernel soon.
>>
>> Now it's in the kernel for more than 6 month, but no users were added.
>> Can you clarify if you still need it, and if so why it's not used?

We would need it but although the code using GENMASK_U128() has not been
posted upstream.

>>
>> As you see, people add another fixed-types GENMASK() macros, and their
>> implementation differ from GENMASK_U128().

I will take a look. Is GENMASK_U128() being problematic for the this new
scheme ?

>>
>> My second concern is that __GENMASK_U128() is declared in uapi, while
>> the general understanding for other fixed-type genmasks is that they
>> are not exported to users. Do you need this macro to be exported to
>> userspace? Can you show how and where it is used there?

No, not atleast right now.

These were moved into uapi subsequently via the following commit.

21a3a3d015aee ("tools headers: Synchronize {uapi/}linux/bits.h with the kernel sources")

But in general GENMASK_U128() is needed for generating 128 bit page table
entries, related flags and masks whether in kernel or in user space for
writing kernel test cases etc.

>>
>> Thanks,
>> Yury
>>
>>
>> On Wed, Mar 05, 2025 at 10:00:15PM +0900, Vincent Mailhol via B4 Relay wrote:
>>> From: Yury Norov <yury.norov@gmail.com>
>>>
>>> Add __GENMASK_t() which generalizes __GENMASK() to support different
>>> types, and implement fixed-types versions of GENMASK() based on it.
>>> The fixed-type version allows more strict checks to the min/max values
>>> accepted, which is useful for defining registers like implemented by
>>> i915 and xe drivers with their REG_GENMASK*() macros.
>>>
>>> The strict checks rely on shift-count-overflow compiler check to fail
>>> the build if a number outside of the range allowed is passed.
>>> Example:
>>>
>>> 	#define FOO_MASK GENMASK_U32(33, 4)
>>>
>>> will generate a warning like:
>>>
>>> 	../include/linux/bits.h:41:31: error: left shift count >= width of type [-Werror=shift-count-overflow]
>>> 	   41 |          (((t)~0ULL - ((t)(1) << (l)) + 1) & \
>>> 	      |                               ^~
>>>
>>> Signed-off-by: Yury Norov <yury.norov@gmail.com>
>>> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
>>> Acked-by: Jani Nikula <jani.nikula@intel.com>
>>> Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
>>> ---
>>> Changelog:
>>>
>>>   v3 -> v4:
>>>
>>>     - The v3 is one year old. Meanwhile people started using
>>>       __GENMASK() directly. So instead of generalizing __GENMASK() to
>>>       support different types, add a new GENMASK_t().
>>>
>>>     - replace ~0ULL by ~_ULL(0). Otherwise, __GENMASK_t() would fail
>>>       in asm code.
>>>
>>>     - Make GENMASK_U8() and GENMASK_U16() return an unsigned int. In
>>>       v3, due to the integer promotion rules, these were returning a
>>>       signed integer. By casting these to unsigned int, at least the
>>>       signedness is kept.
>>> ---
>>>  include/linux/bitops.h |  1 -
>>>  include/linux/bits.h   | 33 +++++++++++++++++++++++++++++----
>>>  2 files changed, 29 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/include/linux/bitops.h b/include/linux/bitops.h
>>> index c1cb53cf2f0f8662ed3e324578f74330e63f935d..9be2d50da09a417966b3d11c84092bb2f4cd0bef 100644
>>> --- a/include/linux/bitops.h
>>> +++ b/include/linux/bitops.h
>>> @@ -8,7 +8,6 @@
>>>  
>>>  #include <uapi/linux/kernel.h>
>>>  
>>> -#define BITS_PER_TYPE(type)	(sizeof(type) * BITS_PER_BYTE)
>>>  #define BITS_TO_LONGS(nr)	__KERNEL_DIV_ROUND_UP(nr, BITS_PER_TYPE(long))
>>>  #define BITS_TO_U64(nr)		__KERNEL_DIV_ROUND_UP(nr, BITS_PER_TYPE(u64))
>>>  #define BITS_TO_U32(nr)		__KERNEL_DIV_ROUND_UP(nr, BITS_PER_TYPE(u32))
>>> diff --git a/include/linux/bits.h b/include/linux/bits.h
>>> index 5f68980a1b98d771426872c74d7b5c0f79e5e802..f202e46d2f4b7899c16d975120f3fa3ae41556ae 100644
>>> --- a/include/linux/bits.h
>>> +++ b/include/linux/bits.h
>>> @@ -12,6 +12,7 @@
>>>  #define BIT_ULL_MASK(nr)	(ULL(1) << ((nr) % BITS_PER_LONG_LONG))
>>>  #define BIT_ULL_WORD(nr)	((nr) / BITS_PER_LONG_LONG)
>>>  #define BITS_PER_BYTE		8
>>> +#define BITS_PER_TYPE(type)	(sizeof(type) * BITS_PER_BYTE)
>>>  
>>>  /*
>>>   * Create a contiguous bitmask starting at bit position @l and ending at
>>> @@ -25,14 +26,38 @@
>>>  
>>>  #define GENMASK_INPUT_CHECK(h, l) BUILD_BUG_ON_ZERO(const_true((l) > (h)))
>>>  
>>> -#define GENMASK(h, l) \
>>> -	(GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
>>> -#define GENMASK_ULL(h, l) \
>>> -	(GENMASK_INPUT_CHECK(h, l) + __GENMASK_ULL(h, l))
>>> +/*
>>> + * Generate a mask for the specified type @t. Additional checks are made to
>>> + * guarantee the value returned fits in that type, relying on
>>> + * shift-count-overflow compiler check to detect incompatible arguments.
>>> + * For example, all these create build errors or warnings:
>>> + *
>>> + * - GENMASK(15, 20): wrong argument order
>>> + * - GENMASK(72, 15): doesn't fit unsigned long
>>> + * - GENMASK_U32(33, 15): doesn't fit in a u32
>>> + */
>>> +#define GENMASK_t(t, h, l)				\
>>> +	(GENMASK_INPUT_CHECK(h, l) +			\
>>> +	 (((t)~ULL(0) - ((t)1 << (l)) + 1) &		\
>>> +	  ((t)~ULL(0) >> (BITS_PER_TYPE(t) - 1 - (h)))))
>>> +
>>> +#define GENMASK(h, l) GENMASK_t(unsigned long,  h, l)
>>> +#define GENMASK_ULL(h, l) GENMASK_t(unsigned long long, h, l)
>>>  
>>>  /*
>>>   * Missing asm support
>>>   *
>>> + * __GENMASK_U*() depends on BITS_PER_TYPE() which would not work in the asm
>>> + * code as BITS_PER_TYPE() relies on sizeof(), something not available in
>>> + * asm. Nethertheless, the concept of fixed width integers is a C thing which
>>> + * does not apply to assembly code.
>>> + */
>>> +#define GENMASK_U8(h, l) ((unsigned int)GENMASK_t(u8,  h, l))
>>> +#define GENMASK_U16(h, l) ((unsigned int)GENMASK_t(u16, h, l))
>>> +#define GENMASK_U32(h, l) GENMASK_t(u32, h, l)
>>> +#define GENMASK_U64(h, l) GENMASK_t(u64, h, l)
>>> +
>>> +/*
>>>   * __GENMASK_U128() depends on _BIT128() which would not work
>>>   * in the asm code, as it shifts an 'unsigned __int128' data
>>>   * type instead of direct representation of 128 bit constants
>>>
>>> -- 
>>> 2.45.3
>>>

Re: [PATCH v4 3/8] bits: introduce fixed-type genmasks

[Anshuman Khandual]

On 3/19/25 09:04, Anshuman Khandual wrote:
> On 3/19/25 07:16, Yury Norov wrote:
>> + Catalin Marinas, ARM maillist
>>
>> Hi Catalin and everyone,
> 
> Hello Yury,
> 
>>
>> Anshuman Khandual asked me to merge GENMASK_U128() saying it's
>> important for ARM to stabilize API. While it's a dead code, I
>> accepted his patch as he promised to add users shortly.
>>
>> Now it's more than half a year since that. There's no users,
>> and no feedback from Anshuman.
> 
> My apologies to have missed your email earlier. Please find response
> for the earlier email below as well.
> 
>>
>> Can you please tell if you still need the macro? I don't want to
>> undercut your development, but if you don't need 128-bit genmasks
>> there's no reason to have a dead code in the uapi.
> 
> The code base specifically using GENMASK_U128() has not been posted
> upstream (probably in next couple of months or so) till now, except
> the following patch which has been not been merged and still under
> review and development.
> 
> https://lore.kernel.org/lkml/20240801054436.612024-1-anshuman.khandual@arm.com/
> 
>>
>> Thanks,
>> Yury
>>
>> On Wed, Mar 05, 2025 at 10:22:47AM -0500, Yury Norov wrote:
>>> + Anshuman Khandual <anshuman.khandual@arm.com>
>>>
>>> Anshuman,
>>>
>>> I merged your GENMASK_U128() because you said it's important for your
>>> projects, and that it will get used in the kernel soon.
>>>
>>> Now it's in the kernel for more than 6 month, but no users were added.
>>> Can you clarify if you still need it, and if so why it's not used?
> 
> We would need it but although the code using GENMASK_U128() has not been
> posted upstream.
> 
>>>
>>> As you see, people add another fixed-types GENMASK() macros, and their
>>> implementation differ from GENMASK_U128().
> 
> I will take a look. Is GENMASK_U128() being problematic for the this new
> scheme ?
> 
>>>
>>> My second concern is that __GENMASK_U128() is declared in uapi, while
>>> the general understanding for other fixed-type genmasks is that they
>>> are not exported to users. Do you need this macro to be exported to
>>> userspace? Can you show how and where it is used there?
> 
> No, not atleast right now.
> 
> These were moved into uapi subsequently via the following commit.
> 
> 21a3a3d015aee ("tools headers: Synchronize {uapi/}linux/bits.h with the kernel sources")
> 
> But in general GENMASK_U128() is needed for generating 128 bit page table
> entries, related flags and masks whether in kernel or in user space for
> writing kernel test cases etc.

In the commit 947697c6f0f7 ("uapi: Define GENMASK_U128"), GENMASK_U128() gets defined
using __GENMASK_U128() which in turn calls __BIT128() - both of which are defined in
UAPI headers inside (include/uapi/linux/). 

Just wondering - are you suggesting to move these helpers from include/uapi/linux/ to
include/linux/bits.h instead ?

> 
>>>
>>> Thanks,
>>> Yury
>>>
>>>
>>> On Wed, Mar 05, 2025 at 10:00:15PM +0900, Vincent Mailhol via B4 Relay wrote:
>>>> From: Yury Norov <yury.norov@gmail.com>
>>>>
>>>> Add __GENMASK_t() which generalizes __GENMASK() to support different
>>>> types, and implement fixed-types versions of GENMASK() based on it.
>>>> The fixed-type version allows more strict checks to the min/max values
>>>> accepted, which is useful for defining registers like implemented by
>>>> i915 and xe drivers with their REG_GENMASK*() macros.
>>>>
>>>> The strict checks rely on shift-count-overflow compiler check to fail
>>>> the build if a number outside of the range allowed is passed.
>>>> Example:
>>>>
>>>> 	#define FOO_MASK GENMASK_U32(33, 4)
>>>>
>>>> will generate a warning like:
>>>>
>>>> 	../include/linux/bits.h:41:31: error: left shift count >= width of type [-Werror=shift-count-overflow]
>>>> 	   41 |          (((t)~0ULL - ((t)(1) << (l)) + 1) & \
>>>> 	      |                               ^~
>>>>
>>>> Signed-off-by: Yury Norov <yury.norov@gmail.com>
>>>> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
>>>> Acked-by: Jani Nikula <jani.nikula@intel.com>
>>>> Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
>>>> ---
>>>> Changelog:
>>>>
>>>>   v3 -> v4:
>>>>
>>>>     - The v3 is one year old. Meanwhile people started using
>>>>       __GENMASK() directly. So instead of generalizing __GENMASK() to
>>>>       support different types, add a new GENMASK_t().
>>>>
>>>>     - replace ~0ULL by ~_ULL(0). Otherwise, __GENMASK_t() would fail
>>>>       in asm code.
>>>>
>>>>     - Make GENMASK_U8() and GENMASK_U16() return an unsigned int. In
>>>>       v3, due to the integer promotion rules, these were returning a
>>>>       signed integer. By casting these to unsigned int, at least the
>>>>       signedness is kept.
>>>> ---
>>>>  include/linux/bitops.h |  1 -
>>>>  include/linux/bits.h   | 33 +++++++++++++++++++++++++++++----
>>>>  2 files changed, 29 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/include/linux/bitops.h b/include/linux/bitops.h
>>>> index c1cb53cf2f0f8662ed3e324578f74330e63f935d..9be2d50da09a417966b3d11c84092bb2f4cd0bef 100644
>>>> --- a/include/linux/bitops.h
>>>> +++ b/include/linux/bitops.h
>>>> @@ -8,7 +8,6 @@
>>>>  
>>>>  #include <uapi/linux/kernel.h>
>>>>  
>>>> -#define BITS_PER_TYPE(type)	(sizeof(type) * BITS_PER_BYTE)
>>>>  #define BITS_TO_LONGS(nr)	__KERNEL_DIV_ROUND_UP(nr, BITS_PER_TYPE(long))
>>>>  #define BITS_TO_U64(nr)		__KERNEL_DIV_ROUND_UP(nr, BITS_PER_TYPE(u64))
>>>>  #define BITS_TO_U32(nr)		__KERNEL_DIV_ROUND_UP(nr, BITS_PER_TYPE(u32))
>>>> diff --git a/include/linux/bits.h b/include/linux/bits.h
>>>> index 5f68980a1b98d771426872c74d7b5c0f79e5e802..f202e46d2f4b7899c16d975120f3fa3ae41556ae 100644
>>>> --- a/include/linux/bits.h
>>>> +++ b/include/linux/bits.h
>>>> @@ -12,6 +12,7 @@
>>>>  #define BIT_ULL_MASK(nr)	(ULL(1) << ((nr) % BITS_PER_LONG_LONG))
>>>>  #define BIT_ULL_WORD(nr)	((nr) / BITS_PER_LONG_LONG)
>>>>  #define BITS_PER_BYTE		8
>>>> +#define BITS_PER_TYPE(type)	(sizeof(type) * BITS_PER_BYTE)
>>>>  
>>>>  /*
>>>>   * Create a contiguous bitmask starting at bit position @l and ending at
>>>> @@ -25,14 +26,38 @@
>>>>  
>>>>  #define GENMASK_INPUT_CHECK(h, l) BUILD_BUG_ON_ZERO(const_true((l) > (h)))
>>>>  
>>>> -#define GENMASK(h, l) \
>>>> -	(GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
>>>> -#define GENMASK_ULL(h, l) \
>>>> -	(GENMASK_INPUT_CHECK(h, l) + __GENMASK_ULL(h, l))
>>>> +/*
>>>> + * Generate a mask for the specified type @t. Additional checks are made to
>>>> + * guarantee the value returned fits in that type, relying on
>>>> + * shift-count-overflow compiler check to detect incompatible arguments.
>>>> + * For example, all these create build errors or warnings:
>>>> + *
>>>> + * - GENMASK(15, 20): wrong argument order
>>>> + * - GENMASK(72, 15): doesn't fit unsigned long
>>>> + * - GENMASK_U32(33, 15): doesn't fit in a u32
>>>> + */
>>>> +#define GENMASK_t(t, h, l)				\
>>>> +	(GENMASK_INPUT_CHECK(h, l) +			\
>>>> +	 (((t)~ULL(0) - ((t)1 << (l)) + 1) &		\
>>>> +	  ((t)~ULL(0) >> (BITS_PER_TYPE(t) - 1 - (h)))))
>>>> +
>>>> +#define GENMASK(h, l) GENMASK_t(unsigned long,  h, l)
>>>> +#define GENMASK_ULL(h, l) GENMASK_t(unsigned long long, h, l)
>>>>  
>>>>  /*
>>>>   * Missing asm support
>>>>   *
>>>> + * __GENMASK_U*() depends on BITS_PER_TYPE() which would not work in the asm
>>>> + * code as BITS_PER_TYPE() relies on sizeof(), something not available in
>>>> + * asm. Nethertheless, the concept of fixed width integers is a C thing which
>>>> + * does not apply to assembly code.
>>>> + */
>>>> +#define GENMASK_U8(h, l) ((unsigned int)GENMASK_t(u8,  h, l))
>>>> +#define GENMASK_U16(h, l) ((unsigned int)GENMASK_t(u16, h, l))
>>>> +#define GENMASK_U32(h, l) GENMASK_t(u32, h, l)
>>>> +#define GENMASK_U64(h, l) GENMASK_t(u64, h, l)
>>>> +
>>>> +/*
>>>>   * __GENMASK_U128() depends on _BIT128() which would not work
>>>>   * in the asm code, as it shifts an 'unsigned __int128' data
>>>>   * type instead of direct representation of 128 bit constants
>>>>
>>>> -- 
>>>> 2.45.3
>>>>
> 

Re: [PATCH v4 3/8] bits: introduce fixed-type genmasks

[Yury Norov]

On Wed, Mar 19, 2025 at 09:43:06AM +0530, Anshuman Khandual wrote:
> 
> 
> On 3/19/25 09:04, Anshuman Khandual wrote:
> > On 3/19/25 07:16, Yury Norov wrote:
> >> + Catalin Marinas, ARM maillist
> >>
> >> Hi Catalin and everyone,
> > 
> > Hello Yury,
> > 
> >>
> >> Anshuman Khandual asked me to merge GENMASK_U128() saying it's
> >> important for ARM to stabilize API. While it's a dead code, I
> >> accepted his patch as he promised to add users shortly.
> >>
> >> Now it's more than half a year since that. There's no users,
> >> and no feedback from Anshuman.
> > 
> > My apologies to have missed your email earlier. Please find response
> > for the earlier email below as well.
> > 
> >>
> >> Can you please tell if you still need the macro? I don't want to
> >> undercut your development, but if you don't need 128-bit genmasks
> >> there's no reason to have a dead code in the uapi.
> > 
> > The code base specifically using GENMASK_U128() has not been posted
> > upstream (probably in next couple of months or so) till now, except
> > the following patch which has been not been merged and still under
> > review and development.
> > 
> > https://lore.kernel.org/lkml/20240801054436.612024-1-anshuman.khandual@arm.com/
> > 
> >>
> >> Thanks,
> >> Yury
> >>
> >> On Wed, Mar 05, 2025 at 10:22:47AM -0500, Yury Norov wrote:
> >>> + Anshuman Khandual <anshuman.khandual@arm.com>
> >>>
> >>> Anshuman,
> >>>
> >>> I merged your GENMASK_U128() because you said it's important for your
> >>> projects, and that it will get used in the kernel soon.
> >>>
> >>> Now it's in the kernel for more than 6 month, but no users were added.
> >>> Can you clarify if you still need it, and if so why it's not used?
> > 
> > We would need it but although the code using GENMASK_U128() has not been
> > posted upstream.
> > 
> >>>
> >>> As you see, people add another fixed-types GENMASK() macros, and their
> >>> implementation differ from GENMASK_U128().
> > 
> > I will take a look. Is GENMASK_U128() being problematic for the this new
> > scheme ?
> > 
> >>>
> >>> My second concern is that __GENMASK_U128() is declared in uapi, while
> >>> the general understanding for other fixed-type genmasks is that they
> >>> are not exported to users. Do you need this macro to be exported to
> >>> userspace? Can you show how and where it is used there?
> > 
> > No, not atleast right now.

Ok, thanks.

> > These were moved into uapi subsequently via the following commit.
> > 
> > 21a3a3d015aee ("tools headers: Synchronize {uapi/}linux/bits.h with the kernel sources")
> > 
> > But in general GENMASK_U128() is needed for generating 128 bit page table
> > entries, related flags and masks whether in kernel or in user space for
> > writing kernel test cases etc.
> 
> In the commit 947697c6f0f7 ("uapi: Define GENMASK_U128"), GENMASK_U128() gets defined
> using __GENMASK_U128() which in turn calls __BIT128() - both of which are defined in
> UAPI headers inside (include/uapi/linux/). 
> 
> Just wondering - are you suggesting to move these helpers from include/uapi/linux/ to
> include/linux/bits.h instead ?

Vincent is working on fixed-width GENMASK_Uxx() based on GENMASK_TYPE().

https://lore.kernel.org/lkml/20250308-fixed-type-genmasks-v6-0-f59315e73c29@wanadoo.fr/T/

The series adds a general GENMASK_TYPE() in the linux/bits.h. I'd like
all fixed-widh genmasks to be based on it. The implementation doesn't
allow to move GENMASK_TYPE() the to uapi easily.

There was a discussion regarding that, and for now the general understanding
is that userspace doesn't need GENMASK_Uxx().

Are your proposed tests based on the in-kernel tools/ ? If so, linux/bits.h
will be available for you.

Vincent,

Can you please experiment with moving GENMASK_U128() to linux/bits.h
and switching it to GENMASK_TYPE()-based implementation?

If it works, we can do it after merging of GENMASK_TYPE() and
ancestors.

Thanks,
Yury

Re: [PATCH v4 3/8] bits: introduce fixed-type genmasks

[Vincent Mailhol]

On 22/03/2025 at 02:05, Yury Norov wrote:
> On Wed, Mar 19, 2025 at 09:43:06AM +0530, Anshuman Khandual wrote:
>> 
>> 
>> On 3/19/25 09:04, Anshuman Khandual wrote:
>>> On 3/19/25 07:16, Yury Norov wrote:
>>>> + Catalin Marinas, ARM maillist

(...)

>>> These were moved into uapi subsequently via the following 
>>> commit.
>>> 
>>> 21a3a3d015aee ("tools headers: Synchronize {uapi/}linux/bits.h 
>>> with the kernel sources")
>>> 
>>> But in general GENMASK_U128() is needed for generating 128 bit 
>>> page table entries, related flags and masks whether in kernel or
>>> in user space for writing kernel test cases etc.
>> 
>> In the commit 947697c6f0f7 ("uapi: Define GENMASK_U128"), 
>> GENMASK_U128() gets defined using __GENMASK_U128() which in turn 
>> calls __BIT128() - both of which are defined in UAPI headers 
>> inside (include/uapi/linux/).
>> 
>> Just wondering - are you suggesting to move these helpers from 
>> include/uapi/linux/ to include/linux/bits.h instead ?
> 
> Vincent is working on fixed-width GENMASK_Uxx() based on 
> GENMASK_TYPE().
> 
> https://lore.kernel.org/lkml/20250308-fixed-type-genmasks-v6-0- 
> f59315e73c29@wanadoo.fr/T/
> 
> The series adds a general GENMASK_TYPE() in the linux/bits.h. I'd 
> like all fixed-widh genmasks to be based on it. The implementation 
> doesn't allow to move GENMASK_TYPE() the to uapi easily.
> 
> There was a discussion regarding that, and for now the general 
> understanding is that userspace doesn't need GENMASK_Uxx().
> 
> Are your proposed tests based on the in-kernel tools/ ? If so, 
> linux/ bits.h will be available for you.
> 
> Vincent,
> 
> Can you please experiment with moving GENMASK_U128() to linux/ 
> bits.h and switching it to GENMASK_TYPE()-based implementation?
> 
> If it works, we can do it after merging of GENMASK_TYPE() and 
> ancestors.

I sent the new version with the split as you asked in a separate message.

I switched GENMASK_U128() from using __GENMASK_U128() to using
GENMASK_TYPE() in this patch of the second series:

https://lore.kernel.org/all/20250322-consolidate-genmask-
v1-2-54bfd36c5643@wanadoo.fr/

After this, the genmask_u128_test() unit tests from lib/test_bits.c are
all green, so this looks good. Note that because it is not yet used,
there isn't much more things to test aside from that unit test.

To be precise, I am not yet *moving* it. For now, I decoupled
GENMASK_U128() from __GENMASK_U128(). To complete the move, all what is
left is to remove __GENMASK_U128() from the uapi. To be honest, I am not
keen on touching either of the uapi or the asm variants myself. But, if
my work gets merged, that last step should be easy for you.



On a side note, at first glance, I was disturbed by the current
__GENMASK_U128() implementation:

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

If calling __GENMASK_U128(127, x), the macro does a:

  _BIT128(127) << 1

which expands to:

  (unsigned __int128)1 << 127 << 1

So, while (unsigned __int128)1 << 128 is an undefined behaviour, doing
it in two steps: << 127 and << 1 is well defined and gives zero. Then,
when doing the subtraction, the unsigned integer wraparound restores the
most significant bits making things go back to normal.


The same applies to all the other variants. If doing:

  #define GENMASK_TYPE(t, h, l)				\
  	((t)(GENMASK_INPUT_CHECK(h, l) +		\
  	     (((t)1 << (h) << 1) - ((t)1 << (l)))))

The unit tests pass for everything and you even still get the warning if
h is out of bound.

But then, bloat-o-meter (x86_64, defconfig, GCC 12.4.1) shows a small
increase:

  Total: Before=22723482, After=22724586, chg +0.00%

So, probably not worth the change anyway. I am keeping the current version.


Yours sincerely,
Vincent Mailhol

Re: [PATCH v4 3/8] bits: introduce fixed-type genmasks

[Yury Norov]

On Wed, Mar 05, 2025 at 10:00:15PM +0900, Vincent Mailhol via B4 Relay wrote:
> From: Yury Norov <yury.norov@gmail.com>
> 
> Add __GENMASK_t() which generalizes __GENMASK() to support different
> types, and implement fixed-types versions of GENMASK() based on it.
> The fixed-type version allows more strict checks to the min/max values
> accepted, which is useful for defining registers like implemented by
> i915 and xe drivers with their REG_GENMASK*() macros.
> 
> The strict checks rely on shift-count-overflow compiler check to fail
> the build if a number outside of the range allowed is passed.
> Example:
> 
> 	#define FOO_MASK GENMASK_U32(33, 4)
> 
> will generate a warning like:
> 
> 	../include/linux/bits.h:41:31: error: left shift count >= width of type [-Werror=shift-count-overflow]
> 	   41 |          (((t)~0ULL - ((t)(1) << (l)) + 1) & \
> 	      |                               ^~
> 
> Signed-off-by: Yury Norov <yury.norov@gmail.com>
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> Acked-by: Jani Nikula <jani.nikula@intel.com>
> Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>

Co-developed-by?

> ---
> Changelog:
> 
>   v3 -> v4:
> 
>     - The v3 is one year old. Meanwhile people started using
>       __GENMASK() directly. So instead of generalizing __GENMASK() to
>       support different types, add a new GENMASK_t().
> 
>     - replace ~0ULL by ~_ULL(0). Otherwise, __GENMASK_t() would fail
>       in asm code.
> 
>     - Make GENMASK_U8() and GENMASK_U16() return an unsigned int. In
>       v3, due to the integer promotion rules, these were returning a
>       signed integer. By casting these to unsigned int, at least the

This comment will disappear when I'll apply the patch. Can you comment
it in the code instead?

>       signedness is kept.
> ---
>  include/linux/bitops.h |  1 -
>  include/linux/bits.h   | 33 +++++++++++++++++++++++++++++----
>  2 files changed, 29 insertions(+), 5 deletions(-)
> 
> diff --git a/include/linux/bitops.h b/include/linux/bitops.h
> index c1cb53cf2f0f8662ed3e324578f74330e63f935d..9be2d50da09a417966b3d11c84092bb2f4cd0bef 100644
> --- a/include/linux/bitops.h
> +++ b/include/linux/bitops.h
> @@ -8,7 +8,6 @@
>  
>  #include <uapi/linux/kernel.h>
>  
> -#define BITS_PER_TYPE(type)	(sizeof(type) * BITS_PER_BYTE)
>  #define BITS_TO_LONGS(nr)	__KERNEL_DIV_ROUND_UP(nr, BITS_PER_TYPE(long))
>  #define BITS_TO_U64(nr)		__KERNEL_DIV_ROUND_UP(nr, BITS_PER_TYPE(u64))
>  #define BITS_TO_U32(nr)		__KERNEL_DIV_ROUND_UP(nr, BITS_PER_TYPE(u32))
> diff --git a/include/linux/bits.h b/include/linux/bits.h
> index 5f68980a1b98d771426872c74d7b5c0f79e5e802..f202e46d2f4b7899c16d975120f3fa3ae41556ae 100644
> --- a/include/linux/bits.h
> +++ b/include/linux/bits.h
> @@ -12,6 +12,7 @@
>  #define BIT_ULL_MASK(nr)	(ULL(1) << ((nr) % BITS_PER_LONG_LONG))
>  #define BIT_ULL_WORD(nr)	((nr) / BITS_PER_LONG_LONG)
>  #define BITS_PER_BYTE		8
> +#define BITS_PER_TYPE(type)	(sizeof(type) * BITS_PER_BYTE)
>  
>  /*
>   * Create a contiguous bitmask starting at bit position @l and ending at
> @@ -25,14 +26,38 @@
>  
>  #define GENMASK_INPUT_CHECK(h, l) BUILD_BUG_ON_ZERO(const_true((l) > (h)))
>  
> -#define GENMASK(h, l) \
> -	(GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
> -#define GENMASK_ULL(h, l) \
> -	(GENMASK_INPUT_CHECK(h, l) + __GENMASK_ULL(h, l))
> +/*
> + * Generate a mask for the specified type @t. Additional checks are made to
> + * guarantee the value returned fits in that type, relying on
> + * shift-count-overflow compiler check to detect incompatible arguments.
> + * For example, all these create build errors or warnings:
> + *
> + * - GENMASK(15, 20): wrong argument order
> + * - GENMASK(72, 15): doesn't fit unsigned long
> + * - GENMASK_U32(33, 15): doesn't fit in a u32
> + */
> +#define GENMASK_t(t, h, l)				\

Agree with Andy. This should be GENMASK_TYPE, or triple-underscored
___GENMASK() maybe. This _t thing looks misleading.

> +	(GENMASK_INPUT_CHECK(h, l) +			\
> +	 (((t)~ULL(0) - ((t)1 << (l)) + 1) &		\
> +	  ((t)~ULL(0) >> (BITS_PER_TYPE(t) - 1 - (h)))))

Can you rebase it on top of -next? In this dev cycle I merge a patch
that reverts the __GENMASK() back to:

#define __GENMASK(h, l) (((~_UL(0)) << (l)) & (~_UL(0) >> (BITS_PER_LONG - 1 - (h))))

> +#define GENMASK(h, l) GENMASK_t(unsigned long,  h, l)
> +#define GENMASK_ULL(h, l) GENMASK_t(unsigned long long, h, l)

This makes __GENMASK() and __GENMASK_ULL() unused in the kernel, other
than in uapi. Or I misunderstand it?

Having, in fact, different implementations of the same macro for kernel
and userspace is a source of problems. Can we move GENMASK_TYPE() to uapi,
and implement __GENMASK() on top of them? If not, I'd prefer to keep
GENMASK and GENMASK_ULL untouched.

Can you run bloat-o-meter and ensure there's no unwanted effects on
code generation?

>  /*
>   * Missing asm support
>   *
> + * __GENMASK_U*() depends on BITS_PER_TYPE() which would not work in the asm

And there's no __GENMASK_U*(), right?

> + * code as BITS_PER_TYPE() relies on sizeof(), something not available in
> + * asm. Nethertheless, the concept of fixed width integers is a C thing which
> + * does not apply to assembly code.
> + */
> +#define GENMASK_U8(h, l) ((unsigned int)GENMASK_t(u8,  h, l))
> +#define GENMASK_U16(h, l) ((unsigned int)GENMASK_t(u16, h, l))

Typecast to the type that user provides explicitly?  And maybe do
in GENMASK_TYPE()

> +#define GENMASK_U32(h, l) GENMASK_t(u32, h, l)
> +#define GENMASK_U64(h, l) GENMASK_t(u64, h, l)

OK, this looks good. But GENMASK_U128() becomes a special case now.
The 128-bit GENMASK is unsued, but it's exported in uapi. Is there any
simple way to end up with a common implementation for all fixed-type
GENMASKs?

> +
> +/*
>   * __GENMASK_U128() depends on _BIT128() which would not work
>   * in the asm code, as it shifts an 'unsigned __int128' data
>   * type instead of direct representation of 128 bit constants

This comment is duplicated by the previous one. Maybe just join them?
(Let's wait for a while for updates regarding GENMASK_U128 status before
doing it.)

Re: [PATCH v4 3/8] bits: introduce fixed-type genmasks

[Jani Nikula]

On Wed, 05 Mar 2025, Yury Norov <yury.norov@gmail.com> wrote:
> On Wed, Mar 05, 2025 at 10:00:15PM +0900, Vincent Mailhol via B4 Relay wrote:
>> +#define GENMASK_U8(h, l) ((unsigned int)GENMASK_t(u8,  h, l))
>> +#define GENMASK_U16(h, l) ((unsigned int)GENMASK_t(u16, h, l))
>
> Typecast to the type that user provides explicitly?  And maybe do
> in GENMASK_TYPE()

The cast to unsigned int seemed odd to me too.

BR,
Jani.


-- 
Jani Nikula, Intel

Re: [PATCH v4 3/8] bits: introduce fixed-type genmasks

[Vincent Mailhol]

On 06/03/2025 at 00:47, Yury Norov wrote:
> On Wed, Mar 05, 2025 at 10:00:15PM +0900, Vincent Mailhol via B4 Relay wrote:
>> From: Yury Norov <yury.norov@gmail.com>
>>
>> Add __GENMASK_t() which generalizes __GENMASK() to support different
>> types, and implement fixed-types versions of GENMASK() based on it.
>> The fixed-type version allows more strict checks to the min/max values
>> accepted, which is useful for defining registers like implemented by
>> i915 and xe drivers with their REG_GENMASK*() macros.
>>
>> The strict checks rely on shift-count-overflow compiler check to fail
>> the build if a number outside of the range allowed is passed.
>> Example:
>>
>> 	#define FOO_MASK GENMASK_U32(33, 4)
>>
>> will generate a warning like:
>>
>> 	../include/linux/bits.h:41:31: error: left shift count >= width of type [-Werror=shift-count-overflow]
>> 	   41 |          (((t)~0ULL - ((t)(1) << (l)) + 1) & \
>> 	      |                               ^~
>>
>> Signed-off-by: Yury Norov <yury.norov@gmail.com>
>> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
>> Acked-by: Jani Nikula <jani.nikula@intel.com>
>> Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> 
> Co-developed-by?

OK. I will keep you as the main author and credit me as Co-developer.

>> ---
>> Changelog:
>>
>>   v3 -> v4:
>>
>>     - The v3 is one year old. Meanwhile people started using
>>       __GENMASK() directly. So instead of generalizing __GENMASK() to
>>       support different types, add a new GENMASK_t().
>>
>>     - replace ~0ULL by ~_ULL(0). Otherwise, __GENMASK_t() would fail
>>       in asm code.
>>
>>     - Make GENMASK_U8() and GENMASK_U16() return an unsigned int. In
>>       v3, due to the integer promotion rules, these were returning a
>>       signed integer. By casting these to unsigned int, at least the
> 
> This comment will disappear when I'll apply the patch. Can you comment
> it in the code instead?

Ack. I will add below comment in the code:

  /*
   * Because of the C integer promotion rules, the U8 and the U16
   * variants would immediately become signed integers when used in
   * expressions. Cast them to unsigned int so that, at least, the
   * signedness is preserved.
   */

(unless if you prefer to go back to the u8 and u16 casts, c.f. below).

>>       signedness is kept.
>> ---
>>  include/linux/bitops.h |  1 -
>>  include/linux/bits.h   | 33 +++++++++++++++++++++++++++++----
>>  2 files changed, 29 insertions(+), 5 deletions(-)
>>
>> diff --git a/include/linux/bitops.h b/include/linux/bitops.h
>> index c1cb53cf2f0f8662ed3e324578f74330e63f935d..9be2d50da09a417966b3d11c84092bb2f4cd0bef 100644
>> --- a/include/linux/bitops.h
>> +++ b/include/linux/bitops.h
>> @@ -8,7 +8,6 @@
>>  
>>  #include <uapi/linux/kernel.h>
>>  
>> -#define BITS_PER_TYPE(type)	(sizeof(type) * BITS_PER_BYTE)
>>  #define BITS_TO_LONGS(nr)	__KERNEL_DIV_ROUND_UP(nr, BITS_PER_TYPE(long))
>>  #define BITS_TO_U64(nr)		__KERNEL_DIV_ROUND_UP(nr, BITS_PER_TYPE(u64))
>>  #define BITS_TO_U32(nr)		__KERNEL_DIV_ROUND_UP(nr, BITS_PER_TYPE(u32))
>> diff --git a/include/linux/bits.h b/include/linux/bits.h
>> index 5f68980a1b98d771426872c74d7b5c0f79e5e802..f202e46d2f4b7899c16d975120f3fa3ae41556ae 100644
>> --- a/include/linux/bits.h
>> +++ b/include/linux/bits.h
>> @@ -12,6 +12,7 @@
>>  #define BIT_ULL_MASK(nr)	(ULL(1) << ((nr) % BITS_PER_LONG_LONG))
>>  #define BIT_ULL_WORD(nr)	((nr) / BITS_PER_LONG_LONG)
>>  #define BITS_PER_BYTE		8
>> +#define BITS_PER_TYPE(type)	(sizeof(type) * BITS_PER_BYTE)
>>  
>>  /*
>>   * Create a contiguous bitmask starting at bit position @l and ending at
>> @@ -25,14 +26,38 @@
>>  
>>  #define GENMASK_INPUT_CHECK(h, l) BUILD_BUG_ON_ZERO(const_true((l) > (h)))
>>  
>> -#define GENMASK(h, l) \
>> -	(GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
>> -#define GENMASK_ULL(h, l) \
>> -	(GENMASK_INPUT_CHECK(h, l) + __GENMASK_ULL(h, l))
>> +/*
>> + * Generate a mask for the specified type @t. Additional checks are made to
>> + * guarantee the value returned fits in that type, relying on
>> + * shift-count-overflow compiler check to detect incompatible arguments.
>> + * For example, all these create build errors or warnings:
>> + *
>> + * - GENMASK(15, 20): wrong argument order
>> + * - GENMASK(72, 15): doesn't fit unsigned long
>> + * - GENMASK_U32(33, 15): doesn't fit in a u32
>> + */
>> +#define GENMASK_t(t, h, l)				\
> 
> Agree with Andy. This should be GENMASK_TYPE, or triple-underscored
> ___GENMASK() maybe. This _t thing looks misleading.

My preference goes to GENMASK_TYPE().

>> +	(GENMASK_INPUT_CHECK(h, l) +			\
>> +	 (((t)~ULL(0) - ((t)1 << (l)) + 1) &		\
>> +	  ((t)~ULL(0) >> (BITS_PER_TYPE(t) - 1 - (h)))))
> 
> Can you rebase it on top of -next? In this dev cycle I merge a patch
> that reverts the __GENMASK() back to:

Oh, I did not realize that. Do you mean a rebase on top of:

  https://github.com/norov/linux/tree/bitmap-for-next

?

I will do so.

> #define __GENMASK(h, l) (((~_UL(0)) << (l)) & (~_UL(0) >> (BITS_PER_LONG - 1 - (h))))
> 
>> +#define GENMASK(h, l) GENMASK_t(unsigned long,  h, l)
>> +#define GENMASK_ULL(h, l) GENMASK_t(unsigned long long, h, l)
> 
> This makes __GENMASK() and __GENMASK_ULL() unused in the kernel, other
> than in uapi. Or I misunderstand it?

Correct.

> Having, in fact, different implementations of the same macro for kernel
> and userspace is a source of problems. Can we move GENMASK_TYPE() to uapi,
> and implement __GENMASK() on top of them? If not, I'd prefer to keep
> GENMASK and GENMASK_ULL untouched.

This is something which I tried to explain in the cover letter. I am not
confident to declare GENMASK_TYPE() in the uapi and expose it to the
userland. If we do so, any future change in the parameters would be a
user breaking change. __GENMASK_U128() looks already too much to me for
the uapi, I am not keen to bloat it even more with GENMASK_TYPE().

This plus the fact that if we use GENMASK_TYPE() to generate the asm
variant, then we can not rely on sizeof() in the definition which makes
everything over complicated.

I acknowledge that not having a common denominator is not best, but I
see this as an acceptable tradeoff.

> Can you run bloat-o-meter and ensure there's no unwanted effects on
> code generation?

Ack, but that will be tomorrow :)

>>  /*
>>   * Missing asm support
>>   *
>> + * __GENMASK_U*() depends on BITS_PER_TYPE() which would not work in the asm
> 
> And there's no __GENMASK_U*(), right?

Yes, silly typo, sorry. Will fix in v5.

>> + * code as BITS_PER_TYPE() relies on sizeof(), something not available in
>> + * asm. Nethertheless, the concept of fixed width integers is a C thing which
>> + * does not apply to assembly code.
>> + */
>> +#define GENMASK_U8(h, l) ((unsigned int)GENMASK_t(u8,  h, l))
>> +#define GENMASK_U16(h, l) ((unsigned int)GENMASK_t(u16, h, l))
> 
> Typecast to the type that user provides explicitly?  And maybe do
> in GENMASK_TYPE()

I have a slight preference for the cast to unsigned int for the reason
explained above. But that is not a deal breaker. If you believe that the
u8/u16 casts are better, let me know, I will be happy to change it :)

>> +#define GENMASK_U32(h, l) GENMASK_t(u32, h, l)
>> +#define GENMASK_U64(h, l) GENMASK_t(u64, h, l)
> 
> OK, this looks good. But GENMASK_U128() becomes a special case now.
> The 128-bit GENMASK is unsued, but it's exported in uapi. Is there any
> simple way to end up with a common implementation for all fixed-type
> GENMASKs?

What bothers me is that the 128 bit types are not something available on
all architectures, c.f. the CONFIG_ARCH_SUPPORTS_INT128. So, I would
need a U128() equivalent to the ULL() but which does not break on
architectures which do not support 128 bits integers.

This is where I am stuck. If someone can guide me on how to write a
robust U128() macro, then I think the common implementation could be
feasible.

>> +
>> +/*
>>   * __GENMASK_U128() depends on _BIT128() which would not work
>>   * in the asm code, as it shifts an 'unsigned __int128' data
>>   * type instead of direct representation of 128 bit constants
> 
> This comment is duplicated by the previous one. Maybe just join them?
> (Let's wait for a while for updates regarding GENMASK_U128 status before
> doing it.)

OK. I will wait for this one. I will probably send the v5 before we get
the answer but I do not this this is an issue if we have two parallel
streams.


Yours sincerely,
Vincent Mailhol

Re: [PATCH v4 3/8] bits: introduce fixed-type genmasks

[Andy Shevchenko]

On Thu, Mar 06, 2025 at 01:48:49AM +0900, Vincent Mailhol wrote:
> On 06/03/2025 at 00:47, Yury Norov wrote:
> > On Wed, Mar 05, 2025 at 10:00:15PM +0900, Vincent Mailhol via B4 Relay wrote:

...

> > Having, in fact, different implementations of the same macro for kernel
> > and userspace is a source of problems. Can we move GENMASK_TYPE() to uapi,
> > and implement __GENMASK() on top of them? If not, I'd prefer to keep
> > GENMASK and GENMASK_ULL untouched.
> 
> This is something which I tried to explain in the cover letter. I am not
> confident to declare GENMASK_TYPE() in the uapi and expose it to the
> userland. If we do so, any future change in the parameters would be a
> user breaking change. __GENMASK_U128() looks already too much to me for
> the uapi, I am not keen to bloat it even more with GENMASK_TYPE().
> 
> This plus the fact that if we use GENMASK_TYPE() to generate the asm
> variant, then we can not rely on sizeof() in the definition which makes
> everything over complicated.

I am with you here. The less we done in uAPI the better.
uAPI is something carved in stone, once done it's impossible to change.

> I acknowledge that not having a common denominator is not best, but I
> see this as an acceptable tradeoff.

...

> >> +#define GENMASK_U8(h, l) ((unsigned int)GENMASK_t(u8,  h, l))
> >> +#define GENMASK_U16(h, l) ((unsigned int)GENMASK_t(u16, h, l))
> > 
> > Typecast to the type that user provides explicitly?  And maybe do
> > in GENMASK_TYPE()
> 
> I have a slight preference for the cast to unsigned int for the reason
> explained above. But that is not a deal breaker. If you believe that the
> u8/u16 casts are better, let me know, I will be happy to change it :)

At least can you provide an existing use case (or use cases) that need
this castings? Also still a big question what will happen with it on asm.
Can it cope with 0x000000f0 passed as imm8, for example?

> >> +#define GENMASK_U32(h, l) GENMASK_t(u32, h, l)
> >> +#define GENMASK_U64(h, l) GENMASK_t(u64, h, l)

...

> > But GENMASK_U128() becomes a special case now.
> > The 128-bit GENMASK is unsued, but it's exported in uapi. Is there any
> > simple way to end up with a common implementation for all fixed-type
> > GENMASKs?
> 
> What bothers me is that the 128 bit types are not something available on
> all architectures, c.f. the CONFIG_ARCH_SUPPORTS_INT128. So, I would
> need a U128() equivalent to the ULL() but which does not break on
> architectures which do not support 128 bits integers.
> 
> This is where I am stuck. If someone can guide me on how to write a
> robust U128() macro, then I think the common implementation could be
> feasible.

I think we may leave that U128 stuff alone for now.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 3/8] bits: introduce fixed-type genmasks

[Vincent Mailhol]

On 06/03/2025 at 04:45, Andy Shevchenko wrote:
>>> But GENMASK_U128() becomes a special case now.
>>> The 128-bit GENMASK is unsued, but it's exported in uapi. Is there any
>>> simple way to end up with a common implementation for all fixed-type
>>> GENMASKs?
>>
>> What bothers me is that the 128 bit types are not something available on
>> all architectures, c.f. the CONFIG_ARCH_SUPPORTS_INT128. So, I would
>> need a U128() equivalent to the ULL() but which does not break on
>> architectures which do not support 128 bits integers.
>>
>> This is where I am stuck. If someone can guide me on how to write a
>> robust U128() macro, then I think the common implementation could be
>> feasible.
> 
> I think we may leave that U128 stuff alone for now.

I found the solution! The trick is to use type_max() from overflow.h.

With this, GENMASK_TYPE() becomes:

  #define GENMASK_TYPE(t, h, l)					\
  	((t)(GENMASK_INPUT_CHECK(h, l) +			\
  	     (type_max(t) << (l) &				\
  	      type_max(t) >> (BITS_PER_TYPE(t) - 1 - (h)))))

and works with all the GENMASK variants, including the U128 one! The
unit tests under lib/test_bits.c are all green.

Of course, this does *not* work in assembly. But as explained before,
GENMASK_TYPE() is guarded by a #if !defined(__ASSEMBLY__), so all good!

The question raised by Yury on whether or not we should keep
__GENMASK_U128() in the uapi still remains. And in full honesty, I will
not touch that one. This is not in the scope of this series.


Yours sincerely,
Vincent Mailhol

Re: [PATCH v4 3/8] bits: introduce fixed-type genmasks

[Andy Shevchenko]

On Thu, Mar 06, 2025 at 06:22:33PM +0900, Vincent Mailhol wrote:
> On 06/03/2025 at 04:45, Andy Shevchenko wrote:
> >>> But GENMASK_U128() becomes a special case now.
> >>> The 128-bit GENMASK is unsued, but it's exported in uapi. Is there any
> >>> simple way to end up with a common implementation for all fixed-type
> >>> GENMASKs?
> >>
> >> What bothers me is that the 128 bit types are not something available on
> >> all architectures, c.f. the CONFIG_ARCH_SUPPORTS_INT128. So, I would
> >> need a U128() equivalent to the ULL() but which does not break on
> >> architectures which do not support 128 bits integers.
> >>
> >> This is where I am stuck. If someone can guide me on how to write a
> >> robust U128() macro, then I think the common implementation could be
> >> feasible.
> > 
> > I think we may leave that U128 stuff alone for now.
> 
> I found the solution! The trick is to use type_max() from overflow.h.
> 
> With this, GENMASK_TYPE() becomes:
> 
>   #define GENMASK_TYPE(t, h, l)					\
>   	((t)(GENMASK_INPUT_CHECK(h, l) +			\
>   	     (type_max(t) << (l) &				\
>   	      type_max(t) >> (BITS_PER_TYPE(t) - 1 - (h)))))
> 
> and works with all the GENMASK variants, including the U128 one! The
> unit tests under lib/test_bits.c are all green.
> 
> Of course, this does *not* work in assembly. But as explained before,
> GENMASK_TYPE() is guarded by a #if !defined(__ASSEMBLY__), so all good!
> 
> The question raised by Yury on whether or not we should keep
> __GENMASK_U128() in the uapi still remains. And in full honesty, I will
> not touch that one. This is not in the scope of this series.

I vote for not touching it right now independently on its destiny.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v4 4/8] bits: introduce fixed-type BIT

[Vincent Mailhol via B4 Relay]

From: Lucas De Marchi <lucas.demarchi@intel.com>

Implement fixed-type BIT to help drivers add stricter checks, like was
done for GENMASK().

Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
Acked-by: Jani Nikula <jani.nikula@intel.com>
Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
Changelog:

  v3 -> v4:

    - Use const_true() to simplify BIT_INPUT_CHECK().

    - Make BIT_U8() and BIT_U16() return an unsigned int instead of a
      u8 and u16. Because of the integer promotion rules in C, an u8
      or an u16 would become a signed integer as soon as these are
      used in any expression. By casting these to unsigned ints, at
      least the signedness is kept.

    - Put the cast next to the BIT() macro.

    - In BIT_U64(): use BIT_ULL() instead of BIT().
---
 include/linux/bits.h | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/include/linux/bits.h b/include/linux/bits.h
index f202e46d2f4b7899c16d975120f3fa3ae41556ae..1b6f5262b79093a01aae6c14ead944e0e85821cc 100644
--- a/include/linux/bits.h
+++ b/include/linux/bits.h
@@ -68,6 +68,22 @@
 #define GENMASK_U128(h, l) \
 	(GENMASK_INPUT_CHECK(h, l) + __GENMASK_U128(h, l))
 
+/*
+ * Fixed-type variants of BIT(), with additional checks like GENMASK_t(). The
+ * following examples generate compiler warnings due to shift-count-overflow:
+ *
+ * - BIT_U8(8)
+ * - BIT_U32(-1)
+ * - BIT_U32(40)
+ */
+#define BIT_INPUT_CHECK(type, b) \
+	BUILD_BUG_ON_ZERO(const_true((b) >= BITS_PER_TYPE(type)))
+
+#define BIT_U8(b) (BIT_INPUT_CHECK(u8, b) + (unsigned int)BIT(b))
+#define BIT_U16(b) (BIT_INPUT_CHECK(u16, b) + (unsigned int)BIT(b))
+#define BIT_U32(b) (BIT_INPUT_CHECK(u32, b) + (u32)BIT(b))
+#define BIT_U64(b) (BIT_INPUT_CHECK(u64, b) + (u64)BIT_ULL(b))
+
 #else /* defined(__ASSEMBLY__) */
 
 #define GENMASK(h, l) __GENMASK(h, l)

-- 
2.45.3

Re: [PATCH v4 4/8] bits: introduce fixed-type BIT

[Andy Shevchenko]

On Wed, Mar 05, 2025 at 10:00:16PM +0900, Vincent Mailhol via B4 Relay wrote:
> From: Lucas De Marchi <lucas.demarchi@intel.com>
> 
> Implement fixed-type BIT to help drivers add stricter checks, like was

Here and in the Subject I would use BIT_Uxx().

> done for GENMASK().

...

> +/*
> + * Fixed-type variants of BIT(), with additional checks like GENMASK_t(). The

GENMASK_t() is not a well named macro.

> + * following examples generate compiler warnings due to shift-count-overflow:
> + *
> + * - BIT_U8(8)
> + * - BIT_U32(-1)
> + * - BIT_U32(40)
> + */
> +#define BIT_INPUT_CHECK(type, b) \
> +	BUILD_BUG_ON_ZERO(const_true((b) >= BITS_PER_TYPE(type)))
> +
> +#define BIT_U8(b) (BIT_INPUT_CHECK(u8, b) + (unsigned int)BIT(b))
> +#define BIT_U16(b) (BIT_INPUT_CHECK(u16, b) + (unsigned int)BIT(b))

Why not u8 and u16? This inconsistency needs to be well justified.

> +#define BIT_U32(b) (BIT_INPUT_CHECK(u32, b) + (u32)BIT(b))
> +#define BIT_U64(b) (BIT_INPUT_CHECK(u64, b) + (u64)BIT_ULL(b))

Can you also use a TAB between the parentheses for better readability?
E.g.,

#define BIT_U64(b)r	(BIT_INPUT_CHECK(u64, b) + (u64)BIT_ULL(b))

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 4/8] bits: introduce fixed-type BIT

[Vincent Mailhol]

On 05/03/2025 at 23:33, Andy Shevchenko wrote:
> On Wed, Mar 05, 2025 at 10:00:16PM +0900, Vincent Mailhol via B4 Relay wrote:
>> From: Lucas De Marchi <lucas.demarchi@intel.com>
>>
>> Implement fixed-type BIT to help drivers add stricter checks, like was
> 
> Here and in the Subject I would use BIT_Uxx().
> 
>> done for GENMASK().
> 
> ...
> 
>> +/*
>> + * Fixed-type variants of BIT(), with additional checks like GENMASK_t(). The
> 
> GENMASK_t() is not a well named macro.

Ack. I will rename to GENMASK_TYPE().

>> + * following examples generate compiler warnings due to shift-count-overflow:
>> + *
>> + * - BIT_U8(8)
>> + * - BIT_U32(-1)
>> + * - BIT_U32(40)
>> + */
>> +#define BIT_INPUT_CHECK(type, b) \
>> +	BUILD_BUG_ON_ZERO(const_true((b) >= BITS_PER_TYPE(type)))
>> +
>> +#define BIT_U8(b) (BIT_INPUT_CHECK(u8, b) + (unsigned int)BIT(b))
>> +#define BIT_U16(b) (BIT_INPUT_CHECK(u16, b) + (unsigned int)BIT(b))
> 
> Why not u8 and u16? This inconsistency needs to be well justified.

Because of the C integer promotion rules, if casted to u8 or u16, the
expression will immediately become a signed integer as soon as it is get
used. For example, if casted to u8

  BIT_U8(0) + BIT_U8(1)

would be a signed integer. And that may surprise people.

David also pointed this in the v3:

https://lore.kernel.org/intel-xe/d42dc197a15649e69d459362849a37f2@AcuMS.aculab.com/

and I agree with his comment.

I explained this in the changelog below the --- cutter, but it is
probably better to make the explanation more visible. I will add a
comment in the code to explain this.

>> +#define BIT_U32(b) (BIT_INPUT_CHECK(u32, b) + (u32)BIT(b))
>> +#define BIT_U64(b) (BIT_INPUT_CHECK(u64, b) + (u64)BIT_ULL(b))
> 
> Can you also use a TAB between the parentheses for better readability?
> E.g.,
> 
> #define BIT_U64(b)r	(BIT_INPUT_CHECK(u64, b) + (u64)BIT_ULL(b))

Sure. I prefer it with space, but no strong opinion. I will put tab in v5.

Yours sincerely,
Vincent Mailhol

Re: [PATCH v4 4/8] bits: introduce fixed-type BIT

[Andy Shevchenko]

On Wed, Mar 05, 2025 at 11:48:10PM +0900, Vincent Mailhol wrote:
> On 05/03/2025 at 23:33, Andy Shevchenko wrote:
> > On Wed, Mar 05, 2025 at 10:00:16PM +0900, Vincent Mailhol via B4 Relay wrote:

...

> >> +#define BIT_U8(b) (BIT_INPUT_CHECK(u8, b) + (unsigned int)BIT(b))
> >> +#define BIT_U16(b) (BIT_INPUT_CHECK(u16, b) + (unsigned int)BIT(b))
> > 
> > Why not u8 and u16? This inconsistency needs to be well justified.
> 
> Because of the C integer promotion rules, if casted to u8 or u16, the
> expression will immediately become a signed integer as soon as it is get
> used. For example, if casted to u8
> 
>   BIT_U8(0) + BIT_U8(1)
> 
> would be a signed integer. And that may surprise people.

Yes, but wouldn't be better to put it more explicitly like

#define BIT_U8(b)	(BIT_INPUT_CHECK(u8, b) + (u8)BIT(b) + 0 + UL(0)) // + ULL(0) ?

Also, BIT_Uxx() gives different type at the end, shouldn't they all be promoted
to unsigned long long at the end? Probably it won't work in real assembly.
Can you add test cases which are written in assembly? (Yes, I understand that it will
be architecture dependent, but still.)

> David also pointed this in the v3:
> 
> https://lore.kernel.org/intel-xe/d42dc197a15649e69d459362849a37f2@AcuMS.aculab.com/
> 
> and I agree with his comment.
> 
> I explained this in the changelog below the --- cutter, but it is
> probably better to make the explanation more visible. I will add a
> comment in the code to explain this.
> 
> >> +#define BIT_U32(b) (BIT_INPUT_CHECK(u32, b) + (u32)BIT(b))
> >> +#define BIT_U64(b) (BIT_INPUT_CHECK(u64, b) + (u64)BIT_ULL(b))

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 4/8] bits: introduce fixed-type BIT

[Vincent Mailhol]

On 06/03/2025 at 00:48, Andy Shevchenko wrote:
> On Wed, Mar 05, 2025 at 11:48:10PM +0900, Vincent Mailhol wrote:
>> On 05/03/2025 at 23:33, Andy Shevchenko wrote:
>>> On Wed, Mar 05, 2025 at 10:00:16PM +0900, Vincent Mailhol via B4 Relay wrote:
> 
> ...
> 
>>>> +#define BIT_U8(b) (BIT_INPUT_CHECK(u8, b) + (unsigned int)BIT(b))
>>>> +#define BIT_U16(b) (BIT_INPUT_CHECK(u16, b) + (unsigned int)BIT(b))
>>>
>>> Why not u8 and u16? This inconsistency needs to be well justified.
>>
>> Because of the C integer promotion rules, if casted to u8 or u16, the
>> expression will immediately become a signed integer as soon as it is get
>> used. For example, if casted to u8
>>
>>   BIT_U8(0) + BIT_U8(1)
>>
>> would be a signed integer. And that may surprise people.
> 
> Yes, but wouldn't be better to put it more explicitly like
> 
> #define BIT_U8(b)	(BIT_INPUT_CHECK(u8, b) + (u8)BIT(b) + 0 + UL(0)) // + ULL(0) ?

OK, the final result would be unsigned. But, I do not follow how this is
more explicit.

Also, why doing:

  (u8)BIT(b) + 0 + UL(0)

and not just:

  (u8)BIT(b) + UL(0)

?

What is that intermediary '+ 0' for?

I am sorry, but I am having a hard time understanding how casting to u8
and then doing an addition with an unsigned long is more explicit than
directly doing a cast to the desired type.

As I mentioned in my answer to Yuri, I have a slight preference for the
unsigned int cast, but I am OK to go back to the u8/u16 cast as it was
in v3.

However, I really do not see how that '+ 0 + UL(0)' would be an improvement.

> Also, BIT_Uxx() gives different type at the end, shouldn't they all be promoted
> to unsigned long long at the end? Probably it won't work in real assembly.
> Can you add test cases which are written in assembly? (Yes, I understand that it will
> be architecture dependent, but still.)

No. I purposely guarded the definition of the BIT_Uxx() by a

  #if !defined(__ASSEMBLY__)

so that these are never visible in assembly. I actually put a comment to
explain why the GENMASK_U*() are not available in assembly. I can copy
paste the same comment to explain why why BIT_U*() are not made
available either:

  /*
   * Missing asm support
   *
   * BIT_U*() depends on BITS_PER_TYPE() which would not work in the asm
   * code as BITS_PER_TYPE() relies on sizeof(), something not available
   * in asm.  Nethertheless, the concept of fixed width integers is a C
   * thing which does not apply to assembly code.
   */

I really believe that it would be a mistake to make the GENMASK_U*() or
the BIT_U*() available to assembly.

>> David also pointed this in the v3:
>>
>> https://lore.kernel.org/intel-xe/d42dc197a15649e69d459362849a37f2@AcuMS.aculab.com/
>>
>> and I agree with his comment.
>>
>> I explained this in the changelog below the --- cutter, but it is
>> probably better to make the explanation more visible. I will add a
>> comment in the code to explain this.
>>
>>>> +#define BIT_U32(b) (BIT_INPUT_CHECK(u32, b) + (u32)BIT(b))
>>>> +#define BIT_U64(b) (BIT_INPUT_CHECK(u64, b) + (u64)BIT_ULL(b))
> 

Yours sincerely,
Vincent Mailhol

Re: [PATCH v4 4/8] bits: introduce fixed-type BIT

[Andy Shevchenko]

On Thu, Mar 06, 2025 at 02:17:18AM +0900, Vincent Mailhol wrote:
> On 06/03/2025 at 00:48, Andy Shevchenko wrote:
> > On Wed, Mar 05, 2025 at 11:48:10PM +0900, Vincent Mailhol wrote:
> >> On 05/03/2025 at 23:33, Andy Shevchenko wrote:
> >>> On Wed, Mar 05, 2025 at 10:00:16PM +0900, Vincent Mailhol via B4 Relay wrote:

...

> >>>> +#define BIT_U8(b) (BIT_INPUT_CHECK(u8, b) + (unsigned int)BIT(b))
> >>>> +#define BIT_U16(b) (BIT_INPUT_CHECK(u16, b) + (unsigned int)BIT(b))
> >>>
> >>> Why not u8 and u16? This inconsistency needs to be well justified.
> >>
> >> Because of the C integer promotion rules, if casted to u8 or u16, the
> >> expression will immediately become a signed integer as soon as it is get
> >> used. For example, if casted to u8
> >>
> >>   BIT_U8(0) + BIT_U8(1)
> >>
> >> would be a signed integer. And that may surprise people.
> > 
> > Yes, but wouldn't be better to put it more explicitly like
> > 
> > #define BIT_U8(b)	(BIT_INPUT_CHECK(u8, b) + (u8)BIT(b) + 0 + UL(0)) // + ULL(0) ?
> 
> OK, the final result would be unsigned. But, I do not follow how this is
> more explicit.
> 
> Also, why doing:
> 
>   (u8)BIT(b) + 0 + UL(0)
> 
> and not just:
> 
>   (u8)BIT(b) + UL(0)
> 
> ?
> 
> What is that intermediary '+ 0' for?
> 
> I am sorry, but I am having a hard time understanding how casting to u8
> and then doing an addition with an unsigned long is more explicit than
> directly doing a cast to the desired type.

Reading this again, I think we don't need it at all. u8, aka unsigned char,
will be promoted to int, but it will be int with a value < 256, can't be signed
as far as I understand this correctly.

> As I mentioned in my answer to Yuri, I have a slight preference for the
> unsigned int cast, but I am OK to go back to the u8/u16 cast as it was
> in v3.

Which means that the simples uXX castings should suffice. In any case we need
test cases for that.

> However, I really do not see how that '+ 0 + UL(0)' would be an improvement.
> 
> > Also, BIT_Uxx() gives different type at the end, shouldn't they all be promoted
> > to unsigned long long at the end? Probably it won't work in real assembly.
> > Can you add test cases which are written in assembly? (Yes, I understand that it will
> > be architecture dependent, but still.)
> 
> No. I purposely guarded the definition of the BIT_Uxx() by a
> 
>   #if !defined(__ASSEMBLY__)
> 
> so that these are never visible in assembly. I actually put a comment to
> explain why the GENMASK_U*() are not available in assembly. I can copy
> paste the same comment to explain why why BIT_U*() are not made
> available either:
> 
>   /*
>    * Missing asm support
>    *
>    * BIT_U*() depends on BITS_PER_TYPE() which would not work in the asm
>    * code as BITS_PER_TYPE() relies on sizeof(), something not available
>    * in asm.  Nethertheless, the concept of fixed width integers is a C
>    * thing which does not apply to assembly code.
>    */
> 
> I really believe that it would be a mistake to make the GENMASK_U*() or
> the BIT_U*() available to assembly.

Ah, okay then!

> >> David also pointed this in the v3:
> >>
> >> https://lore.kernel.org/intel-xe/d42dc197a15649e69d459362849a37f2@AcuMS.aculab.com/
> >>
> >> and I agree with his comment.

Why unsigned char won't work?

> >> I explained this in the changelog below the --- cutter, but it is
> >> probably better to make the explanation more visible. I will add a
> >> comment in the code to explain this.
> >>
> >>>> +#define BIT_U32(b) (BIT_INPUT_CHECK(u32, b) + (u32)BIT(b))
> >>>> +#define BIT_U64(b) (BIT_INPUT_CHECK(u64, b) + (u64)BIT_ULL(b))

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 4/8] bits: introduce fixed-type BIT

[David Laight]

On Wed, 5 Mar 2025 21:56:22 +0200
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Thu, Mar 06, 2025 at 02:17:18AM +0900, Vincent Mailhol wrote:
> > On 06/03/2025 at 00:48, Andy Shevchenko wrote:  
> > > On Wed, Mar 05, 2025 at 11:48:10PM +0900, Vincent Mailhol wrote:  
> > >> On 05/03/2025 at 23:33, Andy Shevchenko wrote:  
> > >>> On Wed, Mar 05, 2025 at 10:00:16PM +0900, Vincent Mailhol via B4 Relay wrote:  
> 
> ...
> 
> > >>>> +#define BIT_U8(b) (BIT_INPUT_CHECK(u8, b) + (unsigned int)BIT(b))
> > >>>> +#define BIT_U16(b) (BIT_INPUT_CHECK(u16, b) + (unsigned int)BIT(b))  
> > >>>
> > >>> Why not u8 and u16? This inconsistency needs to be well justified.  
> > >>
> > >> Because of the C integer promotion rules, if casted to u8 or u16, the
> > >> expression will immediately become a signed integer as soon as it is get
> > >> used. For example, if casted to u8
> > >>
> > >>   BIT_U8(0) + BIT_U8(1)
> > >>
> > >> would be a signed integer. And that may surprise people.  
> > > 
> > > Yes, but wouldn't be better to put it more explicitly like
> > > 
> > > #define BIT_U8(b)	(BIT_INPUT_CHECK(u8, b) + (u8)BIT(b) + 0 + UL(0)) // + ULL(0) ?  
> > 
> > OK, the final result would be unsigned. But, I do not follow how this is
> > more explicit.
> > 
> > Also, why doing:
> > 
> >   (u8)BIT(b) + 0 + UL(0)
> > 
> > and not just:
> > 
> >   (u8)BIT(b) + UL(0)
> > 
> > ?
> > 
> > What is that intermediary '+ 0' for?
> > 
> > I am sorry, but I am having a hard time understanding how casting to u8
> > and then doing an addition with an unsigned long is more explicit than
> > directly doing a cast to the desired type.  
> 
> Reading this again, I think we don't need it at all. u8, aka unsigned char,
> will be promoted to int, but it will be int with a value < 256, can't be signed
> as far as I understand this correctly.

The value can't be negative, but the type will be a signed one.
Anything comparing types (and there are a few) will treat it as signed.
It really is bad practise to even pretend you can have an expression
(rather that a variable) that has a type smaller than 'int'.
It wouldn't surprise me if even an 'a = b' assignment promotes 'b' to int.

So it is even questionable whether BIT8() and BIT16() should even exist at all.
There can be reasons to return 'unsigned int' rather than 'unsigned long'.
But with the type definitions that Linux uses (and can't really be changed)
you can have BIT32() that is 'unsigned int' and BIT64() that is 'unsigned long
long'. These are then the same on 32bit and 64bit.

	David 

Re: [PATCH v4 4/8] bits: introduce fixed-type BIT

[Jani Nikula]

On Wed, 05 Mar 2025, David Laight <david.laight.linux@gmail.com> wrote:
> So it is even questionable whether BIT8() and BIT16() should even
> exist at all.

If nothing else, they do provide compile time checks for the bit being
0..7 and 0..15, respectively.

BR,
Jani.


-- 
Jani Nikula, Intel

Re: [PATCH v4 4/8] bits: introduce fixed-type BIT

[Andy Shevchenko]

On Wed, Mar 05, 2025 at 09:50:27PM +0000, David Laight wrote:
> On Wed, 5 Mar 2025 21:56:22 +0200
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> > On Thu, Mar 06, 2025 at 02:17:18AM +0900, Vincent Mailhol wrote:
> > > On 06/03/2025 at 00:48, Andy Shevchenko wrote:  
> > > > On Wed, Mar 05, 2025 at 11:48:10PM +0900, Vincent Mailhol wrote:  
> > > >> On 05/03/2025 at 23:33, Andy Shevchenko wrote:  
> > > >>> On Wed, Mar 05, 2025 at 10:00:16PM +0900, Vincent Mailhol via B4 Relay wrote:  

...

> > > >>>> +#define BIT_U8(b) (BIT_INPUT_CHECK(u8, b) + (unsigned int)BIT(b))
> > > >>>> +#define BIT_U16(b) (BIT_INPUT_CHECK(u16, b) + (unsigned int)BIT(b))  
> > > >>>
> > > >>> Why not u8 and u16? This inconsistency needs to be well justified.  
> > > >>
> > > >> Because of the C integer promotion rules, if casted to u8 or u16, the
> > > >> expression will immediately become a signed integer as soon as it is get
> > > >> used. For example, if casted to u8
> > > >>
> > > >>   BIT_U8(0) + BIT_U8(1)
> > > >>
> > > >> would be a signed integer. And that may surprise people.  
> > > > 
> > > > Yes, but wouldn't be better to put it more explicitly like
> > > > 
> > > > #define BIT_U8(b)	(BIT_INPUT_CHECK(u8, b) + (u8)BIT(b) + 0 + UL(0)) // + ULL(0) ?  
> > > 
> > > OK, the final result would be unsigned. But, I do not follow how this is
> > > more explicit.
> > > 
> > > Also, why doing:
> > > 
> > >   (u8)BIT(b) + 0 + UL(0)
> > > 
> > > and not just:
> > > 
> > >   (u8)BIT(b) + UL(0)
> > > 
> > > ?
> > > 
> > > What is that intermediary '+ 0' for?
> > > 
> > > I am sorry, but I am having a hard time understanding how casting to u8
> > > and then doing an addition with an unsigned long is more explicit than
> > > directly doing a cast to the desired type.  
> > 
> > Reading this again, I think we don't need it at all. u8, aka unsigned char,
> > will be promoted to int, but it will be int with a value < 256, can't be signed
> > as far as I understand this correctly.
> 
> The value can't be negative, but the type will be a signed one.

Yes, that's what I mentioned above: "int with the value < 256".

> Anything comparing types (and there are a few) will treat it as signed.
> It really is bad practise to even pretend you can have an expression
> (rather that a variable) that has a type smaller than 'int'.
> It wouldn't surprise me if even an 'a = b' assignment promotes 'b' to int.

We have tons of code with u8/u16, what you are proposing here is like
"let's get rid of those types and replace all of them by int/unsigned int".
We have ISAs that are byte-oriented despite being 32- or 64-bit platforms.

> So it is even questionable whether BIT8() and BIT16() should even exist at all.

The point is to check the boundaries and not in the returned value per se.

> There can be reasons to return 'unsigned int' rather than 'unsigned long'.
> But with the type definitions that Linux uses (and can't really be changed)
> you can have BIT32() that is 'unsigned int' and BIT64() that is 'unsigned long
> long'. These are then the same on 32bit and 64bit.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 4/8] bits: introduce fixed-type BIT

[Vincent Mailhol]

On 06/03/2025 at 18:12, Andy Shevchenko wrote:
> On Wed, Mar 05, 2025 at 09:50:27PM +0000, David Laight wrote:
>> On Wed, 5 Mar 2025 21:56:22 +0200
>> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
>>> On Thu, Mar 06, 2025 at 02:17:18AM +0900, Vincent Mailhol wrote:
>>>> On 06/03/2025 at 00:48, Andy Shevchenko wrote:  
>>>>> On Wed, Mar 05, 2025 at 11:48:10PM +0900, Vincent Mailhol wrote:  
>>>>>> On 05/03/2025 at 23:33, Andy Shevchenko wrote:  
>>>>>>> On Wed, Mar 05, 2025 at 10:00:16PM +0900, Vincent Mailhol via B4 Relay wrote:  
> 
> ...
> 
>>>>>>>> +#define BIT_U8(b) (BIT_INPUT_CHECK(u8, b) + (unsigned int)BIT(b))
>>>>>>>> +#define BIT_U16(b) (BIT_INPUT_CHECK(u16, b) + (unsigned int)BIT(b))  
>>>>>>>
>>>>>>> Why not u8 and u16? This inconsistency needs to be well justified.  
>>>>>>
>>>>>> Because of the C integer promotion rules, if casted to u8 or u16, the
>>>>>> expression will immediately become a signed integer as soon as it is get
>>>>>> used. For example, if casted to u8
>>>>>>
>>>>>>   BIT_U8(0) + BIT_U8(1)
>>>>>>
>>>>>> would be a signed integer. And that may surprise people.  
>>>>>
>>>>> Yes, but wouldn't be better to put it more explicitly like
>>>>>
>>>>> #define BIT_U8(b)	(BIT_INPUT_CHECK(u8, b) + (u8)BIT(b) + 0 + UL(0)) // + ULL(0) ?  
>>>>
>>>> OK, the final result would be unsigned. But, I do not follow how this is
>>>> more explicit.
>>>>
>>>> Also, why doing:
>>>>
>>>>   (u8)BIT(b) + 0 + UL(0)
>>>>
>>>> and not just:
>>>>
>>>>   (u8)BIT(b) + UL(0)
>>>>
>>>> ?
>>>>
>>>> What is that intermediary '+ 0' for?
>>>>
>>>> I am sorry, but I am having a hard time understanding how casting to u8
>>>> and then doing an addition with an unsigned long is more explicit than
>>>> directly doing a cast to the desired type.  
>>>
>>> Reading this again, I think we don't need it at all. u8, aka unsigned char,
>>> will be promoted to int, but it will be int with a value < 256, can't be signed
>>> as far as I understand this correctly.
>>
>> The value can't be negative, but the type will be a signed one.
> 
> Yes, that's what I mentioned above: "int with the value < 256".
> 
>> Anything comparing types (and there are a few) will treat it as signed.
>> It really is bad practise to even pretend you can have an expression
>> (rather that a variable) that has a type smaller than 'int'.
>> It wouldn't surprise me if even an 'a = b' assignment promotes 'b' to int.
> 
> We have tons of code with u8/u16, what you are proposing here is like
> "let's get rid of those types and replace all of them by int/unsigned int".
> We have ISAs that are byte-oriented despite being 32- or 64-bit platforms.
> 
>> So it is even questionable whether BIT8() and BIT16() should even exist at all.
> 
> The point is to check the boundaries and not in the returned value per se.

+1

I will also add that this adds to the readability of the code. In a
driver, if I see:

  #define REG_FOO1_MASK GENMASK(6, 2)
  #define REG_FOO2_MASK GENMASK(12, 7)

it does not tell me much about the register. Whereas if I see:

  #define REG_FOO1_MASK GENMASK_U16(6, 2)
  #define REG_FOO2_MASK GENMASK_U16(12, 7)

then I know that this is for a 16 bit register.

>> There can be reasons to return 'unsigned int' rather than 'unsigned long'.
>> But with the type definitions that Linux uses (and can't really be changed)
>> you can have BIT32() that is 'unsigned int' and BIT64() that is 'unsigned long
>> long'. These are then the same on 32bit and 64bit.

So, at the end, my goal when introducing that unsigned int cast was not
to confuse people. This had the opposite effect. Nearly all the
reviewers pointed at that cast.

I will revert this in the v5. The U8 and U16 variants of both GENMASK
and BIT will return an u8 and u16 respectively. And unless someone
manages to convince Yury otherwise, I will keep it as such.


Yours sincerely,
Vincent Mailhol

Re: [PATCH v4 4/8] bits: introduce fixed-type BIT

[David Laight]

On Wed, 5 Mar 2025 17:48:05 +0200
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Wed, Mar 05, 2025 at 11:48:10PM +0900, Vincent Mailhol wrote:
> > On 05/03/2025 at 23:33, Andy Shevchenko wrote:  
> > > On Wed, Mar 05, 2025 at 10:00:16PM +0900, Vincent Mailhol via B4 Relay wrote:  
> 
> ...
> 
> > >> +#define BIT_U8(b) (BIT_INPUT_CHECK(u8, b) + (unsigned int)BIT(b))
> > >> +#define BIT_U16(b) (BIT_INPUT_CHECK(u16, b) + (unsigned int)BIT(b))

Why even pretend you are checking against a type - just use 8 or 16.

> > > 
> > > Why not u8 and u16? This inconsistency needs to be well justified.

What is the type of BIT(b) ?
it really ought to be unsigned int (so always 32bit), but I bet it
is unsigned long (possibly historically because someone was worried
int might be 16 bits!)

> > 
> > Because of the C integer promotion rules, if casted to u8 or u16, the
> > expression will immediately become a signed integer as soon as it is get
> > used. For example, if casted to u8
> > 
> >   BIT_U8(0) + BIT_U8(1)
> > 
> > would be a signed integer. And that may surprise people.

They always get 'surprised' by that.
I found some 'dayjob' code that was doing (byte_var << 1) >> 1 in order
to get the high bit discarded.
Been like that for best part of 30 years...
I wasn't scared to fix it :-)

> Yes, but wouldn't be better to put it more explicitly like
> 
> #define BIT_U8(b)	(BIT_INPUT_CHECK(u8, b) + (u8)BIT(b) + 0 + UL(0)) // + ULL(0) ?

I don't think you should force it to 'unsigned long'.
On 64bit a comparison against a 32bit 'signed int' will sign-extend the
value before making it unsigned.
While that shouldn't matter here, someone might copy it.
You just want to ensure that all the values are 'unsigned int', trying
to return u8 or u16 isn't worth the effort.

When I was doing min_unsigned() I did ((x) + 0u + 0ul + 0ull) to ensure
that values would always be zero extended.
But I was doing the same to both sides of the expression - and the compiler
optimises away all the 'known 0' extension to 64bits.

> Also, BIT_Uxx() gives different type at the end, shouldn't they all be promoted
> to unsigned long long at the end? Probably it won't work in real assembly.
> Can you add test cases which are written in assembly? (Yes, I understand that it will
> be architecture dependent, but still.)

There is no point doing multiple versions for asm files.
The reason UL(x) and ULL(x) exist is because the assembler just has integers.
Both expand to (x).
There might not even be a distinction between signed and unsigned.
I'm not sure you can assume that a shift right won't replicate the sign bit.
Since the expression can only be valid for constants, something simple
like ((2 << (hi)) - (1 << (lo)) really is the best you are going to get
for GENMASK().

So just define a completely different version for asm any nuke the UL() etc
for readability.

	David

[PATCH v4 5/8] drm/i915: Convert REG_GENMASK* to fixed-width GENMASK_*

[Vincent Mailhol via B4 Relay]

From: Lucas De Marchi <lucas.demarchi@intel.com>

Now that include/linux/bits.h implements fixed-width GENMASK_*, use them
to implement the i915/xe specific macros. Converting each driver to use
the generic macros are left for later, when/if other driver-specific
macros are also generalized.

Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
Acked-by: Jani Nikula <jani.nikula@intel.com>
Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
Changelog:

  v3 -> v4:

    - Remove the prefixes in macro parameters,
      e.g. 'REG_GENMASK(__high, __low)' -> 'REG_GENMASK(high, low)'
---
 drivers/gpu/drm/i915/i915_reg_defs.h | 108 ++++-------------------------------
 1 file changed, 11 insertions(+), 97 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg_defs.h b/drivers/gpu/drm/i915/i915_reg_defs.h
index e251bcc0c89f5710125bc70f07851b2cb978c89c..39e5ed9511174b8757b9201bff735fa362651b34 100644
--- a/drivers/gpu/drm/i915/i915_reg_defs.h
+++ b/drivers/gpu/drm/i915/i915_reg_defs.h
@@ -9,76 +9,19 @@
 #include <linux/bitfield.h>
 #include <linux/bits.h>
 
-/**
- * REG_BIT() - Prepare a u32 bit value
- * @__n: 0-based bit number
- *
- * Local wrapper for BIT() to force u32, with compile time checks.
- *
- * @return: Value with bit @__n set.
- */
-#define REG_BIT(__n)							\
-	((u32)(BIT(__n) +						\
-	       BUILD_BUG_ON_ZERO(__is_constexpr(__n) &&		\
-				 ((__n) < 0 || (__n) > 31))))
-
-/**
- * REG_BIT8() - Prepare a u8 bit value
- * @__n: 0-based bit number
- *
- * Local wrapper for BIT() to force u8, with compile time checks.
- *
- * @return: Value with bit @__n set.
- */
-#define REG_BIT8(__n)                                                   \
-	((u8)(BIT(__n) +                                                \
-	       BUILD_BUG_ON_ZERO(__is_constexpr(__n) &&         \
-				 ((__n) < 0 || (__n) > 7))))
-
-/**
- * REG_GENMASK() - Prepare a continuous u32 bitmask
- * @__high: 0-based high bit
- * @__low: 0-based low bit
- *
- * Local wrapper for GENMASK() to force u32, with compile time checks.
- *
- * @return: Continuous bitmask from @__high to @__low, inclusive.
- */
-#define REG_GENMASK(__high, __low)					\
-	((u32)(GENMASK(__high, __low) +					\
-	       BUILD_BUG_ON_ZERO(__is_constexpr(__high) &&	\
-				 __is_constexpr(__low) &&		\
-				 ((__low) < 0 || (__high) > 31 || (__low) > (__high)))))
-
-/**
- * REG_GENMASK64() - Prepare a continuous u64 bitmask
- * @__high: 0-based high bit
- * @__low: 0-based low bit
- *
- * Local wrapper for GENMASK_ULL() to force u64, with compile time checks.
- *
- * @return: Continuous bitmask from @__high to @__low, inclusive.
+/*
+ * Wrappers over the generic BIT_* and GENMASK_* implementations,
+ * for compatibility reasons with previous implementation
  */
-#define REG_GENMASK64(__high, __low)					\
-	((u64)(GENMASK_ULL(__high, __low) +				\
-	       BUILD_BUG_ON_ZERO(__is_constexpr(__high) &&		\
-				 __is_constexpr(__low) &&		\
-				 ((__low) < 0 || (__high) > 63 || (__low) > (__high)))))
+#define REG_GENMASK(high, low)		GENMASK_U32(high, low)
+#define REG_GENMASK64(high, low)	GENMASK_U64(high, low)
+#define REG_GENMASK16(high, low)	GENMASK_U16(high, low)
+#define REG_GENMASK8(high, low)		GENMASK_U8(high, low)
 
-/**
- * REG_GENMASK8() - Prepare a continuous u8 bitmask
- * @__high: 0-based high bit
- * @__low: 0-based low bit
- *
- * Local wrapper for GENMASK() to force u8, with compile time checks.
- *
- * @return: Continuous bitmask from @__high to @__low, inclusive.
- */
-#define REG_GENMASK8(__high, __low)                                     \
-	((u8)(GENMASK(__high, __low) +                                  \
-	       BUILD_BUG_ON_ZERO(__is_constexpr(__high) &&      \
-				 __is_constexpr(__low) &&               \
-				 ((__low) < 0 || (__high) > 7 || (__low) > (__high)))))
+#define REG_BIT(n)			BIT_U32(n)
+#define REG_BIT64(n)			BIT_U64(n)
+#define REG_BIT16(n)			BIT_U16(n)
+#define REG_BIT8(n)			BIT_U8(n)
 
 /*
  * Local integer constant expression version of is_power_of_2().
@@ -143,35 +86,6 @@
  */
 #define REG_FIELD_GET64(__mask, __val)	((u64)FIELD_GET(__mask, __val))
 
-/**
- * REG_BIT16() - Prepare a u16 bit value
- * @__n: 0-based bit number
- *
- * Local wrapper for BIT() to force u16, with compile time
- * checks.
- *
- * @return: Value with bit @__n set.
- */
-#define REG_BIT16(__n)                                                   \
-	((u16)(BIT(__n) +                                                \
-	       BUILD_BUG_ON_ZERO(__is_constexpr(__n) &&         \
-				 ((__n) < 0 || (__n) > 15))))
-
-/**
- * REG_GENMASK16() - Prepare a continuous u8 bitmask
- * @__high: 0-based high bit
- * @__low: 0-based low bit
- *
- * Local wrapper for GENMASK() to force u16, with compile time
- * checks.
- *
- * @return: Continuous bitmask from @__high to @__low, inclusive.
- */
-#define REG_GENMASK16(__high, __low)                                     \
-	((u16)(GENMASK(__high, __low) +                                  \
-	       BUILD_BUG_ON_ZERO(__is_constexpr(__high) &&      \
-				 __is_constexpr(__low) &&               \
-				 ((__low) < 0 || (__high) > 15 || (__low) > (__high)))))
 
 /**
  * REG_FIELD_PREP16() - Prepare a u16 bitfield value

-- 
2.45.3

Re: [PATCH v4 5/8] drm/i915: Convert REG_GENMASK* to fixed-width GENMASK_*

[Andy Shevchenko]

On Wed, Mar 05, 2025 at 10:00:17PM +0900, Vincent Mailhol via B4 Relay wrote:
> From: Lucas De Marchi <lucas.demarchi@intel.com>
> 
> Now that include/linux/bits.h implements fixed-width GENMASK_*, use them

GENMASK_*()

and in the Subject

REG_GENMASK*()

> to implement the i915/xe specific macros. Converting each driver to use
> the generic macros are left for later, when/if other driver-specific
> macros are also generalized.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v4 6/8] test_bits: add tests for __GENMASK() and __GENMASK_ULL()

[Vincent Mailhol via B4 Relay]

From: Vincent Mailhol <mailhol.vincent@wanadoo.fr>

The definitions of GENMASK() and GENMASK_ULL() do not depend any more
on __GENMASK() and __GENMASK_ULL(). Duplicate the existing unit tests
so that __GENMASK{,ULL}() is still covered.

Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
 lib/test_bits.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/lib/test_bits.c b/lib/test_bits.c
index c7b38d91e1f16d42b7ca92e62fbd6c19b37e76a0..dc93ded9fdb201e0d44b3c1cd71e233fd62258a5 100644
--- a/lib/test_bits.c
+++ b/lib/test_bits.c
@@ -7,6 +7,22 @@
 #include <linux/bits.h>
 
 
+static void __genmask_test(struct kunit *test)
+{
+	KUNIT_EXPECT_EQ(test, 1ul, __GENMASK(0, 0));
+	KUNIT_EXPECT_EQ(test, 3ul, __GENMASK(1, 0));
+	KUNIT_EXPECT_EQ(test, 6ul, __GENMASK(2, 1));
+	KUNIT_EXPECT_EQ(test, 0xFFFFFFFFul, __GENMASK(31, 0));
+}
+
+static void __genmask_ull_test(struct kunit *test)
+{
+	KUNIT_EXPECT_EQ(test, 1ull, __GENMASK_ULL(0, 0));
+	KUNIT_EXPECT_EQ(test, 3ull, __GENMASK_ULL(1, 0));
+	KUNIT_EXPECT_EQ(test, 0x000000ffffe00000ull, __GENMASK_ULL(39, 21));
+	KUNIT_EXPECT_EQ(test, 0xffffffffffffffffull, __GENMASK_ULL(63, 0));
+}
+
 static void genmask_test(struct kunit *test)
 {
 	KUNIT_EXPECT_EQ(test, 1ul, GENMASK(0, 0));
@@ -93,6 +109,8 @@ 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),
 	KUNIT_CASE(genmask_test),
 	KUNIT_CASE(genmask_ull_test),
 	KUNIT_CASE(genmask_u128_test),

-- 
2.45.3

[PATCH v4 7/8] test_bits: add tests for fixed-type genmasks

[Vincent Mailhol via B4 Relay]

From: Lucas De Marchi <lucas.demarchi@intel.com>

Add some additional tests in lib/test_bits.c to cover the
expected/non-expected values of the fixed-type genmasks.

Also check that the result value matches the expected type. Since
those are known at build time, use static_assert() instead of normal
kunit tests.

Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
Changelog:

  v3 -> v4:

    - Adjust the type of GENMASK_U8()/GENMASK_U16() from u8/u16 to
      unsigned int.

    - Reorder the tests to match the order in which the macros are
      declared in bits.h.
---
 lib/test_bits.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/lib/test_bits.c b/lib/test_bits.c
index dc93ded9fdb201e0d44b3c1cd71e233fd62258a5..c3a40995a2577322252371eb10ada0c33fb5d9b4 100644
--- a/lib/test_bits.c
+++ b/lib/test_bits.c
@@ -5,7 +5,16 @@
 
 #include <kunit/test.h>
 #include <linux/bits.h>
+#include <linux/types.h>
 
+#define assert_type(t, x) _Generic(x, t: x, default: 0)
+
+static_assert(assert_type(unsigned long, GENMASK(31, 0)) == U32_MAX);
+static_assert(assert_type(unsigned long long, GENMASK_ULL(63, 0)) == U64_MAX);
+static_assert(assert_type(unsigned int, GENMASK_U8(7, 0)) == U8_MAX);
+static_assert(assert_type(unsigned int, GENMASK_U16(15, 0)) == U16_MAX);
+static_assert(assert_type(u32, GENMASK_U32(31, 0)) == U32_MAX);
+static_assert(assert_type(u64, GENMASK_U64(63, 0)) == U64_MAX);
 
 static void __genmask_test(struct kunit *test)
 {
@@ -30,11 +39,21 @@ static void genmask_test(struct kunit *test)
 	KUNIT_EXPECT_EQ(test, 6ul, GENMASK(2, 1));
 	KUNIT_EXPECT_EQ(test, 0xFFFFFFFFul, GENMASK(31, 0));
 
+	KUNIT_EXPECT_EQ(test, 1u, GENMASK_U8(0, 0));
+	KUNIT_EXPECT_EQ(test, 3u, GENMASK_U16(1, 0));
+	KUNIT_EXPECT_EQ(test, 0x10000, GENMASK_U32(16, 16));
+
 #ifdef TEST_GENMASK_FAILURES
 	/* these should fail compilation */
 	GENMASK(0, 1);
 	GENMASK(0, 10);
 	GENMASK(9, 10);
+
+	GENMASK_U32(0, 31);
+	GENMASK_U64(64, 0);
+	GENMASK_U32(32, 0);
+	GENMASK_U16(16, 0);
+	GENMASK_U8(8, 0);
 #endif
 
 

-- 
2.45.3

[PATCH v4 8/8] test_bits: add tests for fixed-type BIT

[Vincent Mailhol via B4 Relay]

From: Vincent Mailhol <mailhol.vincent@wanadoo.fr>

Add some additional tests in lib/test_bits.c to cover the expected
results of the BIT_U*() macros.

Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
 lib/test_bits.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/lib/test_bits.c b/lib/test_bits.c
index c3a40995a2577322252371eb10ada0c33fb5d9b4..641001a04f21bc3b788ae05c6d2eaaba9052e463 100644
--- a/lib/test_bits.c
+++ b/lib/test_bits.c
@@ -9,6 +9,16 @@
 
 #define assert_type(t, x) _Generic(x, t: x, default: 0)
 
+static_assert(assert_type(unsigned int, BIT_U8(0)) == 1u);
+static_assert(assert_type(unsigned int, BIT_U16(0)) == 1u);
+static_assert(assert_type(u32, BIT_U32(0)) == 1u);
+static_assert(assert_type(u64, BIT_U64(0)) == 1ull);
+
+static_assert(assert_type(unsigned int, BIT_U8(7)) == 0x80u);
+static_assert(assert_type(unsigned int, BIT_U16(15)) == 0x8000u);
+static_assert(assert_type(u32, BIT_U32(31)) == 0x80000000u);
+static_assert(assert_type(u64, BIT_U64(63)) == 0x8000000000000000ull);
+
 static_assert(assert_type(unsigned long, GENMASK(31, 0)) == U32_MAX);
 static_assert(assert_type(unsigned long long, GENMASK_ULL(63, 0)) == U64_MAX);
 static_assert(assert_type(unsigned int, GENMASK_U8(7, 0)) == U8_MAX);

-- 
2.45.3

Re: [PATCH v4 0/8] bits: Fixed-type GENMASK()/BIT()

[Jani Nikula]

On Wed, 05 Mar 2025, Vincent Mailhol via B4 Relay <devnull+mailhol.vincent.wanadoo.fr@kernel.org> wrote:
> Introduce some fixed width variant of the GENMASK() and the BIT()
> macros in bits.h. Note that the main goal is not to get the correct
> type, but rather to enforce more checks at compile time. For example:
>
>   GENMASK_U16(16, 0)
>
> will raise a build bug.
>
> This series is a continuation of:
>
>   https://lore.kernel.org/intel-xe/20240208074521.577076-1-lucas.demarchi@intel.com
>
> from Lucas De Marchi. Above series is one year old. I really think
> that this was a good idea and I do not want this series to die. So I
> am volunteering to revive it.

Thanks for picking this up. My acks in the patches still stand.

BR,
Jani.


-- 
Jani Nikula, Intel