On Fri, 7 Mar 2025 18:58:08 +0900 Vincent Mailhol <mailhol.vincent@xxxxxxxxxx> 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@xxxxxxxxxx> wrote: > > > >> From: Vincent Mailhol <mailhol.vincent@xxxxxxxxxx> > >> > >> 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 >
