dri-devel.lists.freedesktop.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 0/3] Fixed-type GENMASK/BIT
@ 2024-02-08  7:45 Lucas De Marchi
  2024-02-08  7:45 ` [PATCH v3 1/3] bits: introduce fixed-type genmasks Lucas De Marchi
                   ` (2 more replies)
  0 siblings, 3 replies; 32+ messages in thread
From: Lucas De Marchi @ 2024-02-08  7:45 UTC (permalink / raw)
  To: Yury Norov
  Cc: linux-kernel, dri-devel, Andy Shevchenko, Jani Nikula, intel-xe,
	intel-gfx, Lucas De Marchi

ove the implementation of REG_GENMASK/REG_BIT to a more appropriate
place to be shared by i915, xe and possibly other parts of the kernel.

For now this re-defines the old macros. In future we may start using the
new macros directly, but that's a more intrusive search-and-replace.

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.

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

Lucas De Marchi (2):
  bits: Introduce fixed-type BIT
  drm/i915: Convert REG_GENMASK* to fixed-width GENMASK_*

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                 |  51 ++++++++++---
 3 files changed, 51 insertions(+), 109 deletions(-)

-- 
2.43.0


^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH v3 1/3] bits: introduce fixed-type genmasks
  2024-02-08  7:45 [PATCH v3 0/3] Fixed-type GENMASK/BIT Lucas De Marchi
@ 2024-02-08  7:45 ` Lucas De Marchi
  2024-02-08 19:48   ` Andi Shyti
  2024-02-21 20:30   ` Dmitry Baryshkov
  2024-02-08  7:45 ` [PATCH v3 2/3] bits: Introduce fixed-type BIT Lucas De Marchi
  2024-02-08  7:45 ` [PATCH v3 3/3] drm/i915: Convert REG_GENMASK* to fixed-width GENMASK_* Lucas De Marchi
  2 siblings, 2 replies; 32+ messages in thread
From: Lucas De Marchi @ 2024-02-08  7:45 UTC (permalink / raw)
  To: Yury Norov
  Cc: linux-kernel, dri-devel, Andy Shevchenko, Jani Nikula, intel-xe,
	intel-gfx, Lucas De Marchi, Jani Nikula

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

Generalize __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>
---
 include/linux/bitops.h |  1 -
 include/linux/bits.h   | 32 ++++++++++++++++++++++----------
 2 files changed, 22 insertions(+), 11 deletions(-)

diff --git a/include/linux/bitops.h b/include/linux/bitops.h
index 2ba557e067fe..1db50c69cfdb 100644
--- a/include/linux/bitops.h
+++ b/include/linux/bitops.h
@@ -15,7 +15,6 @@
 #  define aligned_byte_mask(n) (~0xffUL << (BITS_PER_LONG - 8 - 8*(n)))
 #endif
 
-#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 7c0cf5031abe..bd56f32de44e 100644
--- a/include/linux/bits.h
+++ b/include/linux/bits.h
@@ -6,6 +6,8 @@
 #include <vdso/bits.h>
 #include <asm/bitsperlong.h>
 
+#define BITS_PER_TYPE(type)	(sizeof(type) * BITS_PER_BYTE)
+
 #define BIT_MASK(nr)		(UL(1) << ((nr) % BITS_PER_LONG))
 #define BIT_WORD(nr)		((nr) / BITS_PER_LONG)
 #define BIT_ULL_MASK(nr)	(ULL(1) << ((nr) % BITS_PER_LONG_LONG))
@@ -30,16 +32,26 @@
 #define GENMASK_INPUT_CHECK(h, l) 0
 #endif
 
-#define __GENMASK(h, l) \
-	(((~UL(0)) - (UL(1) << (l)) + 1) & \
-	 (~UL(0) >> (BITS_PER_LONG - 1 - (h))))
-#define GENMASK(h, l) \
-	(GENMASK_INPUT_CHECK(h, l) + __GENMASK(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, h, l) \
+	(GENMASK_INPUT_CHECK(h, l) + \
+	 (((t)~0ULL - ((t)(1) << (l)) + 1) & \
+	 ((t)~0ULL >> (BITS_PER_TYPE(t) - 1 - (h)))))
 
-#define __GENMASK_ULL(h, l) \
-	(((~ULL(0)) - (ULL(1) << (l)) + 1) & \
-	 (~ULL(0) >> (BITS_PER_LONG_LONG - 1 - (h))))
-#define GENMASK_ULL(h, l) \
-	(GENMASK_INPUT_CHECK(h, l) + __GENMASK_ULL(h, l))
+#define GENMASK(h, l)		__GENMASK(unsigned long,  h, l)
+#define GENMASK_ULL(h, l)	__GENMASK(unsigned long long, h, l)
+#define GENMASK_U8(h, l)	__GENMASK(u8,  h, l)
+#define GENMASK_U16(h, l)	__GENMASK(u16, h, l)
+#define GENMASK_U32(h, l)	__GENMASK(u32, h, l)
+#define GENMASK_U64(h, l)	__GENMASK(u64, h, l)
 
 #endif	/* __LINUX_BITS_H */
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 32+ messages in thread

* [PATCH v3 2/3] bits: Introduce fixed-type BIT
  2024-02-08  7:45 [PATCH v3 0/3] Fixed-type GENMASK/BIT Lucas De Marchi
  2024-02-08  7:45 ` [PATCH v3 1/3] bits: introduce fixed-type genmasks Lucas De Marchi
@ 2024-02-08  7:45 ` Lucas De Marchi
  2024-02-08 20:04   ` Andi Shyti
  2024-02-09 16:53   ` Yury Norov
  2024-02-08  7:45 ` [PATCH v3 3/3] drm/i915: Convert REG_GENMASK* to fixed-width GENMASK_* Lucas De Marchi
  2 siblings, 2 replies; 32+ messages in thread
From: Lucas De Marchi @ 2024-02-08  7:45 UTC (permalink / raw)
  To: Yury Norov
  Cc: linux-kernel, dri-devel, Andy Shevchenko, Jani Nikula, intel-xe,
	intel-gfx, Lucas De Marchi, Jani Nikula

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>
---
 include/linux/bits.h | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/include/linux/bits.h b/include/linux/bits.h
index bd56f32de44e..811846ce110e 100644
--- a/include/linux/bits.h
+++ b/include/linux/bits.h
@@ -24,12 +24,16 @@
 #define GENMASK_INPUT_CHECK(h, l) \
 	(BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
 		__is_constexpr((l) > (h)), (l) > (h), 0)))
+#define BIT_INPUT_CHECK(type, b) \
+	((BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
+		__is_constexpr(b), (b) >= BITS_PER_TYPE(type), 0))))
 #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
+#define BIT_INPUT_CHECK(type, b) 0
 #endif
 
 /*
@@ -54,4 +58,17 @@
 #define GENMASK_U32(h, l)	__GENMASK(u32, h, l)
 #define GENMASK_U64(h, l)	__GENMASK(u64, h, l)
 
+/*
+ * Fixed-type variants of BIT(), with additional checks like __GENMASK().  The
+ * following examples generate compiler warnings due to shift-count-overflow:
+ *
+ * - BIT_U8(8)
+ * - BIT_U32(-1)
+ * - BIT_U32(40)
+ */
+#define BIT_U8(b)		((u8)(BIT_INPUT_CHECK(u8, b) + BIT(b)))
+#define BIT_U16(b)		((u16)(BIT_INPUT_CHECK(u16, b) + BIT(b)))
+#define BIT_U32(b)		((u32)(BIT_INPUT_CHECK(u32, b) + BIT(b)))
+#define BIT_U64(b)		((u64)(BIT_INPUT_CHECK(u64, b) + BIT(b)))
+
 #endif	/* __LINUX_BITS_H */
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 32+ messages in thread

* [PATCH v3 3/3] drm/i915: Convert REG_GENMASK* to fixed-width GENMASK_*
  2024-02-08  7:45 [PATCH v3 0/3] Fixed-type GENMASK/BIT Lucas De Marchi
  2024-02-08  7:45 ` [PATCH v3 1/3] bits: introduce fixed-type genmasks Lucas De Marchi
  2024-02-08  7:45 ` [PATCH v3 2/3] bits: Introduce fixed-type BIT Lucas De Marchi
@ 2024-02-08  7:45 ` Lucas De Marchi
  2024-02-08  8:49   ` Jani Nikula
  2024-02-08 20:07   ` Andi Shyti
  2 siblings, 2 replies; 32+ messages in thread
From: Lucas De Marchi @ 2024-02-08  7:45 UTC (permalink / raw)
  To: Yury Norov
  Cc: linux-kernel, dri-devel, Andy Shevchenko, Jani Nikula, intel-xe,
	intel-gfx, Lucas De Marchi, Jani Nikula

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>
---
 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 a685db1e815d..52f99eb96f86 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.43.0


^ permalink raw reply related	[flat|nested] 32+ messages in thread

* Re: [PATCH v3 3/3] drm/i915: Convert REG_GENMASK* to fixed-width GENMASK_*
  2024-02-08  7:45 ` [PATCH v3 3/3] drm/i915: Convert REG_GENMASK* to fixed-width GENMASK_* Lucas De Marchi
@ 2024-02-08  8:49   ` Jani Nikula
  2024-02-08 20:07   ` Andi Shyti
  1 sibling, 0 replies; 32+ messages in thread
From: Jani Nikula @ 2024-02-08  8:49 UTC (permalink / raw)
  To: Lucas De Marchi, Yury Norov
  Cc: linux-kernel, dri-devel, Andy Shevchenko, intel-xe, intel-gfx,
	Lucas De Marchi

On Wed, 07 Feb 2024, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
> 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>

Thanks for doing this!

This is fine to merge via whichever tree you find best. The i915 parts
have very little conflict potential, there haven't been changes here in
a while.

BR,
Jani.


-- 
Jani Nikula, Intel

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v3 1/3] bits: introduce fixed-type genmasks
  2024-02-08  7:45 ` [PATCH v3 1/3] bits: introduce fixed-type genmasks Lucas De Marchi
@ 2024-02-08 19:48   ` Andi Shyti
  2024-02-09 14:04     ` Andy Shevchenko
  2024-02-21 20:30   ` Dmitry Baryshkov
  1 sibling, 1 reply; 32+ messages in thread
From: Andi Shyti @ 2024-02-08 19:48 UTC (permalink / raw)
  To: Lucas De Marchi
  Cc: Yury Norov, linux-kernel, dri-devel, Andy Shevchenko,
	Jani Nikula, intel-xe, intel-gfx, Jani Nikula

Hi Lucas and Yury,

On Wed, Feb 07, 2024 at 11:45:19PM -0800, Lucas De Marchi wrote:
> From: Yury Norov <yury.norov@gmail.com>
> 
> Generalize __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>

Lucas' SoB should be at the bottom here. In any case, nice patch:

Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com>

Thanks,
Andi

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v3 2/3] bits: Introduce fixed-type BIT
  2024-02-08  7:45 ` [PATCH v3 2/3] bits: Introduce fixed-type BIT Lucas De Marchi
@ 2024-02-08 20:04   ` Andi Shyti
  2024-02-08 21:17     ` Lucas De Marchi
  2024-02-09 16:53   ` Yury Norov
  1 sibling, 1 reply; 32+ messages in thread
From: Andi Shyti @ 2024-02-08 20:04 UTC (permalink / raw)
  To: Lucas De Marchi
  Cc: Yury Norov, linux-kernel, dri-devel, Andy Shevchenko,
	Jani Nikula, intel-xe, intel-gfx, Jani Nikula

Hi Lucas,

looks good, just one idea...

...

