[PATCH 1/1] bitfield.h: Ensure FIELD_PREP_CONST() is constant

[PATCH 1/1] bitfield.h: Ensure FIELD_PREP_CONST() is constant

[david.laight.linux]

From: David Laight <david.laight.linux@gmail.com>

Some versions of gcc report that some expressions involving
__builtin_ffsll(1ull << 63) are not integer constant expressions.

Rework FIELD_PREP_CONST() to avoid the issue.

Signed-off-by: David Laight <david.laight.linux@gmail.com>
---

Note that when 'val' is a variable 'val << constant' is likely
to execute faster than 'val * (1 << constant)'.
So the normal FIELD_PREP() is best left alone.

 include/linux/bitfield.h | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h
index 54aeeef1f0ec..f21765851d5c 100644
--- a/include/linux/bitfield.h
+++ b/include/linux/bitfield.h
@@ -45,6 +45,7 @@
  */
 
 #define __bf_shf(x) (__builtin_ffsll(x) - 1)
+#define __bf_low_bit(mask) ((mask) & (~(mask) + 1))
 
 #define __scalar_type_to_unsigned_cases(type)				\
 		unsigned type:	(unsigned type)0,			\
@@ -138,8 +139,6 @@
 		__FIELD_PREP(_mask, _val, "FIELD_PREP: ");		\
 	})
 
-#define __BF_CHECK_POW2(n)	BUILD_BUG_ON_ZERO(((n) & ((n) - 1)) != 0)
-
 /**
  * FIELD_PREP_CONST() - prepare a constant bitfield element
  * @_mask: shifted mask defining the field's length and position
@@ -157,11 +156,11 @@
 		/* mask must be non-zero */				\
 		BUILD_BUG_ON_ZERO((_mask) == 0) +			\
 		/* check if value fits */				\
-		BUILD_BUG_ON_ZERO(~((_mask) >> __bf_shf(_mask)) & (_val)) + \
+		BUILD_BUG_ON_ZERO(~((_mask) / __bf_low_bit(_mask)) & (_val)) + \
 		/* check if mask is contiguous */			\
-		__BF_CHECK_POW2((_mask) + (1ULL << __bf_shf(_mask))) +	\
+		BUILD_BUG_ON_ZERO((_mask) & ((_mask) + __bf_low_bit(_mask))) + \
 		/* and create the value */				\
