Re: [PATCH v3 1/3] bits: introduce fixed-type genmasks
From: Lucas De Marchi
Date: Thu Mar 07 2024 - 16:47:24 EST
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@xxxxxxxxx>
---
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