> +#define BIT_U8(b)		((u8)(BIT_INPUT_CHECK(u8, b) + BIT(b)))
> +#define BIT_U16(b)		((u16)(BIT_INPUT_CHECK(u16, b) + BIT(b)))
> +#define BIT_U32(b)		((u32)(BIT_INPUT_CHECK(u32, b) + BIT(b)))
> +#define BIT_U64(b)		((u64)(BIT_INPUT_CHECK(u64, b) + BIT(b)))

considering that BIT defines are always referred to unsigned
types, I would just call them

#define BIT8
#define BIT16
#define BIT32
#define BIT64

what do you think?

Andi

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v3 3/3] drm/i915: Convert REG_GENMASK* to fixed-width GENMASK_*
  2024-02-08  7:45 ` [PATCH v3 3/3] drm/i915: Convert REG_GENMASK* to fixed-width GENMASK_* Lucas De Marchi
  2024-02-08  8:49   ` Jani Nikula
@ 2024-02-08 20:07   ` Andi Shyti
  2024-02-08 21:21     ` Lucas De Marchi
  1 sibling, 1 reply; 32+ messages in thread
From: Andi Shyti @ 2024-02-08 20:07 UTC (permalink / raw)
  To: Lucas De Marchi
  Cc: Yury Norov, linux-kernel, dri-devel, Andy Shevchenko,
	Jani Nikula, intel-xe, intel-gfx, Jani Nikula

Hi Lucas,

...

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

I was hoping to use directly GENMASK_U*() functions.

Andi

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: Re: [PATCH v3 2/3] bits: Introduce fixed-type BIT
  2024-02-08 20:04   ` Andi Shyti
@ 2024-02-08 21:17     ` Lucas De Marchi
  2024-02-09  8:01       ` Jani Nikula
  2024-02-10 14:22       ` David Laight
  0 siblings, 2 replies; 32+ messages in thread
From: Lucas De Marchi @ 2024-02-08 21:17 UTC (permalink / raw)
  To: Andi Shyti
  Cc: Yury Norov, linux-kernel, dri-devel, Andy Shevchenko,
	Jani Nikula, intel-xe, intel-gfx, Jani Nikula

On Thu, Feb 08, 2024 at 09:04:45PM +0100, Andi Shyti wrote:
>Hi Lucas,
>
>looks good, just one idea...
>
>...
>
>> +#define BIT_U8(b)		((u8)(BIT_INPUT_CHECK(u8, b) + BIT(b)))
>> +#define BIT_U16(b)		((u16)(BIT_INPUT_CHECK(u16, b) + BIT(b)))
>> +#define BIT_U32(b)		((u32)(BIT_INPUT_CHECK(u32, b) + BIT(b)))
>> +#define BIT_U64(b)		((u64)(BIT_INPUT_CHECK(u64, b) + BIT(b)))
>
>considering that BIT defines are always referred to unsigned
>types, I would just call them
>
>#define BIT8
>#define BIT16
>#define BIT32
>#define BIT64
>
>what do you think?

it will clash with defines from other headers and not match the ones for
GENMASK  so I prefer it the other way.

thanks
Lucas De Marchi

>
>Andi

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: Re: [PATCH v3 3/3] drm/i915: Convert REG_GENMASK* to fixed-width GENMASK_*
  2024-02-08 20:07   ` Andi Shyti
@ 2024-02-08 21:21     ` Lucas De Marchi
  0 siblings, 0 replies; 32+ messages in thread
From: Lucas De Marchi @ 2024-02-08 21:21 UTC (permalink / raw)
  To: Andi Shyti
  Cc: Yury Norov, linux-kernel, dri-devel, Andy Shevchenko,
	Jani Nikula, intel-xe, intel-gfx, Jani Nikula

On Thu, Feb 08, 2024 at 09:07:32PM +0100, Andi Shyti wrote:
>Hi Lucas,
>
>...
>
>> +#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)
>
>I was hoping to use directly GENMASK_U*() functions.

this would be something to be done on top on each drivers' trees, not
something to apply here.

Lucas De Marchi

>
>Andi

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: Re: [PATCH v3 2/3] bits: Introduce fixed-type BIT
  2024-02-08 21:17     ` Lucas De Marchi
@ 2024-02-09  8:01       ` Jani Nikula
  2024-02-10 14:22       ` David Laight
  1 sibling, 0 replies; 32+ messages in thread
From: Jani Nikula @ 2024-02-09  8:01 UTC (permalink / raw)
  To: Lucas De Marchi, Andi Shyti
  Cc: Yury Norov, linux-kernel, dri-devel, Andy Shevchenko, intel-xe,
	intel-gfx

On Thu, 08 Feb 2024, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
> On Thu, Feb 08, 2024 at 09:04:45PM +0100, Andi Shyti wrote:
>>Hi Lucas,
>>
>>looks good, just one idea...
>>
>>...
>>
>>> +#define BIT_U8(b)		((u8)(BIT_INPUT_CHECK(u8, b) + BIT(b)))
>>> +#define BIT_U16(b)		((u16)(BIT_INPUT_CHECK(u16, b) + BIT(b)))
>>> +#define BIT_U32(b)		((u32)(BIT_INPUT_CHECK(u32, b) + BIT(b)))
>>> +#define BIT_U64(b)		((u64)(BIT_INPUT_CHECK(u64, b) + BIT(b)))
>>
>>considering that BIT defines are always referred to unsigned
>>types, I would just call them
>>
>>#define BIT8
>>#define BIT16
>>#define BIT32
>>#define BIT64
>>
>>what do you think?
>
> it will clash with defines from other headers and not match the ones for
> GENMASK  so I prefer it the other way.

Agreed.


-- 
Jani Nikula, Intel

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v3 1/3] bits: introduce fixed-type genmasks
  2024-02-08 19:48   ` Andi Shyti
@ 2024-02-09 14:04     ` Andy Shevchenko
  0 siblings, 0 replies; 32+ messages in thread
From: Andy Shevchenko @ 2024-02-09 14:04 UTC (permalink / raw)
  To: Andi Shyti
  Cc: Lucas De Marchi, Yury Norov, linux-kernel, dri-devel,
	Jani Nikula, intel-xe, intel-gfx, Jani Nikula

On Thu, Feb 08, 2024 at 08:48:59PM +0100, Andi Shyti wrote:
> On Wed, Feb 07, 2024 at 11:45:19PM -0800, Lucas De Marchi wrote:

...

> > 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>
> 
> Lucas' SoB should be at the bottom here. In any case, nice patch:

And it's at the bottom (among SoB lines), there is no violation with Submitting
Patches.

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v3 2/3] bits: Introduce fixed-type BIT
  2024-02-08  7:45 ` [PATCH v3 2/3] bits: Introduce fixed-type BIT Lucas De Marchi
  2024-02-08 20:04   ` Andi Shyti
@ 2024-02-09 16:53   ` Yury Norov
  2024-02-20  5:13     ` Lucas De Marchi
  1 sibling, 1 reply; 32+ messages in thread
From: Yury Norov @ 2024-02-09 16:53 UTC (permalink / raw)
  To: Lucas De Marchi
  Cc: linux-kernel, dri-devel, Andy Shevchenko, Jani Nikula, intel-xe,
	intel-gfx, Jani Nikula

On Wed, Feb 07, 2024 at 11:45:20PM -0800, Lucas De Marchi wrote:
> 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>

So I get v1 from Jan.23 in my mailbox, and this one is v3. Did I miss
a v2? Anyways, please bear my reviewed-by from v1 for this patch.

Thanks,
Yury

> ---
>  include/linux/bits.h | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/include/linux/bits.h b/include/linux/bits.h
> index bd56f32de44e..811846ce110e 100644
> --- a/include/linux/bits.h
> +++ b/include/linux/bits.h
> @@ -24,12 +24,16 @@
>  #define GENMASK_INPUT_CHECK(h, l) \
>  	(BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
>  		__is_constexpr((l) > (h)), (l) > (h), 0)))
> +#define BIT_INPUT_CHECK(type, b) \
> +	((BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
> +		__is_constexpr(b), (b) >= BITS_PER_TYPE(type), 0))))
>  #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
> +#define BIT_INPUT_CHECK(type, b) 0
>  #endif
>  
>  /*
> @@ -54,4 +58,17 @@
>  #define GENMASK_U32(h, l)	__GENMASK(u32, h, l)
>  #define GENMASK_U64(h, l)	__GENMASK(u64, h, l)
>  
> +/*
> + * Fixed-type variants of BIT(), with additional checks like __GENMASK().  The
> + * following examples generate compiler warnings due to shift-count-overflow:
> + *
> + * - BIT_U8(8)
> + * - BIT_U32(-1)
> + * - BIT_U32(40)
> + */
> +#define BIT_U8(b)		((u8)(BIT_INPUT_CHECK(u8, b) + BIT(b)))
> +#define BIT_U16(b)		((u16)(BIT_INPUT_CHECK(u16, b) + BIT(b)))
> +#define BIT_U32(b)		((u32)(BIT_INPUT_CHECK(u32, b) + BIT(b)))
> +#define BIT_U64(b)		((u64)(BIT_INPUT_CHECK(u64, b) + BIT(b)))
> +
>  #endif	/* __LINUX_BITS_H */
> -- 
> 2.43.0

^ permalink raw reply	[flat|nested] 32+ messages in thread

* RE: Re: [PATCH v3 2/3] bits: Introduce fixed-type BIT
  2024-02-08 21:17     ` Lucas De Marchi
  2024-02-09  8:01       ` Jani Nikula
@ 2024-02-10 14:22       ` David Laight
  1 sibling, 0 replies; 32+ messages in thread
From: David Laight @ 2024-02-10 14:22 UTC (permalink / raw)
  To: 'Lucas De Marchi', Andi Shyti
  Cc: Yury Norov, linux-kernel, dri-devel, Andy Shevchenko,
	Jani Nikula, intel-xe, intel-gfx, Jani Nikula

...
> >> +#define BIT_U8(b)		((u8)(BIT_INPUT_CHECK(u8, b) + BIT(b)))
> >> +#define BIT_U16(b)		((u16)(BIT_INPUT_CHECK(u16, b) + BIT(b)))
> >> +#define BIT_U32(b)		((u32)(BIT_INPUT_CHECK(u32, b) + BIT(b)))
> >> +#define BIT_U64(b)		((u64)(BIT_INPUT_CHECK(u64, b) + BIT(b)))
> >
> >considering that BIT defines are always referred to unsigned
> >types, I would just call them

Except that pretty much as soon as you breath on them
the u8 and u16 types get converted to int.
If you want them to be an unsigned type then you need
to cast them to (unsigned int).

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)


