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

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

[Yury Norov]

+ Catalin Marinas, ARM maillist

Hi Catalin and everyone,

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

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

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

Thanks,
Yury

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

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

[Anshuman Khandual]

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

Hello Yury,

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

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

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

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

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

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

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

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

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

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

No, not atleast right now.

These were moved into uapi subsequently via the following commit.

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

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

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

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

[Anshuman Khandual]

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

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

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

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

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

[Yury Norov]

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

Ok, thanks.

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

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

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

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

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

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

Vincent,

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

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

Thanks,
Yury

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

[Vincent Mailhol]

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

(...)

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

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

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

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

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

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



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

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

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

  _BIT128(127) << 1

which expands to:

  (unsigned __int128)1 << 127 << 1

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


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

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

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

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

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

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


Yours sincerely,
Vincent Mailhol