Re: [RFC][PATCH 07/22] x86,extable: Extend extable functionality

From: Sean Christopherson <seanjc@google.com>
To: Peter Zijlstra <peterz@infradead.org>
Cc: x86@kernel.org, linux-kernel@vger.kernel.org,
	jpoimboe@redhat.com, mark.rutland@arm.com, dvyukov@google.com,
	pbonzini@redhat.com, mbenes@suse.cz
Subject: Re: [RFC][PATCH 07/22] x86,extable: Extend extable functionality
Date: Fri, 5 Nov 2021 19:47:22 +0000	[thread overview]
Message-ID: <YYWKSmHkgdMA2euh@google.com> (raw)
In-Reply-To: <20211105193229.GP174703@worktop.programming.kicks-ass.net>

On Fri, Nov 05, 2021, Peter Zijlstra wrote:
> On Fri, Nov 05, 2021 at 07:17:20PM +0000, Sean Christopherson wrote:
> > On Fri, Nov 05, 2021, Peter Zijlstra wrote:
> > > On Fri, Nov 05, 2021 at 05:32:14PM +0000, Sean Christopherson wrote:
> > > 
> > > > > +#define EX_IMM_MASK	0xFFFF0000
> > > 
> > > > > +	imm  = FIELD_GET(EX_IMM_MASK,  e->type);
> > > > 
> > > > FIELD_GET casts the result based on the type of the mask, but doesn't explicitly
> > > > sign extend the masked field, i.e. there's no intermediate cast to tell the compiler
> > > > that the imm is a 16-bit value that should be sign extended.
> > > > 
> > > > Modifying FIELD_GET to sign extended is probably a bad idea as I'm guessing the
> > > > vast, vast majority of use cases don't want that behavior.  I'm not sure how that
> > > > would even work with masks that are e.g. 5 bits or so.
> > > 
> > > So the way I was reading it was that typeof(_mask) is 'int', e->type is
> > > also 'int', we mask out the top bits, and since it's all 'int' we do an
> > > arith shift right (ie. preserves sign).
> > > 
> > > Where did that reading go wrong?
> > 
> > Hmm, C99 standard says that right shift with a negative value is implementation
> > specific:
> 
> C99 is sodding daft wrt signed values. That's why we force -fwrapv and
> say signed is 2s complement and expect sanity.

FWIW, -fwrapv was supplanted by -fno-strict-overflow back in 2009 by commit
a137802ee839 ("Don't use '-fwrapv' compiler option: it's buggy in gcc-4.1.x").
But I don't think that matters because AFAICT both apply only to "addition,
subtraction and multiplication".

> > gcc-10 generates a bare "shr", i.e. doesn't special case negative values, and "shr"
> > is explicitly defined as an unsigned divide.
> 
> We hard rely on signed shift right to preserve sign all over the place,
> how come it goes sideways here? Lemme go stare at asm...

Huh.  TIL there's actually a use case for this.

Anyways, I think the issue is that EX_IMM_MASK is being interpreted as an unsigned
value by the compiler.  Which I think is legal for a 64-bit kernel?  Because bit 63
is the MSB, not bit 31.  E.g. this

	FIELD_GET((int)EX_IMM_MASK, e->type)

generates SAR instead of SHR.