^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: Re: [PATCH v3 2/3] bits: Introduce fixed-type BIT
  2024-02-09 16:53   ` Yury Norov
@ 2024-02-20  5:13     ` Lucas De Marchi
  2024-02-22 14:51       ` Yury Norov
  0 siblings, 1 reply; 32+ messages in thread
From: Lucas De Marchi @ 2024-02-20  5:13 UTC (permalink / raw)
  To: Yury Norov
  Cc: linux-kernel, dri-devel, Andy Shevchenko, Jani Nikula, intel-xe,
	intel-gfx, Jani Nikula

On Fri, Feb 09, 2024 at 08:53:25AM -0800, Yury Norov wrote:
>On Wed, Feb 07, 2024 at 11:45:20PM -0800, Lucas De Marchi wrote:
>> 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>
>
>So I get v1 from Jan.23 in my mailbox, and this one is v3. Did I miss
>a v2? Anyways, please bear my reviewed-by from v1 for this patch.

Jan 23 was actually the v2 and I missed the subject prefix.

My understanding was that you were going to apply this through some
bitmap tree, but checking MAINTAINERS now it seems there's no git tree
associated.  So I will just add your r-b and merge this through
drm-xe.

thanks
Lucas De Marchi

>
>Thanks,
>Yury
>
>> ---
>>  include/linux/bits.h | 17 +++++++++++++++++
>>  1 file changed, 17 insertions(+)
>>
>> diff --git a/include/linux/bits.h b/include/linux/bits.h
>> index bd56f32de44e..811846ce110e 100644
>> --- a/include/linux/bits.h
>> +++ b/include/linux/bits.h
>> @@ -24,12 +24,16 @@
>>  #define GENMASK_INPUT_CHECK(h, l) \
>>  	(BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
>>  		__is_constexpr((l) > (h)), (l) > (h), 0)))
>> +#define BIT_INPUT_CHECK(type, b) \
>> +	((BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
>> +		__is_constexpr(b), (b) >= BITS_PER_TYPE(type), 0))))
>>  #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
>> +#define BIT_INPUT_CHECK(type, b) 0
>>  #endif
>>
>>  /*
>> @@ -54,4 +58,17 @@
>>  #define GENMASK_U32(h, l)	__GENMASK(u32, h, l)
>>  #define GENMASK_U64(h, l)	__GENMASK(u64, h, l)
>>
>> +/*
>> + * Fixed-type variants of BIT(), with additional checks like __GENMASK().  The
>> + * following examples generate compiler warnings due to shift-count-overflow:
>> + *
>> + * - BIT_U8(8)
>> + * - BIT_U32(-1)
>> + * - BIT_U32(40)
>> + */
>> +#define BIT_U8(b)		((u8)(BIT_INPUT_CHECK(u8, b) + BIT(b)))
>> +#define BIT_U16(b)		((u16)(BIT_INPUT_CHECK(u16, b) + BIT(b)))
>> +#define BIT_U32(b)		((u32)(BIT_INPUT_CHECK(u32, b) + BIT(b)))
>> +#define BIT_U64(b)		((u64)(BIT_INPUT_CHECK(u64, b) + BIT(b)))
>> +
>>  #endif	/* __LINUX_BITS_H */
>> --
>> 2.43.0

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v3 1/3] bits: introduce fixed-type genmasks
  2024-02-08  7:45 ` [PATCH v3 1/3] bits: introduce fixed-type genmasks Lucas De Marchi
  2024-02-08 19:48   ` Andi Shyti
@ 2024-02-21 20:30   ` Dmitry Baryshkov
  2024-02-21 20:37     ` Dmitry Baryshkov
  2024-02-21 21:04     ` Andy Shevchenko
  1 sibling, 2 replies; 32+ messages in thread
From: Dmitry Baryshkov @ 2024-02-21 20:30 UTC (permalink / raw)
  To: Lucas De Marchi
  Cc: Yury Norov, linux-kernel, dri-devel, Andy Shevchenko,
	Jani Nikula, intel-xe, intel-gfx, Jani Nikula

On Thu, 8 Feb 2024 at 09:45, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
>
> From: Yury Norov <yury.norov@gmail.com>
>
> Generalize __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>
> ---
>  include/linux/bitops.h |  1 -
>  include/linux/bits.h   | 32 ++++++++++++++++++++++----------
>  2 files changed, 22 insertions(+), 11 deletions(-)
>
> diff --git a/include/linux/bitops.h b/include/linux/bitops.h
> index 2ba557e067fe..1db50c69cfdb 100644
> --- a/include/linux/bitops.h
> +++ b/include/linux/bitops.h
> @@ -15,7 +15,6 @@
>  #  define aligned_byte_mask(n) (~0xffUL << (BITS_PER_LONG - 8 - 8*(n)))
>  #endif
>
> -#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 7c0cf5031abe..bd56f32de44e 100644
> --- a/include/linux/bits.h
> +++ b/include/linux/bits.h
> @@ -6,6 +6,8 @@
>  #include <vdso/bits.h>
>  #include <asm/bitsperlong.h>
>
> +#define BITS_PER_TYPE(type)    (sizeof(type) * BITS_PER_BYTE)
> +
>  #define BIT_MASK(nr)           (UL(1) << ((nr) % BITS_PER_LONG))
>  #define BIT_WORD(nr)           ((nr) / BITS_PER_LONG)
>  #define BIT_ULL_MASK(nr)       (ULL(1) << ((nr) % BITS_PER_LONG_LONG))
> @@ -30,16 +32,26 @@
>  #define GENMASK_INPUT_CHECK(h, l) 0
>  #endif
>
> -#define __GENMASK(h, l) \
> -       (((~UL(0)) - (UL(1) << (l)) + 1) & \
> -        (~UL(0) >> (BITS_PER_LONG - 1 - (h))))
> -#define GENMASK(h, l) \
> -       (GENMASK_INPUT_CHECK(h, l) + __GENMASK(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, h, l) \
> +       (GENMASK_INPUT_CHECK(h, l) + \
> +        (((t)~0ULL - ((t)(1) << (l)) + 1) & \
> +        ((t)~0ULL >> (BITS_PER_TYPE(t) - 1 - (h)))))
>
> -#define __GENMASK_ULL(h, l) \
> -       (((~ULL(0)) - (ULL(1) << (l)) + 1) & \
> -        (~ULL(0) >> (BITS_PER_LONG_LONG - 1 - (h))))
> -#define GENMASK_ULL(h, l) \
> -       (GENMASK_INPUT_CHECK(h, l) + __GENMASK_ULL(h, l))
> +#define GENMASK(h, l)          __GENMASK(unsigned long,  h, l)
> +#define GENMASK_ULL(h, l)      __GENMASK(unsigned long long, h, l)
> +#define GENMASK_U8(h, l)       __GENMASK(u8,  h, l)
> +#define GENMASK_U16(h, l)      __GENMASK(u16, h, l)
> +#define GENMASK_U32(h, l)      __GENMASK(u32, h, l)
> +#define GENMASK_U64(h, l)      __GENMASK(u64, h, l)

This breaks drm-tip on arm64 architecture:

arch/arm64/kernel/entry-fpsimd.S: Assembler messages:
465arch/arm64/kernel/entry-fpsimd.S:271: Error: found 'l', expected: ')'
466arch/arm64/kernel/entry-fpsimd.S:66: Info: macro invoked from here
467arch/arm64/kernel/entry-fpsimd.S:271: Error: found 'l', expected: ')'
468arch/arm64/kernel/entry-fpsimd.S:66: Info: macro invoked from here
469arch/arm64/kernel/entry-fpsimd.S:271: Error: found 'l', expected: ')'
470arch/arm64/kernel/entry-fpsimd.S:66: Info: macro invoked from here
471arch/arm64/kernel/entry-fpsimd.S:271: Error: found 'l', expected: ')'
472arch/arm64/kernel/entry-fpsimd.S:66: Info: macro invoked from here
473arch/arm64/kernel/entry-fpsimd.S:271: Error: unexpected characters
following instruction at operand 3 -- `bic x2,x1,(0+(((unsigned
long)~0ULL-((unsigned long)(1)<<(0))+1)&((unsigned
long)~0ULL>>((sizeof(unsigned long)*8)-1-(3)))))'
474arch/arm64/kernel/entry-fpsimd.S:66: Info: macro invoked from here
475arch/arm64/kernel/entry-fpsimd.S:282: Error: found 'l', expected: ')'
476arch/arm64/kernel/entry-fpsimd.S:98: Info: macro invoked from here
477arch/arm64/kernel/entry-fpsimd.S:282: Error: found 'l', expected: ')'
478arch/arm64/kernel/entry-fpsimd.S:98: Info: macro invoked from here
479arch/arm64/kernel/entry-fpsimd.S:282: Error: found 'l', expected: ')'
480arch/arm64/kernel/entry-fpsimd.S:98: Info: macro invoked from here
481arch/arm64/kernel/entry-fpsimd.S:282: Error: found 'l', expected: ')'
482arch/arm64/kernel/entry-fpsimd.S:98: Info: macro invoked from here
483arch/arm64/kernel/entry-fpsimd.S:282: Error: unexpected characters
following instruction at operand 3 -- `bic x2,x1,(0+(((unsigned
long)~0ULL-((unsigned long)(1)<<(0))+1)&((unsigned
long)~0ULL>>((sizeof(unsigned long)*8)-1-(3)))))'
484arch/arm64/kernel/entry-fpsimd.S:98: Info: macro invoked from here

>
>  #endif /* __LINUX_BITS_H */
> --
> 2.43.0
>


-- 
With best wishes
Dmitry

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v3 1/3] bits: introduce fixed-type genmasks
  2024-02-21 20:30   ` Dmitry Baryshkov
@ 2024-02-21 20:37     ` Dmitry Baryshkov
  2024-02-21 21:06       ` Andy Shevchenko
  2024-02-21 21:04     ` Andy Shevchenko
  1 sibling, 1 reply; 32+ messages in thread
From: Dmitry Baryshkov @ 2024-02-21 20:37 UTC (permalink / raw)
  To: Lucas De Marchi
  Cc: Yury Norov, linux-kernel, dri-devel, Andy Shevchenko,
	Jani Nikula, intel-xe, intel-gfx, Jani Nikula

