| Message ID | 20250306-fixed-type-genmasks-v5-1-b443e9dcba63@wanadoo.fr (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | bits: Fixed-type GENMASK()/BIT() | expand |
On Thu, Mar 06, 2025 at 08:29:52PM +0900, Vincent Mailhol via B4 Relay 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. ... > -/* > - * BUILD_BUG_ON_ZERO is not available in h files included from asm files, > - * disable the input check if that is the case. > - */ I believe this comment is still valid... > +#else /* defined(__ASSEMBLY__) */ ...here. Otherwise justify its removal in the commit message. > +#define GENMASK(h, l) __GENMASK(h, l) > +#define GENMASK_ULL(h, l) __GENMASK_ULL(h, l) > + > +#endif /* !defined(__ASSEMBLY__) */
On Thu, Mar 06, 2025 at 08:29:52PM +0900, Vincent Mailhol via B4 Relay 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. > >Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr> >--- >Changelog: > > v4 -> v5: > > - Use tab indentations instead of single space to separate the > macro name from its body. > > v3 -> v4: > > - New patch in the series >--- > include/linux/bits.h | 18 +++++++++--------- > 1 file changed, 9 insertions(+), 9 deletions(-) > >diff --git a/include/linux/bits.h b/include/linux/bits.h >index 14fd0ca9a6cd17339dd2f69e449558312a8a001b..4819cbe7bd48fbae796fc6005c9f92b1c93a034c 100644 >--- a/include/linux/bits.h >+++ b/include/linux/bits.h >@@ -19,23 +19,17 @@ > * GENMASK_ULL(39, 21) gives us the 64bit vector 0x000000ffffe00000. > */ > #if !defined(__ASSEMBLY__) >+ > #include <linux/build_bug.h> > #include <linux/compiler.h> >+ > #define GENMASK_INPUT_CHECK(h, l) BUILD_BUG_ON_ZERO(const_true((l) > (h))) >-#else >-/* >- * BUILD_BUG_ON_ZERO is not available in h files included from asm files, >- * disable the input check if that is the case. >- */ it seems we now have 1 inconsistency that we comment why GENMASK_U128() is not available in asm, but we don't comment why GENMASK_INPUT_CHECK() is not available there. Maybe move this comment on top of GENMASK_INPUT_CHECK(). Anyway, Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com> thanks for picking up this series. Lucas De Marchi >-#define GENMASK_INPUT_CHECK(h, l) 0 >-#endif > > #define GENMASK(h, l) \ > (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l)) > #define GENMASK_ULL(h, l) \ > (GENMASK_INPUT_CHECK(h, l) + __GENMASK_ULL(h, l)) > >-#if !defined(__ASSEMBLY__) > /* > * Missing asm support > * >@@ -48,6 +42,12 @@ > */ > #define GENMASK_U128(h, l) \ > (GENMASK_INPUT_CHECK(h, l) + __GENMASK_U128(h, l)) >-#endif >+ >+#else /* defined(__ASSEMBLY__) */ >+ >+#define GENMASK(h, l) __GENMASK(h, l) >+#define GENMASK_ULL(h, l) __GENMASK_ULL(h, l) >+ >+#endif /* !defined(__ASSEMBLY__) */ > > #endif /* __LINUX_BITS_H */ > >-- >2.45.3 > >
On 06/03/2025 at 22:05, Andy Shevchenko wrote: > On Thu, Mar 06, 2025 at 08:29:52PM +0900, Vincent Mailhol via B4 Relay 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. > > ... > >> -/* >> - * BUILD_BUG_ON_ZERO is not available in h files included from asm files, >> - * disable the input check if that is the case. >> - */ > > I believe this comment is still valid... > >> +#else /* defined(__ASSEMBLY__) */ > > > ...here. > > Otherwise justify its removal in the commit message. OK. I will restore the comment in v6, but will move it to the #else branch, like this: #else /* defined(__ASSEMBLY__) */ /* * BUILD_BUG_ON_ZERO is not available in h files included from asm * files, so no input checks in assembly. */ #define GENMASK(h, l) __GENMASK(h, l) #define GENMASK_ULL(h, l) __GENMASK_ULL(h, l) #endif /* !defined(__ASSEMBLY__) */ >> +#define GENMASK(h, l) __GENMASK(h, l) >> +#define GENMASK_ULL(h, l) __GENMASK_ULL(h, l) >> + >> +#endif /* !defined(__ASSEMBLY__) */ > Yours sincerely, Vincent Mailhol
On 06/03/2025 at 23:34, Lucas De Marchi wrote: > On Thu, Mar 06, 2025 at 08:29:52PM +0900, Vincent Mailhol via B4 Relay > wrote: (...) > it seems we now have 1 inconsistency that we comment why > GENMASK_U128() is not available in asm, but we don't comment why > GENMASK_INPUT_CHECK() is not available there. Maybe move this comment on > top of GENMASK_INPUT_CHECK(). I will restore the comment in v6 and put it next to the asm definition, c.f. my reply to Andy. > Anyway, > > Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com> Is this only valid for the first patch or for the full series? If this is for the full series, would you mind replying to the cover letter with your review tag? > thanks for picking up this series. You are welcome! > Lucas De Marchi (...) Yours sincerely, Vincent Mailhol
On Fri, Mar 07, 2025 at 12:07:45AM +0900, Vincent Mailhol wrote: > On 06/03/2025 at 22:05, Andy Shevchenko wrote: > > On Thu, Mar 06, 2025 at 08:29:52PM +0900, Vincent Mailhol via B4 Relay wrote: > >> From: Vincent Mailhol <mailhol.vincent@wanadoo.fr> ... > >> -/* > >> - * BUILD_BUG_ON_ZERO is not available in h files included from asm files, > >> - * disable the input check if that is the case. > >> - */ > > > > I believe this comment is still valid... > > > >> +#else /* defined(__ASSEMBLY__) */ > > > > > > ...here. > > > > Otherwise justify its removal in the commit message. > > OK. I will restore the comment in v6, but will move it to the #else > branch, Isn't it what I suggested? :-) > like this: > #else /* defined(__ASSEMBLY__) */ > > /* > * BUILD_BUG_ON_ZERO is not available in h files included from asm > * files, so no input checks in assembly. > */ > #define GENMASK(h, l) __GENMASK(h, l) > #define GENMASK_ULL(h, l) __GENMASK_ULL(h, l) > > #endif /* !defined(__ASSEMBLY__) */ > > >> +#define GENMASK(h, l) __GENMASK(h, l) > >> +#define GENMASK_ULL(h, l) __GENMASK_ULL(h, l) > >> + > >> +#endif /* !defined(__ASSEMBLY__) */
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. 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. David > + > +#endif /* !defined(__ASSEMBLY__) */ > > #endif /* __LINUX_BITS_H */ >
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. > 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. Yours sincerely, Vincent Mailhol
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 >
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
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. I've just fed those into godbolt (https://www.godbolt.org/z/Ter6WE9qE) as: int fi(void) { int v; asm("mov $(((~(0)) << (8)) & (~(0) >> (32 - 1 - (15)))),%0": "=r" (v)); return v -(((~(0u)) << (8)) & (~(0u) >> (32 - 1 - (15)))); } gas warns: <source>:4: Warning: 0xffffffffff00 shortened to 0xffffff00 unsigned long long fll(void) { unsigned long long v; asm("mov $(((~(0)) << (8)) & (~(0) >> (64 - 1 - (15)))),%0": "=r" (v)); return v -(((~(0ull)) << (8)) & (~(0ull) >> (64 - 1 - (15)))); } (for other architectures you'll need to change the opcode) For x86 and x86-32 the assembler seems to be doing 64bit maths with unsigned right shifts - so the second function (with the 64 in it) generates 0xff00. I doubt a 32bit only assembler does 64bit maths, but the '>> 48' above might get masked to a '>> 16' by the cpu and generate the correct result. So __GENMASK() is likely to be broken for any assembler that supports 64bits when generating 32bit code. __GENMASK_ULL() works (assuming all have unsigned >>) on 64bit assemblers (even when generating 32bit code). It may work on some 32bit assemblers. Since most uses in the header files will be GENMASK() I doubt (hope) no asm code actually uses the values! The headers assemble - but that is about all that can be said. Bags of worms :-) David
On Sun, 9 Mar 2025 01:58:53 +0000 David Laight <david.laight.linux@gmail.com> 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> > > >> > > >> 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. > > I've just fed those into godbolt (https://www.godbolt.org/z/Ter6WE9qE) as: > int fi(void) > { > int v; > asm("mov $(((~(0)) << (8)) & (~(0) >> (32 - 1 - (15)))),%0": "=r" (v)); > return v -(((~(0u)) << (8)) & (~(0u) >> (32 - 1 - (15)))); > } > > gas warns: > <source>:4: Warning: 0xffffffffff00 shortened to 0xffffff00 > > unsigned long long fll(void) > { > unsigned long long v; > asm("mov $(((~(0)) << (8)) & (~(0) >> (64 - 1 - (15)))),%0": "=r" (v)); > return v -(((~(0ull)) << (8)) & (~(0ull) >> (64 - 1 - (15)))); > } > > (for other architectures you'll need to change the opcode) > > For x86 and x86-32 the assembler seems to be doing 64bit maths with unsigned > right shifts - so the second function (with the 64 in it) generates 0xff00. > I doubt a 32bit only assembler does 64bit maths, but the '>> 48' above > might get masked to a '>> 16' by the cpu and generate the correct result. > > So __GENMASK() is likely to be broken for any assembler that supports 64bits > when generating 32bit code. > __GENMASK_ULL() works (assuming all have unsigned >>) on 64bit assemblers > (even when generating 32bit code). It may work on some 32bit assemblers. I've remembered my 'pi' has a 32bit userspace (on a 64bit kernel). I quick test of "mov %0,#(...)" and bits 11..8 gives the correct output for size '32' but the error message: /tmp/ccPB7bWh.s:26: Warning: shift count out of range (56 is not between 0 and 31) with size '64'. Assuming that part of the gnu assembler is consistent across architectures you can't use either GENMASK in asm for 32bit architectures. Any change (probably including removing the asm support for the uapi) isn't going to make things worse! David > > Since most uses in the header files will be GENMASK() I doubt (hope) no > asm code actually uses the values! > The headers assemble - but that is about all that can be said. > > Bags of worms :-) > > David >
On 09/03/2025 at 19:23, David Laight wrote: > On Sun, 9 Mar 2025 01:58:53 +0000 > David Laight <david.laight.linux@gmail.com> 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> >>>>> >>>>> 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. >> >> I've just fed those into godbolt (https://www.godbolt.org/z/Ter6WE9qE) as: >> int fi(void) >> { >> int v; >> asm("mov $(((~(0)) << (8)) & (~(0) >> (32 - 1 - (15)))),%0": "=r" (v)); >> return v -(((~(0u)) << (8)) & (~(0u) >> (32 - 1 - (15)))); >> } >> >> gas warns: >> <source>:4: Warning: 0xffffffffff00 shortened to 0xffffff00 >> >> unsigned long long fll(void) >> { >> unsigned long long v; >> asm("mov $(((~(0)) << (8)) & (~(0) >> (64 - 1 - (15)))),%0": "=r" (v)); >> return v -(((~(0ull)) << (8)) & (~(0ull) >> (64 - 1 - (15)))); >> } >> >> (for other architectures you'll need to change the opcode) >> >> For x86 and x86-32 the assembler seems to be doing 64bit maths with unsigned >> right shifts - so the second function (with the 64 in it) generates 0xff00. >> I doubt a 32bit only assembler does 64bit maths, but the '>> 48' above >> might get masked to a '>> 16' by the cpu and generate the correct result. >> >> So __GENMASK() is likely to be broken for any assembler that supports 64bits >> when generating 32bit code. >> __GENMASK_ULL() works (assuming all have unsigned >>) on 64bit assemblers >> (even when generating 32bit code). It may work on some 32bit assemblers.> > I've remembered my 'pi' has a 32bit userspace (on a 64bit kernel). > I quick test of "mov %0,#(...)" and bits 11..8 gives the correct output > for size '32' but the error message: > /tmp/ccPB7bWh.s:26: Warning: shift count out of range (56 is not between 0 and 31) > with size '64'. > > Assuming that part of the gnu assembler is consistent across architectures > you can't use either GENMASK in asm for 32bit architectures. > > Any change (probably including removing the asm support for the uapi) isn't > going to make things worse! > > David > >> >> Since most uses in the header files will be GENMASK() I doubt (hope) no >> asm code actually uses the values! After reading your message, I wanted to understand where these GENMASK*() were used in asm code. It happens that most of the architectures simply don't use it! I see these are using in aarch64, but that's about it. I couldn't find any use of neither GENMASK() nor GENMASK_ULL() in x86, arm 32 bits, m68k or powerpc asm code (I did not check the other architectures). aarch64 uses both the long and long long variants, but this being a 64 bits architecture, these are the same. So OK. So, this goes into the same direction as you: I guess that the fact that no one noticed the issue is that no one uses this on a 32 bits arch, probably for historical reasons, i.e. the asm code was written before the introduction of the GENMASK() macros. >> The headers assemble - but that is about all that can be said. >> >> Bags of worms :-) +1 (I will not open that bag) I think that the asm and non asm variant should have used a different implementation from the begining. By wanting to have a single variant that fit both the asm and the C code, we have a complex result that is hard to understand and maintain (c.f. the bug which you pointed out which have been unnoticed for ever). But now that it is in the uapi, I am not sure of what is the best approach. I sincerely hope that we can just modify it with no userland impact. Well, just to make it clear, I am not touching the asm part. I acknowledge the issue, but I do not think that I have the skill to fix it. Yours sincerely, Vincent Mailhol
diff --git a/include/linux/bits.h b/include/linux/bits.h index 14fd0ca9a6cd17339dd2f69e449558312a8a001b..4819cbe7bd48fbae796fc6005c9f92b1c93a034c 100644 --- a/include/linux/bits.h +++ b/include/linux/bits.h @@ -19,23 +19,17 @@ * GENMASK_ULL(39, 21) gives us the 64bit vector 0x000000ffffe00000. */ #if !defined(__ASSEMBLY__) + #include <linux/build_bug.h> #include <linux/compiler.h> + #define GENMASK_INPUT_CHECK(h, l) BUILD_BUG_ON_ZERO(const_true((l) > (h))) -#else -/* - * BUILD_BUG_ON_ZERO is not available in h files included from asm files, - * disable the input check if that is the case. - */ -#define GENMASK_INPUT_CHECK(h, l) 0 -#endif #define GENMASK(h, l) \ (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l)) #define GENMASK_ULL(h, l) \ (GENMASK_INPUT_CHECK(h, l) + __GENMASK_ULL(h, l)) -#if !defined(__ASSEMBLY__) /* * Missing asm support * @@ -48,6 +42,12 @@ */ #define GENMASK_U128(h, l) \ (GENMASK_INPUT_CHECK(h, l) + __GENMASK_U128(h, l)) -#endif + +#else /* defined(__ASSEMBLY__) */ + +#define GENMASK(h, l) __GENMASK(h, l) +#define GENMASK_ULL(h, l) __GENMASK_ULL(h, l) + +#endif /* !defined(__ASSEMBLY__) */ #endif /* __LINUX_BITS_H */