[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <Z9mjJ3gJoqLwjIFX@thinkpad>
Date: Tue, 18 Mar 2025 12:45:27 -0400
From: Yury Norov <yury.norov@...il.com>
To: mailhol.vincent@...adoo.fr
Cc: Lucas De Marchi <lucas.demarchi@...el.com>,
Rasmus Villemoes <linux@...musvillemoes.dk>,
Jani Nikula <jani.nikula@...ux.intel.com>,
Joonas Lahtinen <joonas.lahtinen@...ux.intel.com>,
Rodrigo Vivi <rodrigo.vivi@...el.com>,
Tvrtko Ursulin <tursulin@...ulin.net>,
David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>,
Andrew Morton <akpm@...ux-foundation.org>,
linux-kernel@...r.kernel.org, intel-gfx@...ts.freedesktop.org,
dri-devel@...ts.freedesktop.org,
Andi Shyti <andi.shyti@...ux.intel.com>,
David Laight <David.Laight@...lab.com>,
Dmitry Baryshkov <dmitry.baryshkov@...aro.org>,
Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
Jani Nikula <jani.nikula@...el.com>
Subject: Re: [PATCH v6 2/7] bits: introduce fixed-type genmasks
On Sat, Mar 08, 2025 at 01:48:49AM +0900, Vincent Mailhol via B4 Relay wrote:
> From: Yury Norov <yury.norov@...il.com>
>
> Add GENMASK_TYPE() which generalizes __GENMASK() to support different
> types, and implement fixed-types versions of GENMASK() based on it.
> The fixed-type version allows more strict checks to the min/max values
> accepted, which is useful for defining registers like implemented by
> i915 and xe drivers with their REG_GENMASK*() macros.
>
> The strict checks rely on shift-count-overflow compiler check to fail
> the build if a number outside of the range allowed is passed.
> Example:
>
> #define FOO_MASK GENMASK_U32(33, 4)
>
> will generate a warning like:
>
> include/linux/bits.h:51:27: error: right shift count >= width of type [-Werror=shift-count-overflow]
> 51 | type_max(t) >> (BITS_PER_TYPE(t) - 1 - (h)))))
> | ^~
>
> Signed-off-by: Yury Norov <yury.norov@...il.com>
> Signed-off-by: Lucas De Marchi <lucas.demarchi@...el.com>
> Acked-by: Jani Nikula <jani.nikula@...el.com>
> Co-developed-by: Vincent Mailhol <mailhol.vincent@...adoo.fr>
> Signed-off-by: Vincent Mailhol <mailhol.vincent@...adoo.fr>
> ---
> Changelog:
>
> v5 -> v6:
>
> - No changes.
>
> v4 -> v5:
>
> - Rename GENMASK_t() to GENMASK_TYPE().
>
> - Fix typo in patch description.
>
> - Use tab indentations instead of single space to separate the
> macro name from its body.
>
> - s/__GENMASK_U*()/GENMASK_U*()/g in the comment.
>
> - Add a tag to credit myself as Co-developer. Keep Yury as the
> main author.
>
> - Modify GENMASK_TYPE() to match the changes made to __GENMASK()
> in: https://github.com/norov/linux/commit/1e7933a575ed
>
> - Replace (t)~_ULL(0) with type_max(t). This is OK because
> GENMASK_TYPE() is not available in asm.
>
> - linux/const.h and asm/bitsperlong.h are not used anymore. Remove
> them.
>
> - Apply GENMASK_TYPE() to GENMASK_U128().
>
> - Remove the unsigned int cast for the U8 and U16 variants. Cast
> to the target type instead. Do that cast directly in
> GENMASK_TYPE().
>
> v3 -> v4:
>
> - The v3 is one year old. Meanwhile people started using
> __GENMASK() directly. So instead of generalizing __GENMASK() to
> support different types, add a new GENMASK_t().
>
> - replace ~0ULL by ~_ULL(0). Otherwise, GENMASK_t() would fail in
> asm code.
>
> - Make GENMASK_U8() and GENMASK_U16() return an unsigned int. In
> v3, due to the integer promotion rules, these were returning a
> signed integer. By casting these to unsigned int, at least the
> signedness is kept.
> ---
> include/linux/bitops.h | 1 -
> include/linux/bits.h | 55 ++++++++++++++++++++++++++++++++------------------
> 2 files changed, 35 insertions(+), 21 deletions(-)
>
> diff --git a/include/linux/bitops.h b/include/linux/bitops.h
> index c1cb53cf2f0f8662ed3e324578f74330e63f935d..9be2d50da09a417966b3d11c84092bb2f4cd0bef 100644
> --- a/include/linux/bitops.h
> +++ b/include/linux/bitops.h
> @@ -8,7 +8,6 @@
>
> #include <uapi/linux/kernel.h>
>
> -#define BITS_PER_TYPE(type) (sizeof(type) * BITS_PER_BYTE)
> #define BITS_TO_LONGS(nr) __KERNEL_DIV_ROUND_UP(nr, BITS_PER_TYPE(long))
> #define BITS_TO_U64(nr) __KERNEL_DIV_ROUND_UP(nr, BITS_PER_TYPE(u64))
> #define BITS_TO_U32(nr) __KERNEL_DIV_ROUND_UP(nr, BITS_PER_TYPE(u32))
> diff --git a/include/linux/bits.h b/include/linux/bits.h
> index 9c1c7ce0bba6bb09490d891904c143a5394fd512..b690611c769be61ab2b5ced43c8302ba5693308b 100644
> --- a/include/linux/bits.h
> +++ b/include/linux/bits.h
> @@ -2,16 +2,15 @@
> #ifndef __LINUX_BITS_H
> #define __LINUX_BITS_H
>
> -#include <linux/const.h>
> #include <vdso/bits.h>
> #include <uapi/linux/bits.h>
> -#include <asm/bitsperlong.h>
>
> #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))
> #define BIT_ULL_WORD(nr) ((nr) / BITS_PER_LONG_LONG)
> #define BITS_PER_BYTE 8
> +#define BITS_PER_TYPE(type) (sizeof(type) * BITS_PER_BYTE)
>
> /*
> * Create a contiguous bitmask starting at bit position @l and ending at
> @@ -20,28 +19,44 @@
> */
> #if !defined(__ASSEMBLY__)
>
> -#include <linux/build_bug.h>
> -#include <linux/compiler.h>
> -
> -#define GENMASK_INPUT_CHECK(h, l) BUILD_BUG_ON_ZERO(const_true((l) > (h)))
> -
> -#define GENMASK(h, l) \
> - (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
> -#define GENMASK_ULL(h, l) \
> - (GENMASK_INPUT_CHECK(h, l) + __GENMASK_ULL(h, l))
> -
> /*
> * Missing asm support
> *
> - * __GENMASK_U128() depends on _BIT128() which would not work
> - * in the asm code, as it shifts an 'unsigned __int128' data
> - * type instead of direct representation of 128 bit constants
> - * such as long and unsigned long. The fundamental problem is
> - * that a 128 bit constant will get silently truncated by the
> - * gcc compiler.
> + * GENMASK_U*() depends on BITS_PER_TYPE() which relies on sizeof(),
> + * something not available in asm. Nethertheless, fixed width integers
> + * is a C concept. Assembly code can rely on the long and long long
> + * versions instead.
> */
> -#define GENMASK_U128(h, l) \
> - (GENMASK_INPUT_CHECK(h, l) + __GENMASK_U128(h, l))
> +
> +#include <linux/build_bug.h>
> +#include <linux/compiler.h>
> +#include <linux/overflow.h>
> +
> +#define GENMASK_INPUT_CHECK(h, l) BUILD_BUG_ON_ZERO(const_true((l) > (h)))
> +
> +/*
> + * 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_TYPE(t, h, l) \
> + ((t)(GENMASK_INPUT_CHECK(h, l) + \
> + (type_max(t) << (l) & \
> + type_max(t) >> (BITS_PER_TYPE(t) - 1 - (h)))))
> +
> +#define GENMASK(h, l) GENMASK_TYPE(unsigned long, h, l)
> +#define GENMASK_ULL(h, l) GENMASK_TYPE(unsigned long long, h, l)
I like everything except this part. We switch GENMASK() from a well
tested implementation, including an asm code, and we split uapi and
non-uapi users, with no functionality changes.
Unification is a solid point, however.
Let's make it a 2-step procedure? Adding fixed-width GENMASKs is a
non-questionable improvement. Switching an existing API from one
implementation to another should be a separate patch, and probably
even a separate series. And we should be very clear that __GENMASK()
is uapi-only thing from now.
If we decide to switch GENMASK() in a separate series, we'll have some
extra time to think about unification...
> +#define GENMASK_U8(h, l) GENMASK_TYPE(u8, h, l)
> +#define GENMASK_U16(h, l) GENMASK_TYPE(u16, h, l)
> +#define GENMASK_U32(h, l) GENMASK_TYPE(u32, h, l)
> +#define GENMASK_U64(h, l) GENMASK_TYPE(u64, h, l)
> +#define GENMASK_U128(h, l) GENMASK_TYPE(u128, h, l)
>
> #else /* defined(__ASSEMBLY__) */
>
>
> --
> 2.45.3
>
Powered by blists - more mailing lists