On Wed, 21 Feb 2024 at 22:30, Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> On Thu, 8 Feb 2024 at 09:45, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
> >
> > From: Yury Norov <yury.norov@gmail.com>
> >
> > Generalize __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>
> > ---
> >  include/linux/bitops.h |  1 -
> >  include/linux/bits.h   | 32 ++++++++++++++++++++++----------
> >  2 files changed, 22 insertions(+), 11 deletions(-)
> >
> > diff --git a/include/linux/bitops.h b/include/linux/bitops.h
> > index 2ba557e067fe..1db50c69cfdb 100644
> > --- a/include/linux/bitops.h
> > +++ b/include/linux/bitops.h
> > @@ -15,7 +15,6 @@
> >  #  define aligned_byte_mask(n) (~0xffUL << (BITS_PER_LONG - 8 - 8*(n)))
> >  #endif
> >
> > -#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 7c0cf5031abe..bd56f32de44e 100644
> > --- a/include/linux/bits.h
> > +++ b/include/linux/bits.h
> > @@ -6,6 +6,8 @@
> >  #include <vdso/bits.h>
> >  #include <asm/bitsperlong.h>
> >
> > +#define BITS_PER_TYPE(type)    (sizeof(type) * BITS_PER_BYTE)
> > +
> >  #define BIT_MASK(nr)           (UL(1) << ((nr) % BITS_PER_LONG))
> >  #define BIT_WORD(nr)           ((nr) / BITS_PER_LONG)
> >  #define BIT_ULL_MASK(nr)       (ULL(1) << ((nr) % BITS_PER_LONG_LONG))
> > @@ -30,16 +32,26 @@
> >  #define GENMASK_INPUT_CHECK(h, l) 0
> >  #endif
> >
> > -#define __GENMASK(h, l) \
> > -       (((~UL(0)) - (UL(1) << (l)) + 1) & \
> > -        (~UL(0) >> (BITS_PER_LONG - 1 - (h))))
> > -#define GENMASK(h, l) \
> > -       (GENMASK_INPUT_CHECK(h, l) + __GENMASK(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, h, l) \
> > +       (GENMASK_INPUT_CHECK(h, l) + \
> > +        (((t)~0ULL - ((t)(1) << (l)) + 1) & \
> > +        ((t)~0ULL >> (BITS_PER_TYPE(t) - 1 - (h)))))
> >
> > -#define __GENMASK_ULL(h, l) \
> > -       (((~ULL(0)) - (ULL(1) << (l)) + 1) & \
> > -        (~ULL(0) >> (BITS_PER_LONG_LONG - 1 - (h))))
> > -#define GENMASK_ULL(h, l) \
> > -       (GENMASK_INPUT_CHECK(h, l) + __GENMASK_ULL(h, l))
> > +#define GENMASK(h, l)          __GENMASK(unsigned long,  h, l)
> > +#define GENMASK_ULL(h, l)      __GENMASK(unsigned long long, h, l)
> > +#define GENMASK_U8(h, l)       __GENMASK(u8,  h, l)
> > +#define GENMASK_U16(h, l)      __GENMASK(u16, h, l)
> > +#define GENMASK_U32(h, l)      __GENMASK(u32, h, l)
> > +#define GENMASK_U64(h, l)      __GENMASK(u64, h, l)
>
> This breaks drm-tip on arm64 architecture:
>

Excuse me, it seems a c&p from gitlab didn't work as expected.

arch/arm64/kernel/entry-fpsimd.S: Assembler messages:
arch/arm64/kernel/entry-fpsimd.S:271: Error: found 'l', expected: ')'
arch/arm64/kernel/entry-fpsimd.S:66:  Info: macro invoked from here
arch/arm64/kernel/entry-fpsimd.S:271: Error: found 'l', expected: ')'
arch/arm64/kernel/entry-fpsimd.S:66:  Info: macro invoked from here
arch/arm64/kernel/entry-fpsimd.S:271: Error: found 'l', expected: ')'
arch/arm64/kernel/entry-fpsimd.S:66:  Info: macro invoked from here
arch/arm64/kernel/entry-fpsimd.S:271: Error: found 'l', expected: ')'
arch/arm64/kernel/entry-fpsimd.S:66:  Info: macro invoked from here
arch/arm64/kernel/entry-fpsimd.S:271: Error: unexpected characters
following instruction at operand 3 -- `bic x2,x1,(0+(((unsigned
long)~0ULL-((unsigned long)(1)<<(0))+1)&((unsigned
long)~0ULL>>((sizeof(unsigned long)*8)-1-(3)))))'
arch/arm64/kernel/entry-fpsimd.S:66:  Info: macro invoked from here
arch/arm64/kernel/entry-fpsimd.S:282: Error: found 'l', expected: ')'
arch/arm64/kernel/entry-fpsimd.S:98:  Info: macro invoked from here
arch/arm64/kernel/entry-fpsimd.S:282: Error: found 'l', expected: ')'
arch/arm64/kernel/entry-fpsimd.S:98:  Info: macro invoked from here
arch/arm64/kernel/entry-fpsimd.S:282: Error: found 'l', expected: ')'
arch/arm64/kernel/entry-fpsimd.S:98:  Info: macro invoked from here
arch/arm64/kernel/entry-fpsimd.S:282: Error: found 'l', expected: ')'
arch/arm64/kernel/entry-fpsimd.S:98:  Info: macro invoked from here
arch/arm64/kernel/entry-fpsimd.S:282: Error: unexpected characters
following instruction at operand 3 -- `bic x2,x1,(0+(((unsigned
long)~0ULL-((unsigned long)(1)<<(0))+1)&((unsigned
long)~0ULL>>((sizeof(unsigned long)*8)-1-(3)))))'
arch/arm64/kernel/entry-fpsimd.S:98:  Info: macro invoked from here
make[4]: *** [scripts/Makefile.build:361:
arch/arm64/kernel/entry-fpsimd.o] Error 1
make[3]: *** [scripts/Makefile.build:481: arch/arm64/kernel] Error 2
make[2]: *** [scripts/Makefile.build:481: arch/arm64] Error 2
make[2]: *** Waiting for unfinished jobs...


-- 
With best wishes
Dmitry

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v3 1/3] bits: introduce fixed-type genmasks
  2024-02-21 20:30   ` Dmitry Baryshkov
  2024-02-21 20:37     ` Dmitry Baryshkov
@ 2024-02-21 21:04     ` Andy Shevchenko
  2024-02-21 21:59       ` Lucas De Marchi
  1 sibling, 1 reply; 32+ messages in thread
From: Andy Shevchenko @ 2024-02-21 21:04 UTC (permalink / raw)
  To: Dmitry Baryshkov
  Cc: Lucas De Marchi, Yury Norov, linux-kernel, dri-devel,
	Jani Nikula, intel-xe, intel-gfx, Jani Nikula

On Wed, Feb 21, 2024 at 10:30:02PM +0200, Dmitry Baryshkov wrote:
> On Thu, 8 Feb 2024 at 09:45, Lucas De Marchi <lucas.demarchi@intel.com> wrote:

...

> > +#define BITS_PER_TYPE(type)    (sizeof(type) * BITS_PER_BYTE)

Can sizeof() be used in assembly?

...

> > -#define __GENMASK(h, l) \
> > -       (((~UL(0)) - (UL(1) << (l)) + 1) & \
> > -        (~UL(0) >> (BITS_PER_LONG - 1 - (h))))
> > -#define GENMASK(h, l) \
> > -       (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))

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

Nevertheless, the use ~0ULL is not proper assembly, this broke initial
implementation using UL() / ULL().


> > -#define __GENMASK_ULL(h, l) \
> > -       (((~ULL(0)) - (ULL(1) << (l)) + 1) & \
> > -        (~ULL(0) >> (BITS_PER_LONG_LONG - 1 - (h))))
> > -#define GENMASK_ULL(h, l) \
> > -       (GENMASK_INPUT_CHECK(h, l) + __GENMASK_ULL(h, l))

Ditto.

> > +#define GENMASK(h, l)          __GENMASK(unsigned long,  h, l)
> > +#define GENMASK_ULL(h, l)      __GENMASK(unsigned long long, h, l)
> > +#define GENMASK_U8(h, l)       __GENMASK(u8,  h, l)
> > +#define GENMASK_U16(h, l)      __GENMASK(u16, h, l)
> > +#define GENMASK_U32(h, l)      __GENMASK(u32, h, l)
> > +#define GENMASK_U64(h, l)      __GENMASK(u64, h, l)
> 
> This breaks drm-tip on arm64 architecture:
> 
> arch/arm64/kernel/entry-fpsimd.S: Assembler messages:
> 465arch/arm64/kernel/entry-fpsimd.S:271: Error: found 'l', expected: ')'
> 466arch/arm64/kernel/entry-fpsimd.S:66: Info: macro invoked from here
> 467arch/arm64/kernel/entry-fpsimd.S:271: Error: found 'l', expected: ')'
> 468arch/arm64/kernel/entry-fpsimd.S:66: Info: macro invoked from here
> 469arch/arm64/kernel/entry-fpsimd.S:271: Error: found 'l', expected: ')'
> 470arch/arm64/kernel/entry-fpsimd.S:66: Info: macro invoked from here
> 471arch/arm64/kernel/entry-fpsimd.S:271: Error: found 'l', expected: ')'
> 472arch/arm64/kernel/entry-fpsimd.S:66: Info: macro invoked from here
> 473arch/arm64/kernel/entry-fpsimd.S:271: Error: unexpected characters
> following instruction at operand 3 -- `bic x2,x1,(0+(((unsigned
> long)~0ULL-((unsigned long)(1)<<(0))+1)&((unsigned
> long)~0ULL>>((sizeof(unsigned long)*8)-1-(3)))))'
> 474arch/arm64/kernel/entry-fpsimd.S:66: Info: macro invoked from here
> 475arch/arm64/kernel/entry-fpsimd.S:282: Error: found 'l', expected: ')'
> 476arch/arm64/kernel/entry-fpsimd.S:98: Info: macro invoked from here
> 477arch/arm64/kernel/entry-fpsimd.S:282: Error: found 'l', expected: ')'
> 478arch/arm64/kernel/entry-fpsimd.S:98: Info: macro invoked from here
> 479arch/arm64/kernel/entry-fpsimd.S:282: Error: found 'l', expected: ')'
> 480arch/arm64/kernel/entry-fpsimd.S:98: Info: macro invoked from here
> 481arch/arm64/kernel/entry-fpsimd.S:282: Error: found 'l', expected: ')'
> 482arch/arm64/kernel/entry-fpsimd.S:98: Info: macro invoked from here
> 483arch/arm64/kernel/entry-fpsimd.S:282: Error: unexpected characters
> following instruction at operand 3 -- `bic x2,x1,(0+(((unsigned
> long)~0ULL-((unsigned long)(1)<<(0))+1)&((unsigned
> long)~0ULL>>((sizeof(unsigned long)*8)-1-(3)))))'
> 484arch/arm64/kernel/entry-fpsimd.S:98: Info: macro invoked from here

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v3 1/3] bits: introduce fixed-type genmasks
  2024-02-21 20:37     ` Dmitry Baryshkov
@ 2024-02-21 21:06       ` Andy Shevchenko
  2024-02-21 21:07         ` Andy Shevchenko
  0 siblings, 1 reply; 32+ messages in thread
From: Andy Shevchenko @ 2024-02-21 21:06 UTC (permalink / raw)
  To: Dmitry Baryshkov
  Cc: Lucas De Marchi, Yury Norov, linux-kernel, dri-devel,
	Jani Nikula, intel-xe, intel-gfx, Jani Nikula

On Wed, Feb 21, 2024 at 10:37:30PM +0200, Dmitry Baryshkov wrote:
> On Wed, 21 Feb 2024 at 22:30, Dmitry Baryshkov
> <dmitry.baryshkov@linaro.org> wrote:

...

> Excuse me, it seems a c&p from gitlab didn't work as expected.

No problem, it's clear that the patch has not been properly tested and
obviously wrong. Has to be dropped. If somebody wants that, it will
be material for v6.10 cycle.

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v3 1/3] bits: introduce fixed-type genmasks
  2024-02-21 21:06       ` Andy Shevchenko
@ 2024-02-21 21:07         ` Andy Shevchenko
  0 siblings, 0 replies; 32+ messages in thread
From: Andy Shevchenko @ 2024-02-21 21:07 UTC (permalink / raw)
  To: Dmitry Baryshkov
  Cc: Lucas De Marchi, Yury Norov, linux-kernel, dri-devel,
	Jani Nikula, intel-xe, intel-gfx, Jani Nikula

On Wed, Feb 21, 2024 at 11:06:10PM +0200, Andy Shevchenko wrote:
> On Wed, Feb 21, 2024 at 10:37:30PM +0200, Dmitry Baryshkov wrote:
> > On Wed, 21 Feb 2024 at 22:30, Dmitry Baryshkov
> > <dmitry.baryshkov@linaro.org> wrote:

...

> > Excuse me, it seems a c&p from gitlab didn't work as expected.
> 
> No problem, it's clear that the patch has not been properly tested and
> obviously wrong. Has to be dropped. If somebody wants that, it will
> be material for v6.10 cycle.

...which makes me think that bitmap(bitops) lack of assembly test case(s).

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: Re: [PATCH v3 1/3] bits: introduce fixed-type genmasks
  2024-02-21 21:04     ` Andy Shevchenko
@ 2024-02-21 21:59       ` Lucas De Marchi
  2024-02-22 14:49         ` Yury Norov
  0 siblings, 1 reply; 32+ messages in thread
From: Lucas De Marchi @ 2024-02-21 21:59 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Dmitry Baryshkov, Yury Norov, linux-kernel, dri-devel,
	Jani Nikula, intel-xe, intel-gfx, Jani Nikula

