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@xxxxxxxxxx/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@xxxxxxxxxx/
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
