[PATCH v3 1/3] bits: introduce fixed-type genmasks

Thu Mar 7 21:45:56 UTC 2024

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