On Wed, Feb 21, 2024 at 11:04:22PM +0200, Andy Shevchenko wrote:
>On Wed, Feb 21, 2024 at 10:30:02PM +0200, Dmitry Baryshkov wrote:
>> On Thu, 8 Feb 2024 at 09:45, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
>
>...
>
>> > +#define BITS_PER_TYPE(type)    (sizeof(type) * BITS_PER_BYTE)
>
>Can sizeof() be used in assembly?
>
>...
>
>> > -#define __GENMASK(h, l) \
>> > -       (((~UL(0)) - (UL(1) << (l)) + 1) & \
>> > -        (~UL(0) >> (BITS_PER_LONG - 1 - (h))))
>> > -#define GENMASK(h, l) \
>> > -       (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
>
>> > +#define __GENMASK(t, h, l) \
>> > +       (GENMASK_INPUT_CHECK(h, l) + \
>> > +        (((t)~0ULL - ((t)(1) << (l)) + 1) & \
>> > +        ((t)~0ULL >> (BITS_PER_TYPE(t) - 1 - (h)))))
>
>Nevertheless, the use ~0ULL is not proper assembly, this broke initial
>implementation using UL() / ULL().

indeed.

>
>
>> > -#define __GENMASK_ULL(h, l) \
>> > -       (((~ULL(0)) - (ULL(1) << (l)) + 1) & \
>> > -        (~ULL(0) >> (BITS_PER_LONG_LONG - 1 - (h))))
>> > -#define GENMASK_ULL(h, l) \
>> > -       (GENMASK_INPUT_CHECK(h, l) + __GENMASK_ULL(h, l))
>
>Ditto.

problem here seems actually because of the cast to the final type. My
previous impl was avoiding that, but was too verbose compared to this.

I will look at reverting this.

Lucas De Marchi

>
>> > +#define GENMASK(h, l)          __GENMASK(unsigned long,  h, l)
>> > +#define GENMASK_ULL(h, l)      __GENMASK(unsigned long long, h, l)
>> > +#define GENMASK_U8(h, l)       __GENMASK(u8,  h, l)
>> > +#define GENMASK_U16(h, l)      __GENMASK(u16, h, l)
>> > +#define GENMASK_U32(h, l)      __GENMASK(u32, h, l)
>> > +#define GENMASK_U64(h, l)      __GENMASK(u64, h, l)
>>
>> This breaks drm-tip on arm64 architecture:
>>
>> arch/arm64/kernel/entry-fpsimd.S: Assembler messages:
>> 465arch/arm64/kernel/entry-fpsimd.S:271: Error: found 'l', expected: ')'
>> 466arch/arm64/kernel/entry-fpsimd.S:66: Info: macro invoked from here
>> 467arch/arm64/kernel/entry-fpsimd.S:271: Error: found 'l', expected: ')'
>> 468arch/arm64/kernel/entry-fpsimd.S:66: Info: macro invoked from here
>> 469arch/arm64/kernel/entry-fpsimd.S:271: Error: found 'l', expected: ')'
>> 470arch/arm64/kernel/entry-fpsimd.S:66: Info: macro invoked from here
>> 471arch/arm64/kernel/entry-fpsimd.S:271: Error: found 'l', expected: ')'
>> 472arch/arm64/kernel/entry-fpsimd.S:66: Info: macro invoked from here
>> 473arch/arm64/kernel/entry-fpsimd.S:271: Error: unexpected characters
>> following instruction at operand 3 -- `bic x2,x1,(0+(((unsigned
>> long)~0ULL-((unsigned long)(1)<<(0))+1)&((unsigned
>> long)~0ULL>>((sizeof(unsigned long)*8)-1-(3)))))'
>> 474arch/arm64/kernel/entry-fpsimd.S:66: Info: macro invoked from here
>> 475arch/arm64/kernel/entry-fpsimd.S:282: Error: found 'l', expected: ')'
>> 476arch/arm64/kernel/entry-fpsimd.S:98: Info: macro invoked from here
>> 477arch/arm64/kernel/entry-fpsimd.S:282: Error: found 'l', expected: ')'
>> 478arch/arm64/kernel/entry-fpsimd.S:98: Info: macro invoked from here
>> 479arch/arm64/kernel/entry-fpsimd.S:282: Error: found 'l', expected: ')'
>> 480arch/arm64/kernel/entry-fpsimd.S:98: Info: macro invoked from here
>> 481arch/arm64/kernel/entry-fpsimd.S:282: Error: found 'l', expected: ')'
>> 482arch/arm64/kernel/entry-fpsimd.S:98: Info: macro invoked from here
>> 483arch/arm64/kernel/entry-fpsimd.S:282: Error: unexpected characters
>> following instruction at operand 3 -- `bic x2,x1,(0+(((unsigned
>> long)~0ULL-((unsigned long)(1)<<(0))+1)&((unsigned
>> long)~0ULL>>((sizeof(unsigned long)*8)-1-(3)))))'
>> 484arch/arm64/kernel/entry-fpsimd.S:98: Info: macro invoked from here
>
>-- 
>With Best Regards,
>Andy Shevchenko
>
>

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: Re: [PATCH v3 1/3] bits: introduce fixed-type genmasks
  2024-02-21 21:59       ` Lucas De Marchi
@ 2024-02-22 14:49         ` Yury Norov
  2024-02-22 15:04           ` Andy Shevchenko
  2024-02-28 23:39           ` Lucas De Marchi
  0 siblings, 2 replies; 32+ messages in thread
From: Yury Norov @ 2024-02-22 14:49 UTC (permalink / raw)
  To: Lucas De Marchi
  Cc: Andy Shevchenko, Dmitry Baryshkov, linux-kernel, dri-devel,
	Jani Nikula, intel-xe, intel-gfx, Jani Nikula

On Wed, Feb 21, 2024 at 03:59:06PM -0600, Lucas De Marchi wrote:
> On Wed, Feb 21, 2024 at 11:04:22PM +0200, Andy Shevchenko wrote:
> > On Wed, Feb 21, 2024 at 10:30:02PM +0200, Dmitry Baryshkov wrote:
> > > On Thu, 8 Feb 2024 at 09:45, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
> > 
> > ...
> > 
> > > > +#define BITS_PER_TYPE(type)    (sizeof(type) * BITS_PER_BYTE)
> > 
> > Can sizeof() be used in assembly?
> > 
> > ...
> > 
> > > > -#define __GENMASK(h, l) \
> > > > -       (((~UL(0)) - (UL(1) << (l)) + 1) & \
> > > > -        (~UL(0) >> (BITS_PER_LONG - 1 - (h))))
> > > > -#define GENMASK(h, l) \
> > > > -       (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
> > 
> > > > +#define __GENMASK(t, h, l) \
> > > > +       (GENMASK_INPUT_CHECK(h, l) + \
> > > > +        (((t)~0ULL - ((t)(1) << (l)) + 1) & \
> > > > +        ((t)~0ULL >> (BITS_PER_TYPE(t) - 1 - (h)))))
> > 
> > Nevertheless, the use ~0ULL is not proper assembly, this broke initial
> > implementation using UL() / ULL().
> 
> indeed.
> 
> > 
> > 
> > > > -#define __GENMASK_ULL(h, l) \
> > > > -       (((~ULL(0)) - (ULL(1) << (l)) + 1) & \
> > > > -        (~ULL(0) >> (BITS_PER_LONG_LONG - 1 - (h))))
> > > > -#define GENMASK_ULL(h, l) \
> > > > -       (GENMASK_INPUT_CHECK(h, l) + __GENMASK_ULL(h, l))
> > 
> > Ditto.
> 
> problem here seems actually because of the cast to the final type. My
> previous impl was avoiding that, but was too verbose compared to this.
> 
> I will look at reverting this.
> 
> Lucas De Marchi

The fix is quite straightforward. Can you consider the following
patch? I tested it for C and x86_64 asm parts, and it compiles well.

Thanks,
Yury

From 78b2887eea26f208aac50ae283ba9a4d062bb997 Mon Sep 17 00:00:00 2001
From: Yury Norov <yury.norov@gmail.com>
Date: Wed, 7 Feb 2024 23:45:19 -0800
Subject: [PATCH v2] bits: introduce fixed-type GENMASKs

Generalize __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) & \
	      |                               ^~

CC: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>	      
Signed-off-by: Yury Norov <yury.norov@gmail.com>
Acked-by: Jani Nikula <jani.nikula@intel.com>
Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com>

---
 include/linux/bitops.h |  1 -
 include/linux/bits.h   | 41 ++++++++++++++++++++++++++++-------------
 2 files changed, 28 insertions(+), 14 deletions(-)

diff --git a/include/linux/bitops.h b/include/linux/bitops.h
index 2ba557e067fe..1db50c69cfdb 100644
--- a/include/linux/bitops.h
+++ b/include/linux/bitops.h
@@ -15,7 +15,6 @@
 #  define aligned_byte_mask(n) (~0xffUL << (BITS_PER_LONG - 8 - 8*(n)))
 #endif
 
-#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 7c0cf5031abe..f3cf8d5f2b55 100644
--- a/include/linux/bits.h
+++ b/include/linux/bits.h
@@ -6,6 +6,8 @@
 #include <vdso/bits.h>
 #include <asm/bitsperlong.h>
 
+#define BITS_PER_TYPE(type)	(sizeof(type) * BITS_PER_BYTE)
+
 #define BIT_MASK(nr)		(UL(1) << ((nr) % BITS_PER_LONG))
 #define BIT_WORD(nr)		((nr) / BITS_PER_LONG)
 #define BIT_ULL_MASK(nr)	(ULL(1) << ((nr) % BITS_PER_LONG_LONG))
@@ -22,24 +24,37 @@
 #define GENMASK_INPUT_CHECK(h, l) \
 	(BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
 		__is_constexpr((l) > (h)), (l) > (h), 0)))
