Re: [PATCH 2/9] x86/extable: switch to using FIELD_GET_SIGNED()

[David Laight]

On Mon, 20 Apr 2026 13:18:47 -0400
Yury Norov <ynorov@xxxxxxxxxx> wrote:

> On Mon, Apr 20, 2026 at 01:24:28PM +0200, Peter Zijlstra wrote:
> > On Fri, Apr 17, 2026 at 01:36:13PM -0400, Yury Norov wrote:  
> > > The EX_DATA register is laid out such that EX_DATA_IMM occupied MSB.
> > > It's done to make sure that FIELD_GET() will sign-extend the IMM
> > > field during extraction.
> > > 
> > > To enforce that, all EX_DATA masks are made signed integers. This
> > > works, but relies on the particular implementation of FIELD_GET(),
> > > i.e. masking then shifting, not vice versa; and the particular
> > > placement of the fields in the register.  
> > 
> > I don't think the order of the mask and shift matters in this case. If
> > we were to first shift down and then mask, it would still work (after
> > all, the mask would also need to be shifted and would also get sign
> > extended, effectively ending up as -1).  
>  
> FIELD_GET() doesn't require mask to be signed when a reg is signed, so
> shifting mask may become zero-extended in an alternative implementation:
> 
>         (reg >> __bf_shf(mask)) & (mask >> __bf_shf(mask)
> 
> This all is hypothetical, anyways.
> 
> > But yes, this very much depends on the signed field being the topmost
> > field and including the MSB.  
> 
> This is the part I dislike mostly. This would look just like undefined
> behavior for the API user: depending on fields placement or type of the
> inputs, sometimes FIELD_GET() sign-extendeds the field, and sometimes
> not. 
> 
> We could likely force FIELD_GET() to treat both reg and mask as unsigned
> types, and state that explicitly in the documentation.
> 

There is already a BUILD_BUG_ON((_mask) == 0), changing it to >= 0
will detect negative masks.
I think the only one is the x86 exception table.
FIELD_GET() casts the result to typeof(_mask) so the sign of 'reg'
shouldn't matter.
I just tried building with a compile-time check for reg being negative.
But there are too many false positives from FIELD_GET(mask, readl(addr))
and FIELD_GET(mask, READ_ONCE(var)).
The pre-processor expansions of those don't bear thinking about.

It's late now, but I will check how __unsigned_scalar_typeof() handles
variables with const or volatile qualifiers.
I think they do though the 'default' the same at bitfields.

	David