-		(((typeof(_mask))(_val) << __bf_shf(_mask)) & (_mask))	\
+		(((_val) * __bf_low_bit(_mask)) & (_mask))		\
 	)
 
 /**
-- 
2.39.5

Re: [PATCH 1/1] bitfield.h: Ensure FIELD_PREP_CONST() is constant

[Yury Norov]

+ Matt Coster

On Fri, Apr 10, 2026 at 10:09:27AM +0100, david.laight.linux@gmail.com wrote:
> From: David Laight <david.laight.linux@gmail.com>
> 
> Some versions of gcc report that some expressions involving
> __builtin_ffsll(1ull << 63) are not integer constant expressions.
> 
> Rework FIELD_PREP_CONST() to avoid the issue.
> 
> Signed-off-by: David Laight <david.laight.linux@gmail.com>

Thanks, David.

So, the original patch that spotted the problem is:

https://lore.kernel.org/all/20260409-field-prep-fix-v1-1-f0e9ae64f63c@imgtec.com/

Let's keep Matt in the loop, at least.

Please keep the details from the original patch. "Some versions of
some expressions" style is too uncertain.

> ---
> 
> Note that when 'val' is a variable 'val << constant' is likely
> to execute faster than 'val * (1 << constant)'.
> So the normal FIELD_PREP() is best left alone.

Do you have any numbers? I'd prefer to have the codebase consistent
when possible.

Thanks,
Yury
 
>  include/linux/bitfield.h | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h
> index 54aeeef1f0ec..f21765851d5c 100644
> --- a/include/linux/bitfield.h
> +++ b/include/linux/bitfield.h
> @@ -45,6 +45,7 @@
>   */
>  
>  #define __bf_shf(x) (__builtin_ffsll(x) - 1)
> +#define __bf_low_bit(mask) ((mask) & (~(mask) + 1))
>  
>  #define __scalar_type_to_unsigned_cases(type)				\
>  		unsigned type:	(unsigned type)0,			\
> @@ -138,8 +139,6 @@
>  		__FIELD_PREP(_mask, _val, "FIELD_PREP: ");		\
>  	})
>  
> -#define __BF_CHECK_POW2(n)	BUILD_BUG_ON_ZERO(((n) & ((n) - 1)) != 0)
> -
>  /**
>   * FIELD_PREP_CONST() - prepare a constant bitfield element
>   * @_mask: shifted mask defining the field's length and position
> @@ -157,11 +156,11 @@
>  		/* mask must be non-zero */				\
>  		BUILD_BUG_ON_ZERO((_mask) == 0) +			\
>  		/* check if value fits */				\
> -		BUILD_BUG_ON_ZERO(~((_mask) >> __bf_shf(_mask)) & (_val)) + \
> +		BUILD_BUG_ON_ZERO(~((_mask) / __bf_low_bit(_mask)) & (_val)) + \
>  		/* check if mask is contiguous */			\
> -		__BF_CHECK_POW2((_mask) + (1ULL << __bf_shf(_mask))) +	\
> +		BUILD_BUG_ON_ZERO((_mask) & ((_mask) + __bf_low_bit(_mask))) + \
>  		/* and create the value */				\
> -		(((typeof(_mask))(_val) << __bf_shf(_mask)) & (_mask))	\
> +		(((_val) * __bf_low_bit(_mask)) & (_mask))		\
>  	)
>  
>  /**
> -- 
> 2.39.5

Re: [PATCH 1/1] bitfield.h: Ensure FIELD_PREP_CONST() is constant

[David Laight]

On Fri, 10 Apr 2026 12:55:25 -0400
Yury Norov <ynorov@nvidia.com> wrote:

> + Matt Coster
> 
> On Fri, Apr 10, 2026 at 10:09:27AM +0100, david.laight.linux@gmail.com wrote:
> > From: David Laight <david.laight.linux@gmail.com>
> > 
> > Some versions of gcc report that some expressions involving
> > __builtin_ffsll(1ull << 63) are not integer constant expressions.
> > 
> > Rework FIELD_PREP_CONST() to avoid the issue.
> > 
> > Signed-off-by: David Laight <david.laight.linux@gmail.com>  
> 
> Thanks, David.
> 
> So, the original patch that spotted the problem is:
> 
> https://lore.kernel.org/all/20260409-field-prep-fix-v1-1-f0e9ae64f63c@imgtec.com/

That isn't the patch that spotted the problem.
That is a suggested fix.
I've not seen the code (or patch) that actually exhibited the issue.

I think someone changed (drivers/gpu/drm/imagination/pvr_device.h):
#define PVR_PACKED_BVNC(b, v, n, c) \
	((((u64)(b) & GENMASK_ULL(15, 0)) << 48) | \
	 (((u64)(v) & GENMASK_ULL(15, 0)) << 32) | \
	 (((u64)(n) & GENMASK_ULL(15, 0)) << 16) | \
	 (((u64)(c) & GENMASK_ULL(15, 0)) <<  0))
to use FIELD_PREP_CONST().
The top line might be FIELD_PREP_CONST(GENMASK_ULL(63, 48), b))
which has the MSB set on the call to __builtin_ffsll() which
seems to be necessary (but not sufficient) to fail.

The expansion of that is used (pvr_device.c) in:
static enum pvr_gpu_support_level
pvr_gpu_support_level(const struct pvr_gpu_id *gpu_id)
{
	switch (pvr_gpu_id_to_packed_bvnc(gpu_id)) {
	case PVR_PACKED_BVNC(33, 15, 11, 3):
	case PVR_PACKED_BVNC(36, 53, 104, 796):
		return PVR_GPU_SUPPORTED;

	case PVR_PACKED_BVNC(36, 52, 104, 182):
		return PVR_GPU_EXPERIMENTAL;

	default:
		return PVR_GPU_UNKNOWN;
	}
}

> 
> Let's keep Matt in the loop, at least.

I thought I'd typed his address in - somewhere it got lost.

> Please keep the details from the original patch. "Some versions of
> some expressions" style is too uncertain.

But that is pretty much what it is.
I could give a failing test case, but it all reminds me of a bug in ash where:
	x=$(command args)
could lose the last byte of output.
Fiddling with the shell script would fix it (never mind changing the
compiler options for the shell itself).
(Basically is was scanning the on-stack temporary buffer when removing
trailing '\n' and could access buffer[-1], if that happened to be 0xa
then the length would be decremented. Only likely to happen on BE.)

In this case minor tweaks to the expression change the behaviour.
This one fails:
	int t : (__builtin_ffsll((1ull << 63)) < 3u) ? 1 : 2;
change the 3u to 3 or the 63 to 62 and it is ok.
Until someone supporting gcc actually identifies the underlying bug
there really isn't much to say.

> 
> > ---
> > 
> > Note that when 'val' is a variable 'val << constant' is likely
> > to execute faster than 'val * (1 << constant)'.
> > So the normal FIELD_PREP() is best left alone.  
> 
> Do you have any numbers? I'd prefer to have the codebase consistent
> when possible.

I think the multiply instruction will have a higher latency than the shift.
So you are talking about a very small number of clocks if the expression
is in the critical register dependency path.
However FIELD_GET() would need to use a divide - and that would be a lot
worse.

Having written that, ISTR that 'mask' is required to be a constant.
So the compiler may use a shift anyway - if the divide is unsigned.
But for non-constant mask you definitely want a 'shift right'.

While you might think that it only makes sense to use unsigned values,
I've found one piece of code (IIRC in the x86 fault handler) that
passes a signed value to FIELD_GET() and needs the result sign extended.
So, unless that is changed, FIELD_GET() must use an explicit right shift.
(Of course, right shift of negative values is probably UB...)

	David

> 
> Thanks,
> Yury
>  
> >  include/linux/bitfield.h | 9 ++++-----
> >  1 file changed, 4 insertions(+), 5 deletions(-)
> > 
> > diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h
> > index 54aeeef1f0ec..f21765851d5c 100644
> > --- a/include/linux/bitfield.h
> > +++ b/include/linux/bitfield.h
> > @@ -45,6 +45,7 @@
> >   */
> >  
> >  #define __bf_shf(x) (__builtin_ffsll(x) - 1)
> > +#define __bf_low_bit(mask) ((mask) & (~(mask) + 1))
> >  
> >  #define __scalar_type_to_unsigned_cases(type)				\
> >  		unsigned type:	(unsigned type)0,			\
> > @@ -138,8 +139,6 @@
> >  		__FIELD_PREP(_mask, _val, "FIELD_PREP: ");		\
> >  	})
> >  
> > -#define __BF_CHECK_POW2(n)	BUILD_BUG_ON_ZERO(((n) & ((n) - 1)) != 0)
> > -
> >  /**
> >   * FIELD_PREP_CONST() - prepare a constant bitfield element
> >   * @_mask: shifted mask defining the field's length and position
> > @@ -157,11 +156,11 @@
> >  		/* mask must be non-zero */				\
> >  		BUILD_BUG_ON_ZERO((_mask) == 0) +			\
> >  		/* check if value fits */				\
> > -		BUILD_BUG_ON_ZERO(~((_mask) >> __bf_shf(_mask)) & (_val)) + \
> > +		BUILD_BUG_ON_ZERO(~((_mask) / __bf_low_bit(_mask)) & (_val)) + \
> >  		/* check if mask is contiguous */			\
> > -		__BF_CHECK_POW2((_mask) + (1ULL << __bf_shf(_mask))) +	\
> > +		BUILD_BUG_ON_ZERO((_mask) & ((_mask) + __bf_low_bit(_mask))) + \
> >  		/* and create the value */				\
> > -		(((typeof(_mask))(_val) << __bf_shf(_mask)) & (_mask))	\
> > +		(((_val) * __bf_low_bit(_mask)) & (_mask))		\
> >  	)
> >  
> >  /**
> > -- 
> > 2.39.5  

Re: [PATCH 1/1] bitfield.h: Ensure FIELD_PREP_CONST() is constant

[Yury Norov]

On Fri, Apr 10, 2026 at 07:45:38PM +0100, David Laight wrote:
> On Fri, 10 Apr 2026 12:55:25 -0400
> Yury Norov <ynorov@nvidia.com> wrote:
 
 ...

> > > Note that when 'val' is a variable 'val << constant' is likely
> > > to execute faster than 'val * (1 << constant)'.
> > > So the normal FIELD_PREP() is best left alone.  
> > 
> > Do you have any numbers? I'd prefer to have the codebase consistent
> > when possible.
> 
> I think the multiply instruction will have a higher latency than the shift.
> So you are talking about a very small number of clocks if the expression
> is in the critical register dependency path.
> However FIELD_GET() would need to use a divide - and that would be a lot
> worse.
>
> Having written that, ISTR that 'mask' is required to be a constant.
> So the compiler may use a shift anyway - if the divide is unsigned.
> But for non-constant mask you definitely want a 'shift right'.

Non-constant masks are handled with __field_get(), which doesn't use
__bf_shf().

> While you might think that it only makes sense to use unsigned values,
> I've found one piece of code (IIRC in the x86 fault handler) that
> passes a signed value to FIELD_GET() and needs the result sign extended.
> So, unless that is changed, FIELD_GET() must use an explicit right shift.
> (Of course, right shift of negative values is probably UB...)

FIELD_GET() is quite fine with the change:

 #define __FIELD_GET(mask, reg, pfx)                                    \
         ({                                                              \
                 __BF_FIELD_CHECK_MASK(mask, 0U, pfx);                   \
 -               (typeof(mask))(((reg) & (mask)) >> __bf_shf(mask));     \
 +               (typeof(mask))(((reg) & (mask)) / __bf_low_bit(mask));  \
         })
 
void my_test(void)
{
        f3 0f 1e fa             endbr64
        48 83 ec 08             sub    $0x8,%rsp
        volatile int i = -1;

        pr_err("%lx\n", FIELD_GET(GENMASK(10,5), i));
        48 c7 c7 13 e3 51 82    mov    $0xffffffff8251e313,%rdi
        volatile int i = -1;
        c7 44 24 04 ff ff ff    movl   $0xffffffff,0x4(%rsp)
        ff 
        pr_err("%lx\n", FIELD_GET(GENMASK(10,5), i));
        8b 74 24 04             mov    0x4(%rsp),%esi

}
        48 83 c4 08             add    $0x8,%rsp
        pr_err("%lx\n", FIELD_GET(GENMASK(10,5), i));
        81 e6 e0 07 00 00       and    $0x7e0,%esi
        48 c1 ee 05             shr    $0x5,%rsi
        e9 32 aa b9 ff          jmp    <_printk>

Thanks,
Yury

Re: [PATCH 1/1] bitfield.h: Ensure FIELD_PREP_CONST() is constant

[David Laight]

On Sat, 11 Apr 2026 00:24:28 -0400
Yury Norov <ynorov@nvidia.com> wrote:

> On Fri, Apr 10, 2026 at 07:45:38PM +0100, David Laight wrote:
> > On Fri, 10 Apr 2026 12:55:25 -0400
> > Yury Norov <ynorov@nvidia.com> wrote:  
>  
>  ...
> 
> > > > Note that when 'val' is a variable 'val << constant' is likely
> > > > to execute faster than 'val * (1 << constant)'.
> > > > So the normal FIELD_PREP() is best left alone.    
> > > 
> > > Do you have any numbers? I'd prefer to have the codebase consistent
> > > when possible.  
> > 
> > I think the multiply instruction will have a higher latency than the shift.
> > So you are talking about a very small number of clocks if the expression
> > is in the critical register dependency path.
> > However FIELD_GET() would need to use a divide - and that would be a lot
> > worse.
> >
> > Having written that, ISTR that 'mask' is required to be a constant.
> > So the compiler may use a shift anyway - if the divide is unsigned.
> > But for non-constant mask you definitely want a 'shift right'.  
> 
> Non-constant masks are handled with __field_get(), which doesn't use
> __bf_shf().
> 
> > While you might think that it only makes sense to use unsigned values,
> > I've found one piece of code (IIRC in the x86 fault handler) that
> > passes a signed value to FIELD_GET() and needs the result sign extended.
> > So, unless that is changed, FIELD_GET() must use an explicit right shift.
> > (Of course, right shift of negative values is probably UB...)  
> 
> FIELD_GET() is quite fine with the change:
> 
>  #define __FIELD_GET(mask, reg, pfx)                                    \
>          ({                                                              \
>                  __BF_FIELD_CHECK_MASK(mask, 0U, pfx);                   \
>  -               (typeof(mask))(((reg) & (mask)) >> __bf_shf(mask));     \
>  +               (typeof(mask))(((reg) & (mask)) / __bf_low_bit(mask));  \
>          })
>  
> void my_test(void)
> {
>         f3 0f 1e fa             endbr64
>         48 83 ec 08             sub    $0x8,%rsp
>         volatile int i = -1;
> 
>         pr_err("%lx\n", FIELD_GET(GENMASK(10,5), i));
>         48 c7 c7 13 e3 51 82    mov    $0xffffffff8251e313,%rdi
>         volatile int i = -1;
>         c7 44 24 04 ff ff ff    movl   $0xffffffff,0x4(%rsp)
>         ff 
>         pr_err("%lx\n", FIELD_GET(GENMASK(10,5), i));
>         8b 74 24 04             mov    0x4(%rsp),%esi
> 
> }
>         48 83 c4 08             add    $0x8,%rsp
>         pr_err("%lx\n", FIELD_GET(GENMASK(10,5), i));
>         81 e6 e0 07 00 00       and    $0x7e0,%esi
>         48 c1 ee 05             shr    $0x5,%rsi
>         e9 32 aa b9 ff          jmp    <_printk>

There is a subtle difference between (https://www.godbolt.org/z/KM7MesPWM):

int a(int x)
{
    return x >> __bf_shf(0xf0u);
}

int b(int x)
{
    return x / __bf_low_bit(0xf0);
}

int c(int x)
{
    return x / __bf_low_bit(0xf0u);
}

a:
        movl    %edi, %eax
        sarl    $4, %eax
        ret
b:
        testl   %edi, %edi
        leal    15(%rdi), %eax
        cmovns  %edi, %eax
        sarl    $4, %eax
        ret
c:
        movl    %edi, %eax
        shrl    $4, %eax
        ret

A while ago I did a compile-test for negative values and found one
place that requires the sign-replicating right shift.

So you'd need that check and to fixup the caller.

	David

> 
> Thanks,
> Yury