+#define __GENMASK(t, h, l) \
+	(GENMASK_INPUT_CHECK(h, l) + \
+	 (((t)~0ULL - ((t)(1) << (l)) + 1) & \
+	 ((t)~0ULL >> (BITS_PER_TYPE(t) - 1 - (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.
+ * BUILD_BUG_ON_ZERO is not available in h files included from asm files.
+ * Similarly, assembler lacks for C types. So no parameters check in asm.
+ * It's users' responsibility to provide bitranges within a machine word
+ * boundaries.
  */
 #define GENMASK_INPUT_CHECK(h, l) 0
+#define __GENMASK(t, h, l) \
+	((~0 - (1 << (l)) + 1) & (~0 >> (BITS_PER_LONG - 1 - (h))))
 #endif
 
-#define __GENMASK(h, l) \
-	(((~UL(0)) - (UL(1) << (l)) + 1) & \
-	 (~UL(0) >> (BITS_PER_LONG - 1 - (h))))
-#define GENMASK(h, l) \
-	(GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
-
-#define __GENMASK_ULL(h, l) \
-	(((~ULL(0)) - (ULL(1) << (l)) + 1) & \
-	 (~ULL(0) >> (BITS_PER_LONG_LONG - 1 - (h))))
-#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(h, l)		__GENMASK(unsigned long,  h, l)
+#define GENMASK_ULL(h, l)	__GENMASK(unsigned long long, h, l)
+#define GENMASK_U8(h, l)	__GENMASK(u8,  h, l)
+#define GENMASK_U16(h, l)	__GENMASK(u16, h, l)
+#define GENMASK_U32(h, l)	__GENMASK(u32, h, l)
+#define GENMASK_U64(h, l)	__GENMASK(u64, h, l)
 
 #endif	/* __LINUX_BITS_H */
-- 
2.40.1


^ permalink raw reply related	[flat|nested] 32+ messages in thread

* Re: Re: [PATCH v3 2/3] bits: Introduce fixed-type BIT
  2024-02-20  5:13     ` Lucas De Marchi
@ 2024-02-22 14:51       ` Yury Norov
  2024-02-22 16:35         ` Lucas De Marchi
  0 siblings, 1 reply; 32+ messages in thread
From: Yury Norov @ 2024-02-22 14:51 UTC (permalink / raw)
  To: Lucas De Marchi
  Cc: linux-kernel, dri-devel, Andy Shevchenko, Jani Nikula, intel-xe,
	intel-gfx, Jani Nikula

On Mon, Feb 19, 2024 at 11:13:57PM -0600, Lucas De Marchi wrote:
> On Fri, Feb 09, 2024 at 08:53:25AM -0800, Yury Norov wrote:
> > On Wed, Feb 07, 2024 at 11:45:20PM -0800, Lucas De Marchi wrote:
> > > 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>
> > 
> > So I get v1 from Jan.23 in my mailbox, and this one is v3. Did I miss
> > a v2? Anyways, please bear my reviewed-by from v1 for this patch.
> 
> Jan 23 was actually the v2 and I missed the subject prefix.
> 
> My understanding was that you were going to apply this through some
> bitmap tree, but checking MAINTAINERS now it seems there's no git tree
> associated.  So I will just add your r-b and merge this through
> drm-xe.

I've got a bitmap-related branch. I can move this series in there if
you prefer. At your discretion.

https://github.com/norov/linux/tree/bitmap_for_next

Thanks,
Yury

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: Re: [PATCH v3 1/3] bits: introduce fixed-type genmasks
  2024-02-22 14:49         ` Yury Norov
@ 2024-02-22 15:04           ` Andy Shevchenko
  2024-02-22 21:31             ` Yury Norov
  2024-02-28 23:39           ` Lucas De Marchi
  1 sibling, 1 reply; 32+ messages in thread
From: Andy Shevchenko @ 2024-02-22 15:04 UTC (permalink / raw)
  To: Yury Norov
  Cc: Lucas De Marchi, Dmitry Baryshkov, linux-kernel, dri-devel,
	Jani Nikula, intel-xe, intel-gfx, Jani Nikula

On Thu, Feb 22, 2024 at 06:49:59AM -0800, Yury Norov wrote:
> On Wed, Feb 21, 2024 at 03:59:06PM -0600, Lucas De Marchi wrote:

...

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

What's wrong on using the UL/ULL() macros?

Also it would be really good to avoid bifurcation of the implementations of
__GENMASK() for both cases.

...

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

This at bare minimum can be left untouched for asm case, no?

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: Re: Re: [PATCH v3 2/3] bits: Introduce fixed-type BIT
  2024-02-22 14:51       ` Yury Norov
@ 2024-02-22 16:35         ` Lucas De Marchi
  0 siblings, 0 replies; 32+ messages in thread
From: Lucas De Marchi @ 2024-02-22 16:35 UTC (permalink / raw)
  To: Yury Norov
  Cc: linux-kernel, dri-devel, Andy Shevchenko, Jani Nikula, intel-xe,
	intel-gfx, Jani Nikula

On Thu, Feb 22, 2024 at 06:51:52AM -0800, Yury Norov wrote:
>On Mon, Feb 19, 2024 at 11:13:57PM -0600, Lucas De Marchi wrote:
>> On Fri, Feb 09, 2024 at 08:53:25AM -0800, Yury Norov wrote:
>> > On Wed, Feb 07, 2024 at 11:45:20PM -0800, Lucas De Marchi wrote:
>> > > 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>
>> >
>> > So I get v1 from Jan.23 in my mailbox, and this one is v3. Did I miss
>> > a v2? Anyways, please bear my reviewed-by from v1 for this patch.
>>
>> Jan 23 was actually the v2 and I missed the subject prefix.
>>
>> My understanding was that you were going to apply this through some
>> bitmap tree, but checking MAINTAINERS now it seems there's no git tree
>> associated.  So I will just add your r-b and merge this through
>> drm-xe.
>
>I've got a bitmap-related branch. I can move this series in there if
>you prefer. At your discretion.
>
>https://github.com/norov/linux/tree/bitmap_for_next

yeah, sounds good.

thanks
Lucas De Marchi

>
>Thanks,
>Yury

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: Re: [PATCH v3 1/3] bits: introduce fixed-type genmasks
  2024-02-22 15:04           ` Andy Shevchenko
@ 2024-02-22 21:31             ` Yury Norov
  0 siblings, 0 replies; 32+ messages in thread
From: Yury Norov @ 2024-02-22 21:31 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Lucas De Marchi, Dmitry Baryshkov, linux-kernel, dri-devel,
	Jani Nikula, intel-xe, intel-gfx, Jani Nikula

On Thu, Feb 22, 2024 at 05:04:10PM +0200, Andy Shevchenko wrote:
> On Thu, Feb 22, 2024 at 06:49:59AM -0800, Yury Norov wrote:
> > On Wed, Feb 21, 2024 at 03:59:06PM -0600, Lucas De Marchi wrote:
> 
> ...
> 
> > +#define __GENMASK(t, h, l) \
> > +	((~0 - (1 << (l)) + 1) & (~0 >> (BITS_PER_LONG - 1 - (h))))
> 
> What's wrong on using the UL/ULL() macros?

Nothing wrong except that in the !__ASSEMBLY section they all are
useless. And having that in mind, useless macros may hurt readability.
 
> Also it would be really good to avoid bifurcation of the implementations of
> __GENMASK() for both cases.

Not exactly. It would be really good if GENMASK_XX() will share the
implementation (and they do). The underscored helper macro is not
intended to be used directly, and I think nobody cares.

> ...
> 
> > -#define __GENMASK(h, l) \
> > -	(((~UL(0)) - (UL(1) << (l)) + 1) & \
> > -	 (~UL(0) >> (BITS_PER_LONG - 1 - (h))))
> 
> This at bare minimum can be left untouched for asm case, no?

As soon as we have to have different versions for the macro depending
on __ASSEMBLY__, I would prefer to remove all compatibility black
magic. After all, the <linux/bits.h> machinery to me is about the same
level of abstraction as the stuff in <linux/const.h>, and in future we
can try to remove dependency on it.

This all is not a big deal to me. I can keep the old implementation
for the asm, if you think it's really important.

What are you thinking guys?

Thanks,
Yury

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v3 1/3] bits: introduce fixed-type genmasks
  2024-02-22 14:49         ` Yury Norov
  2024-02-22 15:04           ` Andy Shevchenko
@ 2024-02-28 23:39           ` Lucas De Marchi
  2024-02-29 10:49             ` Andy Shevchenko
  1 sibling, 1 reply; 32+ messages in thread
From: Lucas De Marchi @ 2024-02-28 23:39 UTC (permalink / raw)
  To: Yury Norov
  Cc: Andy Shevchenko, Dmitry Baryshkov, linux-kernel, dri-devel,
	Jani Nikula, intel-xe, intel-gfx, Jani Nikula

On Thu, Feb 22, 2024 at 06:49:59AM -0800, Yury Norov wrote:
>On Wed, Feb 21, 2024 at 03:59:06PM -0600, Lucas De Marchi wrote:
>> On Wed, Feb 21, 2024 at 11:04:22PM +0200, Andy Shevchenko wrote:
>> > On Wed, Feb 21, 2024 at 10:30:02PM +0200, Dmitry Baryshkov wrote:
>> > > On Thu, 8 Feb 2024 at 09:45, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
>> >
>> > ...
>> >
>> > > > +#define BITS_PER_TYPE(type)    (sizeof(type) * BITS_PER_BYTE)
>> >
>> > Can sizeof() be used in assembly?
>> >
>> > ...
>> >
>> > > > -#define __GENMASK(h, l) \
>> > > > -       (((~UL(0)) - (UL(1) << (l)) + 1) & \
>> > > > -        (~UL(0) >> (BITS_PER_LONG - 1 - (h))))
>> > > > -#define GENMASK(h, l) \
>> > > > -       (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
>> >
>> > > > +#define __GENMASK(t, h, l) \
>> > > > +       (GENMASK_INPUT_CHECK(h, l) + \
>> > > > +        (((t)~0ULL - ((t)(1) << (l)) + 1) & \
>> > > > +        ((t)~0ULL >> (BITS_PER_TYPE(t) - 1 - (h)))))
>> >
>> > Nevertheless, the use ~0ULL is not proper assembly, this broke initial
>> > implementation using UL() / ULL().
>>
>> indeed.
>>
>> >
>> >
>> > > > -#define __GENMASK_ULL(h, l) \
>> > > > -       (((~ULL(0)) - (ULL(1) << (l)) + 1) & \
>> > > > -        (~ULL(0) >> (BITS_PER_LONG_LONG - 1 - (h))))
>> > > > -#define GENMASK_ULL(h, l) \
>> > > > -       (GENMASK_INPUT_CHECK(h, l) + __GENMASK_ULL(h, l))
>> >
>> > Ditto.
>>
>> problem here seems actually because of the cast to the final type. My
>> previous impl was avoiding that, but was too verbose compared to this.
>>
>> I will look at reverting this.
>>
>> Lucas De Marchi
>
>The fix is quite straightforward. Can you consider the following
>patch? I tested it for C and x86_64 asm parts, and it compiles well.
>
>Thanks,
>Yury
>
>From 78b2887eea26f208aac50ae283ba9a4d062bb997 Mon Sep 17 00:00:00 2001
>From: Yury Norov <yury.norov@gmail.com>
>Date: Wed, 7 Feb 2024 23:45:19 -0800
>Subject: [PATCH v2] bits: introduce fixed-type GENMASKs
>
>Generalize __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) & \
>	      |                               ^~
>
>CC: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>	
>Signed-off-by: Yury Norov <yury.norov@gmail.com>
>Acked-by: Jani Nikula <jani.nikula@intel.com>
>Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com>

I build-tested this in x86-64, x86-32 and arm64. I didn't like much the
need to fork the __GENMASK() implementation on the 2 sides of the ifdef
since I think the GENMASK_INPUT_CHECK() should be the one covering the
input checks. However to make it common we'd need to solve 2 problems:
the casts and the sizeof. The sizeof can be passed as arg to
__GENMASK(), however the casts I think would need a __CAST_U8(x)
or the like and sprinkle it everywhere, which would hurt readability.
Not pretty. Or go back to the original submission and make it less
horrible :-/

>
>---
> include/linux/bitops.h |  1 -
> include/linux/bits.h   | 41 ++++++++++++++++++++++++++++-------------
> 2 files changed, 28 insertions(+), 14 deletions(-)
>
>diff --git a/include/linux/bitops.h b/include/linux/bitops.h
>index 2ba557e067fe..1db50c69cfdb 100644
>--- a/include/linux/bitops.h
>+++ b/include/linux/bitops.h
>@@ -15,7 +15,6 @@
> #  define aligned_byte_mask(n) (~0xffUL << (BITS_PER_LONG - 8 - 8*(n)))
> #endif
>
>-#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 7c0cf5031abe..f3cf8d5f2b55 100644
>--- a/include/linux/bits.h
>+++ b/include/linux/bits.h
>@@ -6,6 +6,8 @@
> #include <vdso/bits.h>
> #include <asm/bitsperlong.h>
>
>+#define BITS_PER_TYPE(type)	(sizeof(type) * BITS_PER_BYTE)
>+
> #define BIT_MASK(nr)		(UL(1) << ((nr) % BITS_PER_LONG))
> #define BIT_WORD(nr)		((nr) / BITS_PER_LONG)
> #define BIT_ULL_MASK(nr)	(ULL(1) << ((nr) % BITS_PER_LONG_LONG))
>@@ -22,24 +24,37 @@
> #define GENMASK_INPUT_CHECK(h, l) \
> 	(BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
> 		__is_constexpr((l) > (h)), (l) > (h), 0)))
>+#define __GENMASK(t, h, l) \
>+	(GENMASK_INPUT_CHECK(h, l) + \
>+	 (((t)~0ULL - ((t)(1) << (l)) + 1) & \
>+	 ((t)~0ULL >> (BITS_PER_TYPE(t) - 1 - (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.
>+ * BUILD_BUG_ON_ZERO is not available in h files included from asm files.
>+ * Similarly, assembler lacks for C types. So no parameters check in asm.
>+ * It's users' responsibility to provide bitranges within a machine word
>+ * boundaries.
>  */
> #define GENMASK_INPUT_CHECK(h, l) 0
>+#define __GENMASK(t, h, l) \
>+	((~0 - (1 << (l)) + 1) & (~0 >> (BITS_PER_LONG - 1 - (h))))

humn... this builds, but does it work if GENMASK_ULL() is used in
assembly? That BITS_PER_LONG does not match the type width.

Lucas De Marchi

> #endif
>
>-#define __GENMASK(h, l) \
>-	(((~UL(0)) - (UL(1) << (l)) + 1) & \
>-	 (~UL(0) >> (BITS_PER_LONG - 1 - (h))))
>-#define GENMASK(h, l) \
>-	(GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
>-
>-#define __GENMASK_ULL(h, l) \
>-	(((~ULL(0)) - (ULL(1) << (l)) + 1) & \
>-	 (~ULL(0) >> (BITS_PER_LONG_LONG - 1 - (h))))
>-#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(h, l)		__GENMASK(unsigned long,  h, l)
>+#define GENMASK_ULL(h, l)	__GENMASK(unsigned long long, h, l)
>+#define GENMASK_U8(h, l)	__GENMASK(u8,  h, l)
>+#define GENMASK_U16(h, l)	__GENMASK(u16, h, l)
>+#define GENMASK_U32(h, l)	__GENMASK(u32, h, l)
>+#define GENMASK_U64(h, l)	__GENMASK(u64, h, l)
>
> #endif	/* __LINUX_BITS_H */
>-- 
>2.40.1
>

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v3 1/3] bits: introduce fixed-type genmasks
  2024-02-28 23:39           ` Lucas De Marchi
@ 2024-02-29 10:49             ` Andy Shevchenko
  2024-02-29 18:21               ` Lucas De Marchi
  0 siblings, 1 reply; 32+ messages in thread
From: Andy Shevchenko @ 2024-02-29 10:49 UTC (permalink / raw)
  To: Lucas De Marchi
  Cc: Yury Norov, Dmitry Baryshkov, linux-kernel, dri-devel,
	Jani Nikula, intel-xe, intel-gfx, Jani Nikula

On Wed, Feb 28, 2024 at 05:39:21PM -0600, Lucas De Marchi wrote:
> On Thu, Feb 22, 2024 at 06:49:59AM -0800, Yury Norov wrote:
> > On Wed, Feb 21, 2024 at 03:59:06PM -0600, Lucas De Marchi wrote:
> > > On Wed, Feb 21, 2024 at 11:04:22PM +0200, Andy Shevchenko wrote:
> > > > On Wed, Feb 21, 2024 at 10:30:02PM +0200, Dmitry Baryshkov wrote:
> > > > > On Thu, 8 Feb 2024 at 09:45, Lucas De Marchi <lucas.demarchi@intel.com> wrote:

...

> I build-tested this in x86-64, x86-32 and arm64. I didn't like much the
> need to fork the __GENMASK() implementation on the 2 sides of the ifdef
> since I think the GENMASK_INPUT_CHECK() should be the one covering the
> input checks. However to make it common we'd need to solve 2 problems:
> the casts and the sizeof. The sizeof can be passed as arg to
> __GENMASK(), however the casts I think would need a __CAST_U8(x)
> or the like and sprinkle it everywhere, which would hurt readability.
> Not pretty. Or go back to the original submission and make it less
> horrible :-/

I'm wondering if we can use _Generic() approach here.

...

> > #define GENMASK_INPUT_CHECK(h, l) 0
> > +#define __GENMASK(t, h, l) \
> > +	((~0 - (1 << (l)) + 1) & (~0 >> (BITS_PER_LONG - 1 - (h))))
> 
> humn... this builds, but does it work if GENMASK_ULL() is used in
> assembly? That BITS_PER_LONG does not match the type width.

UL()/ULL() macros are not just for fun.

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v3 1/3] bits: introduce fixed-type genmasks
  2024-02-29 10:49             ` Andy Shevchenko
@ 2024-02-29 18:21               ` Lucas De Marchi
  2024-02-29 18:27                 ` Andy Shevchenko
  0 siblings, 1 reply; 32+ messages in thread
From: Lucas De Marchi @ 2024-02-29 18:21 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Yury Norov, Dmitry Baryshkov, linux-kernel, dri-devel,
	Jani Nikula, intel-xe, intel-gfx, Jani Nikula

On Thu, Feb 29, 2024 at 12:49:57PM +0200, Andy Shevchenko wrote:
>On Wed, Feb 28, 2024 at 05:39:21PM -0600, Lucas De Marchi wrote:
>> On Thu, Feb 22, 2024 at 06:49:59AM -0800, Yury Norov wrote:
>> > On Wed, Feb 21, 2024 at 03:59:06PM -0600, Lucas De Marchi wrote:
>> > > On Wed, Feb 21, 2024 at 11:04:22PM +0200, Andy Shevchenko wrote:
>> > > > On Wed, Feb 21, 2024 at 10:30:02PM +0200, Dmitry Baryshkov wrote:
>> > > > > On Thu, 8 Feb 2024 at 09:45, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
>
>...
>
>> I build-tested this in x86-64, x86-32 and arm64. I didn't like much the
>> need to fork the __GENMASK() implementation on the 2 sides of the ifdef
>> since I think the GENMASK_INPUT_CHECK() should be the one covering the
>> input checks. However to make it common we'd need to solve 2 problems:
>> the casts and the sizeof. The sizeof can be passed as arg to
>> __GENMASK(), however the casts I think would need a __CAST_U8(x)
>> or the like and sprinkle it everywhere, which would hurt readability.
>> Not pretty. Or go back to the original submission and make it less
>> horrible :-/
>
>I'm wondering if we can use _Generic() approach here.

in assembly?

>
>...
>
>> > #define GENMASK_INPUT_CHECK(h, l) 0
>> > +#define __GENMASK(t, h, l) \
>> > +	((~0 - (1 << (l)) + 1) & (~0 >> (BITS_PER_LONG - 1 - (h))))
>>
>> humn... this builds, but does it work if GENMASK_ULL() is used in
>> assembly? That BITS_PER_LONG does not match the type width.
>
>UL()/ULL() macros are not just for fun.

they are not for fun, but they expand to a nop in assembly. And it's up
to the instruction used to be the right one. Since this branch is for
assembly only, having them wouldn't really change the current behavior.
I'm talking about BITS_PER_LONG vs BITS_PER_LONG_LONG. That introduces
a bug here.

Lucas De Marchi

>
>-- 
>With Best Regards,
>Andy Shevchenko
>
>

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v3 1/3] bits: introduce fixed-type genmasks
  2024-02-29 18:21               ` Lucas De Marchi
@ 2024-02-29 18:27                 ` Andy Shevchenko
  2024-03-01  4:06                   ` Lucas De Marchi
  0 siblings, 1 reply; 32+ messages in thread
From: Andy Shevchenko @ 2024-02-29 18:27 UTC (permalink / raw)
  To: Lucas De Marchi
  Cc: Yury Norov, Dmitry Baryshkov, linux-kernel, dri-devel,
	Jani Nikula, intel-xe, intel-gfx, Jani Nikula

On Thu, Feb 29, 2024 at 12:21:34PM -0600, Lucas De Marchi wrote:
> On Thu, Feb 29, 2024 at 12:49:57PM +0200, Andy Shevchenko wrote:
> > On Wed, Feb 28, 2024 at 05:39:21PM -0600, Lucas De Marchi wrote:
> > > On Thu, Feb 22, 2024 at 06:49:59AM -0800, Yury Norov wrote:

...

> > > I build-tested this in x86-64, x86-32 and arm64. I didn't like much the
> > > need to fork the __GENMASK() implementation on the 2 sides of the ifdef
> > > since I think the GENMASK_INPUT_CHECK() should be the one covering the
> > > input checks. However to make it common we'd need to solve 2 problems:
> > > the casts and the sizeof. The sizeof can be passed as arg to
> > > __GENMASK(), however the casts I think would need a __CAST_U8(x)
> > > or the like and sprinkle it everywhere, which would hurt readability.
> > > Not pretty. Or go back to the original submission and make it less
> > > horrible :-/
> > 
> > I'm wondering if we can use _Generic() approach here.
> 
> in assembly?

Yes.

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v3 1/3] bits: introduce fixed-type genmasks
  2024-02-29 18:27                 ` Andy Shevchenko
@ 2024-03-01  4:06                   ` Lucas De Marchi
  2024-03-07 21:45                     ` Lucas De Marchi
  0 siblings, 1 reply; 32+ messages in thread
From: Lucas De Marchi @ 2024-03-01  4:06 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Yury Norov, Dmitry Baryshkov, linux-kernel, dri-devel,
	Jani Nikula, intel-xe, intel-gfx, Jani Nikula

On Thu, Feb 29, 2024 at 08:27:30PM +0200, Andy Shevchenko wrote:
>On Thu, Feb 29, 2024 at 12:21:34PM -0600, Lucas De Marchi wrote:
>> On Thu, Feb 29, 2024 at 12:49:57PM +0200, Andy Shevchenko wrote:
>> > On Wed, Feb 28, 2024 at 05:39:21PM -0600, Lucas De Marchi wrote:
>> > > On Thu, Feb 22, 2024 at 06:49:59AM -0800, Yury Norov wrote:
>
>...
>
>> > > I build-tested this in x86-64, x86-32 and arm64. I didn't like much the
>> > > need to fork the __GENMASK() implementation on the 2 sides of the ifdef
>> > > since I think the GENMASK_INPUT_CHECK() should be the one covering the
>> > > input checks. However to make it common we'd need to solve 2 problems:
>> > > the casts and the sizeof. The sizeof can be passed as arg to
>> > > __GENMASK(), however the casts I think would need a __CAST_U8(x)
>> > > or the like and sprinkle it everywhere, which would hurt readability.
>> > > Not pretty. Or go back to the original submission and make it less
>> > > horrible :-/
>> >
>> > I'm wondering if we can use _Generic() approach here.
>>
>> in assembly?
>
>Yes.

I added a _Generic() in a random .S and to my surprise the build didn't
break. Then I went to implement, and couldn't find where the _Generic()
would actually be useful. What I came up with builds for me with gcc on
x86-64, x86-32 and arm64.

I'm also adding some additional tests in lib/test_bits.c to cover the
expected values and types. Thoughts?

--------8<------------
Subject: [PATCH] bits: introduce fixed-type genmasks

Generalize __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:48:23: warning: right shift count is negative [-Wshift-count-negative]
	   48 |          (~literal(0) >> ((w) - 1 - (h)))))
	      |                       ^~

Some additional tests in lib/test_bits.c are added to cover the
expected/non-expected values and also 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>
---
  include/linux/bits.h | 33 +++++++++++++++++++++++----------
  lib/test_bits.c      | 21 +++++++++++++++++++--
  2 files changed, 42 insertions(+), 12 deletions(-)

diff --git a/include/linux/bits.h b/include/linux/bits.h
index 7c0cf5031abe8..6f089e71a195c 100644
--- a/include/linux/bits.h
+++ b/include/linux/bits.h
@@ -22,24 +22,37 @@
  #define GENMASK_INPUT_CHECK(h, l) \
  	(BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
  		__is_constexpr((l) > (h)), (l) > (h), 0)))
+#define __CAST_T(t, v) ((t)v)
  #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
+#define __CAST_T(t, v) v
  #endif
  
-#define __GENMASK(h, l) \
-	(((~UL(0)) - (UL(1) << (l)) + 1) & \
-	 (~UL(0) >> (BITS_PER_LONG - 1 - (h))))
-#define GENMASK(h, l) \
-	(GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
+/*
+ * Generate a mask for a specific type. @literal is the suffix to be used for
+ * an integer constant of that type and @width is the bits-per-type. 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(literal, w, h, l) \
+	(GENMASK_INPUT_CHECK(h, l) + \
+	 ((~literal(0) - (literal(1) << (l)) + 1) & \
+	 (~literal(0) >> ((w) - 1 - (h)))))
  
-#define __GENMASK_ULL(h, l) \
-	(((~ULL(0)) - (ULL(1) << (l)) + 1) & \
-	 (~ULL(0) >> (BITS_PER_LONG_LONG - 1 - (h))))
-#define GENMASK_ULL(h, l) \
-	(GENMASK_INPUT_CHECK(h, l) + __GENMASK_ULL(h, l))
+#define GENMASK(h, l)		__GENMASK(UL, BITS_PER_LONG, h, l)
+#define GENMASK_ULL(h, l)	__GENMASK(ULL, BITS_PER_LONG_LONG, h, l)
+#define GENMASK_U8(h, l)	__CAST_T(u8, __GENMASK(UL, 8, h, l))
+#define GENMASK_U16(h, l)	__CAST_T(u16, __GENMASK(UL, 16, h, l))
+#define GENMASK_U32(h, l)	__CAST_T(u32, __GENMASK(UL, 32, h, l))
+#define GENMASK_U64(h, l)	__CAST_T(u64, __GENMASK(ULL, 64, h, l))
  
  #endif	/* __LINUX_BITS_H */
diff --git a/lib/test_bits.c b/lib/test_bits.c
index c9368a2314e7c..e2fc1a1d38702 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(u64, GENMASK_U64(63, 0)) == U64_MAX);
+static_assert(assert_type(u32, GENMASK_U32(31, 0)) == U32_MAX);
+static_assert(assert_type(u16, GENMASK_U16(15, 0)) == U16_MAX);
+static_assert(assert_type(u8,  GENMASK_U8(7, 0))   == U8_MAX);
  
  static void genmask_test(struct kunit *test)
  {
@@ -14,14 +23,22 @@ 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);
-#endif
-
  
+	GENMASK_U32(0, 31);
+	GENMASK_U64(64, 0);
+	GENMASK_U32(32, 0);
+	GENMASK_U16(16, 0);
+	GENMASK_U8(8, 0);
+#endif
  }
  
  static void genmask_ull_test(struct kunit *test)
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 32+ messages in thread

* Re: [PATCH v3 1/3] bits: introduce fixed-type genmasks
  2024-03-01  4:06                   ` Lucas De Marchi
@ 2024-03-07 21:45                     ` Lucas De Marchi
  0 siblings, 0 replies; 32+ messages in thread
From: Lucas De Marchi @ 2024-03-07 21:45 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Yury Norov, Dmitry Baryshkov, linux-kernel, dri-devel,
	Jani Nikula, intel-xe, intel-gfx, Jani Nikula

On Thu, Feb 29, 2024 at 10:06:02PM -0600, Lucas De Marchi wrote:
>On Thu, Feb 29, 2024 at 08:27:30PM +0200, Andy Shevchenko wrote:
>>On Thu, Feb 29, 2024 at 12:21:34PM -0600, Lucas De Marchi wrote:
>>>On Thu, Feb 29, 2024 at 12:49:57PM +0200, Andy Shevchenko wrote:
>>>> On Wed, Feb 28, 2024 at 05:39:21PM -0600, Lucas De Marchi wrote:
>>>> > On Thu, Feb 22, 2024 at 06:49:59AM -0800, Yury Norov wrote:
>>
>>...
>>
>>>> > I build-tested this in x86-64, x86-32 and arm64. I didn't like much the
>>>> > need to fork the __GENMASK() implementation on the 2 sides of the ifdef
>>>> > since I think the GENMASK_INPUT_CHECK() should be the one covering the
>>>> > input checks. However to make it common we'd need to solve 2 problems:
>>>> > the casts and the sizeof. The sizeof can be passed as arg to
>>>> > __GENMASK(), however the casts I think would need a __CAST_U8(x)
>>>> > or the like and sprinkle it everywhere, which would hurt readability.
>>>> > Not pretty. Or go back to the original submission and make it less
>>>> > horrible :-/
>>>>
>>>> I'm wondering if we can use _Generic() approach here.
>>>
>>>in assembly?
>>
>>Yes.
>
>I added a _Generic() in a random .S and to my surprise the build didn't
>break. Then I went to implement, and couldn't find where the _Generic()
>would actually be useful. What I came up with builds for me with gcc on
>x86-64, x86-32 and arm64.
>
>I'm also adding some additional tests in lib/test_bits.c to cover the
>expected values and types. Thoughts?
>
>--------8<------------
>Subject: [PATCH] bits: introduce fixed-type genmasks

Yury, is this something you'd take through your tree? Should I prepare
the other patches on top and get some more arch coverage?

thanks
Lucas De Marchi

>
>Generalize __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:48:23: warning: right shift count is negative [-Wshift-count-negative]
>	   48 |          (~literal(0) >> ((w) - 1 - (h)))))
>	      |                       ^~
>
>Some additional tests in lib/test_bits.c are added to cover the
>expected/non-expected values and also 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>
>---
> include/linux/bits.h | 33 +++++++++++++++++++++++----------
> lib/test_bits.c      | 21 +++++++++++++++++++--
> 2 files changed, 42 insertions(+), 12 deletions(-)
>
>diff --git a/include/linux/bits.h b/include/linux/bits.h
>index 7c0cf5031abe8..6f089e71a195c 100644
>--- a/include/linux/bits.h
>+++ b/include/linux/bits.h
>@@ -22,24 +22,37 @@
> #define GENMASK_INPUT_CHECK(h, l) \
> 	(BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
> 		__is_constexpr((l) > (h)), (l) > (h), 0)))
>+#define __CAST_T(t, v) ((t)v)
> #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
>+#define __CAST_T(t, v) v
> #endif
>-#define __GENMASK(h, l) \
>-	(((~UL(0)) - (UL(1) << (l)) + 1) & \
>-	 (~UL(0) >> (BITS_PER_LONG - 1 - (h))))
>-#define GENMASK(h, l) \
>-	(GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
>+/*
>+ * Generate a mask for a specific type. @literal is the suffix to be used for
>+ * an integer constant of that type and @width is the bits-per-type. 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(literal, w, h, l) \
>+	(GENMASK_INPUT_CHECK(h, l) + \
>+	 ((~literal(0) - (literal(1) << (l)) + 1) & \
>+	 (~literal(0) >> ((w) - 1 - (h)))))
>-#define __GENMASK_ULL(h, l) \
>-	(((~ULL(0)) - (ULL(1) << (l)) + 1) & \
>-	 (~ULL(0) >> (BITS_PER_LONG_LONG - 1 - (h))))
>-#define GENMASK_ULL(h, l) \
>-	(GENMASK_INPUT_CHECK(h, l) + __GENMASK_ULL(h, l))
>+#define GENMASK(h, l)		__GENMASK(UL, BITS_PER_LONG, h, l)
>+#define GENMASK_ULL(h, l)	__GENMASK(ULL, BITS_PER_LONG_LONG, h, l)
>+#define GENMASK_U8(h, l)	__CAST_T(u8, __GENMASK(UL, 8, h, l))
>+#define GENMASK_U16(h, l)	__CAST_T(u16, __GENMASK(UL, 16, h, l))
>+#define GENMASK_U32(h, l)	__CAST_T(u32, __GENMASK(UL, 32, h, l))
>+#define GENMASK_U64(h, l)	__CAST_T(u64, __GENMASK(ULL, 64, h, l))
> #endif	/* __LINUX_BITS_H */
>diff --git a/lib/test_bits.c b/lib/test_bits.c
>index c9368a2314e7c..e2fc1a1d38702 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(u64, GENMASK_U64(63, 0)) == U64_MAX);
>+static_assert(assert_type(u32, GENMASK_U32(31, 0)) == U32_MAX);
>+static_assert(assert_type(u16, GENMASK_U16(15, 0)) == U16_MAX);
>+static_assert(assert_type(u8,  GENMASK_U8(7, 0))   == U8_MAX);
> static void genmask_test(struct kunit *test)
> {
>@@ -14,14 +23,22 @@ 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);
>-#endif
>-
>+	GENMASK_U32(0, 31);
>+	GENMASK_U64(64, 0);
>+	GENMASK_U32(32, 0);
>+	GENMASK_U16(16, 0);
>+	GENMASK_U8(8, 0);
>+#endif
> }
> static void genmask_ull_test(struct kunit *test)
>-- 
>2.43.0
>

^ permalink raw reply	[flat|nested] 32+ messages in thread

end of thread, other threads:[~2024-03-07 21:46 UTC | newest]

Thread overview: 32+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-02-08  7:45 [PATCH v3 0/3] Fixed-type GENMASK/BIT Lucas De Marchi
2024-02-08  7:45 ` [PATCH v3 1/3] bits: introduce fixed-type genmasks Lucas De Marchi
2024-02-08 19:48   ` Andi Shyti
2024-02-09 14:04     ` Andy Shevchenko
2024-02-21 20:30   ` Dmitry Baryshkov
2024-02-21 20:37     ` Dmitry Baryshkov
2024-02-21 21:06       ` Andy Shevchenko
2024-02-21 21:07         ` Andy Shevchenko
2024-02-21 21:04     ` Andy Shevchenko
2024-02-21 21:59       ` Lucas De Marchi
2024-02-22 14:49         ` Yury Norov
2024-02-22 15:04           ` Andy Shevchenko
2024-02-22 21:31             ` Yury Norov
2024-02-28 23:39           ` Lucas De Marchi
2024-02-29 10:49             ` Andy Shevchenko
2024-02-29 18:21               ` Lucas De Marchi
2024-02-29 18:27                 ` Andy Shevchenko
2024-03-01  4:06                   ` Lucas De Marchi
2024-03-07 21:45                     ` Lucas De Marchi
2024-02-08  7:45 ` [PATCH v3 2/3] bits: Introduce fixed-type BIT Lucas De Marchi
2024-02-08 20:04   ` Andi Shyti
2024-02-08 21:17     ` Lucas De Marchi
2024-02-09  8:01       ` Jani Nikula
2024-02-10 14:22       ` David Laight
2024-02-09 16:53   ` Yury Norov
2024-02-20  5:13     ` Lucas De Marchi
2024-02-22 14:51       ` Yury Norov
2024-02-22 16:35         ` Lucas De Marchi
2024-02-08  7:45 ` [PATCH v3 3/3] drm/i915: Convert REG_GENMASK* to fixed-width GENMASK_* Lucas De Marchi
2024-02-08  8:49   ` Jani Nikula
2024-02-08 20:07   ` Andi Shyti
2024-02-08 21:21     ` Lucas De Marchi

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).