Re: [PATCH v5 1/7] bits: split the definition of the asm and non-asm GENMASK()

Vincent Mailhol
2025-03-07T15:50:33Z

On 07/03/2025 at 22:27, David Laight wrote:
> On Fri, 7 Mar 2025 18:58:08 +0900
> Vincent Mailhol <mailhol.vincent@wanadoo.fr> wrote:
>
>> On 07/03/2025 at 04:23, David Laight wrote:
>>> On Thu, 06 Mar 2025 20:29:52 +0900
>>> Vincent Mailhol via B4 Relay <devnull+mailhol.vincent.wanadoo.fr@kernel.org> wrote:
>>>
>>>> From: Vincent Mailhol <mailhol.vincent@wanadoo.fr>

(...)

>>>> +#define GENMASK(h, l) __GENMASK(h, l)
>>>> +#define GENMASK_ULL(h, l) __GENMASK_ULL(h, l)
>>>
>>> What do those actually expand to now?
>>> As I've said a few times both UL(0) and ULL(0) are just (0) for __ASSEMBLY__
>>> so the expansions of __GENMASK() and __GENMASK_ULL() contained the
>>> same numeric constants.
>>
>> Indeed, in asm, the UL(0) and ULL(0) expands to the same thing: 0.
>>
>> But the two macros still expand to something different on 32 bits
>> architectures:
>>
>> * __GENMASK:
>>
>> (((~(0)) << (l)) & (~(0) >> (32 - 1 - (h))))
>>
>> * __GENMASK_ULL:
>>
>> (((~(0)) << (l)) & (~(0) >> (64 - 1 - (h))))
>>
>> On 64 bits architecture these are the same.
>
> If the assembler is naive and uses the cpu shift instruction for the >>
> then a lot of cpu (including all x86 since the 286) mask off the high bits.
> So __GENMASK_ULL() may well generate the expected pattern - provided it
> is 32bits wide.

"If", "may", that's still a lot of conditionals in your sentence :)

I do not have enough knowledge to prove or disprove this, but what I am
sure of is that this uncertainty tells me that this is not something I
want to touch myself.

I picked up this stalled fixed width patch series and re-spinned it
because I had confidence here. I do not want to extend this series with
some asm clean-up which looks uncertain to me. I am not telling you are
wrong but just that I will happily delegate this to whoever has more
confidence than me!

>>> This means they should be generating the same values.
>>> I don't know the correct 'sizeof (int_type)' for the shift right of ~0.
>>> My suspicion is that a 32bit assembler used 32bit signed integers and a
>>> 64bit one 64bit signed integers (but a 32bit asm on a 64bit host might
>>> be 64bit).
>>> So the asm versions need to avoid the right shift and only do left shifts.
>>>
>>> Which probably means they need to be enirely separate from the C versions.
>>> And then the C ones can have all the ULL() removed.
>>
>> In this v5, I already have the ULL() removed from the non-uapi C
>> version. And we are left with two distinct variants:
>>
>> - the uapi C & asm
>> - the non-uapi C (including fix width)
>>
>> For the uapi ones, I do not think we can modify it without a risk of
>> breaking some random userland. At least, this is not a risk I will take.
>> And if we have to keep the __GENMASK() and __GENMASK_ULL(), then I would
>> rather just reuse these for the asm variant instead of splitting further
>> more and finding ourselves with three variants:
>>
>> - the uapi C
>> - the asm
>> - the non-uapi C (including fix width)
>>
>> If __GENMASK() and __GENMASK_ULL() were not in the uapi, I would have
>> agreed with you.
>>
>> If you believe that the risk of modifying the uapi GENMASK*() is low
>> enough, then you can submit a patch. But I will definitely not touch
>> these myself.
>
> I don't think you'll break userspace by stopping the uapi .h working
> for asm files (with __ASSEMBLER__ defined).
>
> But someone else might have a different opinion.

How do you know that there isn't someone somewhere who is using the uapi
__GENMASK*() in their userland asm code? Wouldn't such code break?

You may argue that the uapi is not meant to be used that way, but at the
end, we are not supposed to make assumptions on how the uapi code will
be used. Once it is exposed, if something break because the uapi was not
used in the intended way, it is still the kernel fault, not the userland.

Yours sincerely,
Vincent Mailhol