Re: [PATCH V3 1/2] uapi: Define GENMASK_U128

[Anshuman Khandual <anshuman.khandual@arm.com>]

On 8/17/24 19:27, Yury Norov wrote:
> On Fri, Aug 16, 2024 at 11:58:04AM +0530, Anshuman Khandual wrote:
>>
>>
>> On 8/1/24 12:46, Anshuman Khandual wrote:
>>> This adds GENMASK_U128() and __GENMASK_U128() macros using __BITS_PER_U128
>>> and __int128 data types. These macros will be used in providing support for
>>> generating 128 bit masks.
>>>
>>> Cc: Yury Norov <yury.norov@gmail.com>
>>> Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk>
>>> Cc: Arnd Bergmann <arnd@arndb.de>>
>>> Cc: linux-kernel@vger.kernel.org
>>> Cc: linux-arch@vger.kernel.org
>>> Reviewed-by: Arnd Bergmann <arnd@arndb.de>
>>> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
>>> ---
>>>  include/linux/bits.h       | 13 +++++++++++++
>>>  include/uapi/linux/bits.h  |  3 +++
>>>  include/uapi/linux/const.h | 15 +++++++++++++++
>>>  3 files changed, 31 insertions(+)
>>>
>>> diff --git a/include/linux/bits.h b/include/linux/bits.h
>>> index 0eb24d21aac2..bf99feb5570e 100644
>>> --- a/include/linux/bits.h
>>> +++ b/include/linux/bits.h
>>> @@ -36,4 +36,17 @@
>>>  #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 __init128' 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.
>>> + */
>>> +#define GENMASK_U128(h, l) \
>>> +	(GENMASK_INPUT_CHECK(h, l) + __GENMASK_U128(h, l))
>>> +
>>>  #endif	/* __LINUX_BITS_H */
>>> diff --git a/include/uapi/linux/bits.h b/include/uapi/linux/bits.h
>>> index 3c2a101986a3..4d4b7b08003c 100644
>>> --- a/include/uapi/linux/bits.h
>>> +++ b/include/uapi/linux/bits.h
>>> @@ -12,4 +12,7 @@
>>>          (((~_ULL(0)) - (_ULL(1) << (l)) + 1) & \
>>>           (~_ULL(0) >> (__BITS_PER_LONG_LONG - 1 - (h))))
>>>  
>>> +#define __GENMASK_U128(h, l) \
>>> +	((_BIT128((h) + 1)) - (_BIT128(l)))
>>> +
>>>  #endif /* _UAPI_LINUX_BITS_H */
>>> diff --git a/include/uapi/linux/const.h b/include/uapi/linux/const.h
>>> index a429381e7ca5..5be12e8f8f9c 100644
>>> --- a/include/uapi/linux/const.h
>>> +++ b/include/uapi/linux/const.h
>>> @@ -28,6 +28,21 @@
>>>  #define _BITUL(x)	(_UL(1) << (x))
>>>  #define _BITULL(x)	(_ULL(1) << (x))
>>>  
>>> +/*
>>> + * Missing asm support
>>> + *
>>> + * __BIT128() would not work in the asm code, as it shifts an
>>> + * 'unsigned __init128' data type as direct representation of
>>> + * 128 bit constants is not supported in the gcc compiler, as
>>> + * they get silently truncated.
>>> + *
>>> + * TODO: Please revisit this implementation when gcc compiler
>>> + * starts representing 128 bit constants directly like long
>>> + * and unsigned long etc. Subsequently drop the comment for
>>> + * GENMASK_U128() which would then start supporting asm code.
>>> + */
>>> +#define _BIT128(x)	((unsigned __int128)(1) << (x))
>>> +
>>>  #define __ALIGN_KERNEL(x, a)		__ALIGN_KERNEL_MASK(x, (__typeof__(x))(a) - 1)
>>>  #define __ALIGN_KERNEL_MASK(x, mask)	(((x) + (mask)) & ~(mask))
>>>  
>>
>> Hello Yuri/Arnd,
>>
>> This proposed GENMASK_U128(h, l) warns during build when the higher end
>> bit is 127 (which in itself is a valid input).
>>
>> ./include/uapi/linux/const.h:45:44: warning: left shift count >= width of type [-Wshift-count-overflow]
>>    45 | #define _BIT128(x) ((unsigned __int128)(1) << (x))
>>       |                                            ^~
>> ./include/asm-generic/bug.h:123:25: note: in definition of macro ‘WARN_ON’
>>   123 |  int __ret_warn_on = !!(condition);    \
>>       |                         ^~~~~~~~~
>> ./include/uapi/linux/bits.h:16:4: note: in expansion of macro ‘_BIT128’
>>    16 |  ((_BIT128((h) + 1)) - (_BIT128(l)))
>>       |    ^~~~~~~
>> ./include/linux/bits.h:51:31: note: in expansion of macro ‘__GENMASK_U128’
>>    51 |  (GENMASK_INPUT_CHECK(h, l) + __GENMASK_U128(h, l))
>>       |                               ^~~~~~~~~~~~~~
>>
>> This is caused by ((unsigned __int128)(1) << (128)) which is generated
>> via (h + 1) element in __GENMASK_U128().
>>
>> #define _BIT128(x)	((unsigned __int128)(1) << (x))
>> #define __GENMASK_U128(h, l) \
>> 	((_BIT128((h) + 1)) - (_BIT128(l)))
>>
>> Adding some extra tests in lib/test_bits.c exposes this build problem,
>> although it does not fail these new tests.
>>
>> [    1.719221]     # Subtest: bits-test
>> [    1.719291]     # module: test_bits
>> [    1.720522]     ok 1 genmask_test
>> [    1.721570]     ok 2 genmask_ull_test
>> [    1.722668]     ok 3 genmask_u128_test
>> [    1.723760]     ok 4 genmask_input_check_test
>> [    1.723909] # bits-test: pass:4 fail:0 skip:0 total:4
>> [    1.724101] ok 1 bits-test
>>
>> diff --git a/lib/test_bits.c b/lib/test_bits.c
>> index d3d858b24e02..7a972edc7122 100644
>> --- a/lib/test_bits.c
>> +++ b/lib/test_bits.c
>> @@ -49,6 +49,8 @@ static void genmask_u128_test(struct kunit *test)
>>         KUNIT_EXPECT_EQ(test, 0xffffffffffffffffULL, GENMASK_U128(63, 0));
>>         KUNIT_EXPECT_EQ(test, 0xffffffffffffffffULL, GENMASK_U128(64, 0) >> 1);
>>         KUNIT_EXPECT_EQ(test, 0x00000000ffffffffULL, GENMASK_U128(81, 50) >> 50);
>> +       KUNIT_EXPECT_EQ(test, 0x0000000000000003ULL, GENMASK_U128(127, 126) >> 126);
>> +       KUNIT_EXPECT_EQ(test, 0x0000000000000001ULL, GENMASK_U128(127, 127) >> 127);
>>
>> The most significant bit in the generate mask can be added separately
>> , thus voiding that extra shift. The following patch solves the build
>> problem.
>>
>> diff --git a/include/uapi/linux/bits.h b/include/uapi/linux/bits.h
>> index 4d4b7b08003c..4e50f635c6d9 100644
>> --- a/include/uapi/linux/bits.h
>> +++ b/include/uapi/linux/bits.h
>> @@ -13,6 +13,6 @@
>>           (~_ULL(0) >> (__BITS_PER_LONG_LONG - 1 - (h))))
>>  
>>  #define __GENMASK_U128(h, l) \
>> -       ((_BIT128((h) + 1)) - (_BIT128(l)))
>> +       (((_BIT128(h)) - (_BIT128(l))) | (_BIT128(h)))
> 
> Can you send v3 with the fix? I will drop this series from bitmap-for-next
> meanwhile.

