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

[David Laight <david.laight.linux@gmail.com>]

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>
> >>
> >> In an upcoming change, GENMASK() and its friends will indirectly
> >> depend on sizeof() which is not available in asm.
> >>
> >> Instead of adding further complexity to __GENMASK() to make it work
> >> for both asm and non asm, just split the definition of the two
> >> variants.  
> > ...  
> >> +#else /* defined(__ASSEMBLY__) */
> >> +
> >> +#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.

> 
> > 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.

> 
> 
> Yours sincerely,
> Vincent Mailhol
>