Sure, will send out V4 (current series being V3).

> 
> Can you also extend the test for more? I'd like to check for example
> the (127, 0) range. Also, can you please check both HI and LO parts 
> the mask?

Following tests form the complete set on GENMASK_U128(). The last four tests
here will be added in V4. Please also note that only 64 bit mask portion can
be tested at once.

        KUNIT_EXPECT_EQ(test, 0x0000000000ff0000ULL, GENMASK_U128(87, 80) >> 64);
        KUNIT_EXPECT_EQ(test, 0x0000000000ffffffULL, GENMASK_U128(87, 64) >> 64);
        KUNIT_EXPECT_EQ(test, 0x0000000000000001ULL, GENMASK_U128(0, 0));
        KUNIT_EXPECT_EQ(test, 0xffffffffffffffffULL, GENMASK_U128(63, 0));
        KUNIT_EXPECT_EQ(test, 0xffffffffffffffffULL, GENMASK_U128(64, 0) >> 1);
        KUNIT_EXPECT_EQ(test, 0x00000000ffffffffULL, GENMASK_U128(81, 50) >> 50);
        KUNIT_EXPECT_EQ(test, 0x0000000000000003ULL, GENMASK_U128(127, 126) >> 126);
        KUNIT_EXPECT_EQ(test, 0x0000000000000001ULL, GENMASK_U128(127, 127) >> 127);
        KUNIT_EXPECT_EQ(test, 0xffffffffffffffffULL, GENMASK_U128(127, 0) >> 64);
        KUNIT_EXPECT_EQ(test, 0xffffffffffffffffULL, GENMASK_U128(127, 0) & ~GENMASK_U128(127, 64));

Although, please do let me know if you would like to add some more tests.

> 
> For the v3, please share the link to the following series that
> actually uses new API.

Sure, will add the following link pointing to the work in progress on arm64.

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