[RFC][PATCH 00/22] x86: Remove anonymous out-of-line fixups

[RFC][PATCH 00/22] x86: Remove anonymous out-of-line fixups

[Peter Zijlstra]

Hi,

Direct counterpart to the arm64 series from Mark:

  https://lkml.kernel.org/r/20211019160219.5202-1-mark.rutland@arm.com

Since he already put it rather well:

"We recently realised that out-of-line extable fixups cause a number of problems
for backtracing (mattering both for developers and for RELIABLE_STACKTRACE and
LIVEPATCH). Dmitry spotted a confusing backtrace, which we identified was due
to problems with unwinding fixups, as summarized in:

  https://lore.kernel.org/linux-arm-kernel/20210927171812.GB9201@C02TD0UTHF1T.local/

The gist is that while backtracing through a fixup, the fixup gets symbolized
as an offset from the nearest prior symbol (which happens to be
`__entry_tramp_text_end`), and we the backtrace misses the function that was
being fixed up (because the fixup handling adjusts the PC, then the fixup does
a direct branch back to the original function). We can't reliably map from an
arbitrary PC in the fixup text back to the original function.

The way we create fixups is a bit unfortunate: most fixups are generated from
common templates, and only differ in register to be poked and the address to
branch back to, leading to redundant copies of the same logic that must pollute
Since the fixups are all written in assembly, and duplicated for each fixup
site, we can only perform very simple fixups, and can't handle any complex
triage that we might need for some exceptions (e.g. MTE faults)."



So far these patches have only been compile tested on x86_64
(defconfig,allyesconfig) and boot tested in kvm (defconfig) -- realy early
days.

Enjoy..

---
 arch/x86/entry/entry_32.S                  |  28 ++---
 arch/x86/entry/entry_64.S                  |  13 ++-
 arch/x86/entry/vdso/vdso-layout.lds.S      |   1 -
 arch/x86/include/asm/asm.h                 |  27 +++++
 arch/x86/include/asm/extable_fixup_types.h |  44 ++++++--
 arch/x86/include/asm/futex.h               |  28 ++---
 arch/x86/include/asm/msr.h                 |  26 ++---
 arch/x86/include/asm/segment.h             |   9 +-
 arch/x86/include/asm/sgx.h                 |  18 ++++
 arch/x86/include/asm/uaccess.h             |  35 +++---
 arch/x86/include/asm/word-at-a-time.h      |  29 ++---
 arch/x86/include/asm/xen/page.h            |  12 +--
 arch/x86/kernel/cpu/sgx/encls.h            |  36 +------
 arch/x86/kernel/fpu/legacy.h               |   6 +-
 arch/x86/kernel/fpu/xstate.h               |   6 +-
 arch/x86/kernel/ftrace.c                   |   9 +-
 arch/x86/kernel/vmlinux.lds.S              |   1 -
 arch/x86/kvm/emulate.c                     |  14 +--
 arch/x86/kvm/vmx/vmx_ops.h                 |  14 ++-
 arch/x86/lib/checksum_32.S                 |  19 +---
 arch/x86/lib/copy_mc_64.S                  |  12 +--
 arch/x86/lib/copy_user_64.S                |  32 ++----
 arch/x86/lib/mmx_32.c                      |  83 +++++----------
 arch/x86/lib/usercopy_32.c                 |  66 +++++-------
 arch/x86/lib/usercopy_64.c                 |   8 +-
 arch/x86/mm/extable.c                      | 166 ++++++++++++++++++++++++-----
 include/linux/bitfield.h                   |  19 +++-
 27 files changed, 385 insertions(+), 376 deletions(-)

[RFC][PATCH 01/22] bitfield.h: Fix "type of reg too small for mask" test

[Peter Zijlstra]

The test: 'mask > (typeof(_reg))~0ull' only works correctly when both
sides are unsigned, consider:

 - 0xff000000 vs (int)~0ull
 - 0x000000ff vs (int)~0ull

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 include/linux/bitfield.h |   19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

--- a/include/linux/bitfield.h
+++ b/include/linux/bitfield.h
@@ -41,6 +41,22 @@
 
 #define __bf_shf(x) (__builtin_ffsll(x) - 1)
 
+#define __scalar_type_to_unsigned_cases(type)				\
+		unsigned type:	(unsigned type)0,			\
+		signed type:	(unsigned type)0
+
+#define __unsigned_scalar_typeof(x) typeof(				\
+		_Generic((x),						\
+			char:	(unsigned char)0,			\
+			__scalar_type_to_unsigned_cases(char),		\
+			__scalar_type_to_unsigned_cases(short),		\
+			__scalar_type_to_unsigned_cases(int),		\
+			__scalar_type_to_unsigned_cases(long),		\
+			__scalar_type_to_unsigned_cases(long long),	\
+			default: (x)))
+
+#define __bf_cast_unsigned(type, x)	((__unsigned_scalar_typeof(type))(x))
+
 #define __BF_FIELD_CHECK(_mask, _reg, _val, _pfx)			\
 	({								\
 		BUILD_BUG_ON_MSG(!__builtin_constant_p(_mask),		\
@@ -49,7 +65,8 @@
 		BUILD_BUG_ON_MSG(__builtin_constant_p(_val) ?		\
 				 ~((_mask) >> __bf_shf(_mask)) & (_val) : 0, \
 				 _pfx "value too large for the field"); \
-		BUILD_BUG_ON_MSG((_mask) > (typeof(_reg))~0ull,		\
+		BUILD_BUG_ON_MSG(__bf_cast_unsigned(_mask, _mask) >	\
+				 __bf_cast_unsigned(_reg, ~0ull),	\
 				 _pfx "type of reg too small for mask"); \
 		__BUILD_BUG_ON_NOT_POWER_OF_2((_mask) +			\
 					      (1ULL << __bf_shf(_mask))); \

[RFC][PATCH 02/22] x86,mmx_32: Remove .fixup usage

[Peter Zijlstra]

This code puts an exception table entry on the "PREFIX" instruction to
overwrite it with a jmp.d8 when it triggers an exception. Except of
course, our code is no longer writable, also SMP.

Replace it with ALTERNATIVE, the novel

XXX: arguably we should just delete this code

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/x86/lib/mmx_32.c |   83 ++++++++++++++++----------------------------------
 1 file changed, 27 insertions(+), 56 deletions(-)

--- a/arch/x86/lib/mmx_32.c
+++ b/arch/x86/lib/mmx_32.c
@@ -50,23 +50,17 @@ void *_mmx_memcpy(void *to, const void *
 	kernel_fpu_begin_mask(KFPU_387);
 
 	__asm__ __volatile__ (
-		"1: prefetch (%0)\n"		/* This set is 28 bytes */
-		"   prefetch 64(%0)\n"
-		"   prefetch 128(%0)\n"
-		"   prefetch 192(%0)\n"
-		"   prefetch 256(%0)\n"
-		"2:  \n"
-		".section .fixup, \"ax\"\n"
-		"3: movw $0x1AEB, 1b\n"	/* jmp on 26 bytes */
-		"   jmp 2b\n"
-		".previous\n"
-			_ASM_EXTABLE(1b, 3b)
-			: : "r" (from));
+		ALTERNATIVE "",
+			    "prefetch (%0)\n"
+			    "prefetch 64(%0)\n"
+			    "prefetch 128(%0)\n"
+			    "prefetch 192(%0)\n"
+			    "prefetch 256(%0)\n", X86_FEATURE_3DNOW
+		: : "r" (from));
 
 	for ( ; i > 5; i--) {
 		__asm__ __volatile__ (
-		"1:  prefetch 320(%0)\n"
-		"2:  movq (%0), %%mm0\n"
+		"  movq (%0), %%mm0\n"
 		"  movq 8(%0), %%mm1\n"
 		"  movq 16(%0), %%mm2\n"
 		"  movq 24(%0), %%mm3\n"
@@ -82,11 +76,6 @@ void *_mmx_memcpy(void *to, const void *
 		"  movq %%mm1, 40(%1)\n"
 		"  movq %%mm2, 48(%1)\n"
 		"  movq %%mm3, 56(%1)\n"
-		".section .fixup, \"ax\"\n"
-		"3: movw $0x05EB, 1b\n"	/* jmp on 5 bytes */
-		"   jmp 2b\n"
-		".previous\n"
-			_ASM_EXTABLE(1b, 3b)
 			: : "r" (from), "r" (to) : "memory");
 
 		from += 64;
@@ -177,22 +166,17 @@ static void fast_copy_page(void *to, voi
 	 * but that is for later. -AV
 	 */
 	__asm__ __volatile__(
-		"1: prefetch (%0)\n"
-		"   prefetch 64(%0)\n"
-		"   prefetch 128(%0)\n"
-		"   prefetch 192(%0)\n"
-		"   prefetch 256(%0)\n"
-		"2:  \n"
-		".section .fixup, \"ax\"\n"
-		"3: movw $0x1AEB, 1b\n"	/* jmp on 26 bytes */
-		"   jmp 2b\n"
-		".previous\n"
-			_ASM_EXTABLE(1b, 3b) : : "r" (from));
+		ALTERNATIVE "",
+			    "prefetch (%0)\n"
+			    "prefetch 64(%0)\n"
+			    "prefetch 128(%0)\n"
+			    "prefetch 192(%0)\n"
+			    "prefetch 256(%0)\n", X86_FEATURE_3DNOW
+		: : "r" (from));
 
 	for (i = 0; i < (4096-320)/64; i++) {
 		__asm__ __volatile__ (
-		"1: prefetch 320(%0)\n"
-		"2: movq (%0), %%mm0\n"
+		"   movq (%0), %%mm0\n"
 		"   movntq %%mm0, (%1)\n"
 		"   movq 8(%0), %%mm1\n"
 		"   movntq %%mm1, 8(%1)\n"
@@ -208,11 +192,7 @@ static void fast_copy_page(void *to, voi
 		"   movntq %%mm6, 48(%1)\n"
 		"   movq 56(%0), %%mm7\n"
 		"   movntq %%mm7, 56(%1)\n"
-		".section .fixup, \"ax\"\n"
-		"3: movw $0x05EB, 1b\n"	/* jmp on 5 bytes */
-		"   jmp 2b\n"
-		".previous\n"
-		_ASM_EXTABLE(1b, 3b) : : "r" (from), "r" (to) : "memory");
+			: : "r" (from), "r" (to) : "memory");
 
 		from += 64;
 		to += 64;
@@ -220,7 +200,7 @@ static void fast_copy_page(void *to, voi
 
 	for (i = (4096-320)/64; i < 4096/64; i++) {
 		__asm__ __volatile__ (
-		"2: movq (%0), %%mm0\n"
+		"   movq (%0), %%mm0\n"
 		"   movntq %%mm0, (%1)\n"
 		"   movq 8(%0), %%mm1\n"
 		"   movntq %%mm1, 8(%1)\n"
@@ -237,6 +217,7 @@ static void fast_copy_page(void *to, voi
 		"   movq 56(%0), %%mm7\n"
 		"   movntq %%mm7, 56(%1)\n"
 			: : "r" (from), "r" (to) : "memory");
+
 		from += 64;
 		to += 64;
 	}
@@ -295,22 +276,17 @@ static void fast_copy_page(void *to, voi
 	kernel_fpu_begin_mask(KFPU_387);
 
 	__asm__ __volatile__ (
-		"1: prefetch (%0)\n"
-		"   prefetch 64(%0)\n"
-		"   prefetch 128(%0)\n"
-		"   prefetch 192(%0)\n"
-		"   prefetch 256(%0)\n"
-		"2:  \n"
-		".section .fixup, \"ax\"\n"
-		"3: movw $0x1AEB, 1b\n"	/* jmp on 26 bytes */
-		"   jmp 2b\n"
-		".previous\n"
-			_ASM_EXTABLE(1b, 3b) : : "r" (from));
+		ALTERNATIVE "",
+			    "prefetch (%0)\n"
+			    "prefetch 64(%0)\n"
+			    "prefetch 128(%0)\n"
+			    "prefetch 192(%0)\n"
+			    "prefetch 256(%0)\n", X86_FEATURE_3DNOW
+		: : "r" (from));
 
 	for (i = 0; i < 4096/64; i++) {
 		__asm__ __volatile__ (
-		"1: prefetch 320(%0)\n"
-		"2: movq (%0), %%mm0\n"
+		"   movq (%0), %%mm0\n"
 		"   movq 8(%0), %%mm1\n"
 		"   movq 16(%0), %%mm2\n"
 		"   movq 24(%0), %%mm3\n"
@@ -326,11 +302,6 @@ static void fast_copy_page(void *to, voi
 		"   movq %%mm1, 40(%1)\n"
 		"   movq %%mm2, 48(%1)\n"
 		"   movq %%mm3, 56(%1)\n"
-		".section .fixup, \"ax\"\n"
-		"3: movw $0x05EB, 1b\n"	/* jmp on 5 bytes */
-		"   jmp 2b\n"
-		".previous\n"
-			_ASM_EXTABLE(1b, 3b)
 			: : "r" (from), "r" (to) : "memory");
 
 		from += 64;

Re: [RFC][PATCH 02/22] x86,mmx_32: Remove .fixup usage

[Borislav Petkov]

On Thu, Nov 04, 2021 at 05:47:31PM +0100, Peter Zijlstra wrote:
> This code puts an exception table entry on the "PREFIX" instruction to
> overwrite it with a jmp.d8 when it triggers an exception. Except of
> course, our code is no longer writable, also SMP.
> 
> Replace it with ALTERNATIVE, the novel
> 
> XXX: arguably we should just delete this code

Yah, might as well.

Wikipedia says the last AMD CPU which supports 3DNow is A8-3870K which
is family 0x11, i.e.,

1. a real rarity
2. it is pretty much obsolete
3. even if not, it can do AMD64
4. and even if people who have it, wanna run 32-bit, they can use the
normal memcpy, i.e., CONFIG_X86_USE_3DNOW=n should work there

In our case, it is a bit different, though:

config X86_USE_3DNOW
        def_bool y
        depends on (MCYRIXIII || MK7 || MGEODE_LX) && !UML

MK7 is K7 - that is practically dead.

The only thing I have no clue about are those cyrix and geode things and
whether they're still actively used in some embedded gunk.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

RE: [RFC][PATCH 02/22] x86,mmx_32: Remove .fixup usage

[David Laight]

From: Borislav Petkov
> Sent: 04 November 2021 18:01
> 
> On Thu, Nov 04, 2021 at 05:47:31PM +0100, Peter Zijlstra wrote:
> > This code puts an exception table entry on the "PREFIX" instruction to
> > overwrite it with a jmp.d8 when it triggers an exception. Except of
> > course, our code is no longer writable, also SMP.
> >
> > Replace it with ALTERNATIVE, the novel
> >
> > XXX: arguably we should just delete this code
> 
> Yah, might as well.
> 
> Wikipedia says the last AMD CPU which supports 3DNow is A8-3870K which
> is family 0x11, i.e.,
> 
> 1. a real rarity
> 2. it is pretty much obsolete
> 3. even if not, it can do AMD64
> 4. and even if people who have it, wanna run 32-bit, they can use the
> normal memcpy, i.e., CONFIG_X86_USE_3DNOW=n should work there
> 
> In our case, it is a bit different, though:
> 
> config X86_USE_3DNOW
>         def_bool y
>         depends on (MCYRIXIII || MK7 || MGEODE_LX) && !UML
> 
> MK7 is K7 - that is practically dead.
> 
> The only thing I have no clue about are those cyrix and geode things and
> whether they're still actively used in some embedded gunk.

And whether the prefetch actually helps on those at all.

And even if it does, do enough memcpy() need to prefetch the
data to make it an actual gain - rather than just slowing
down memcpy() where the buffers are cache resident.

Not to mention the cases where prefetching on the source displaces
a target buffer cache line.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [RFC][PATCH 02/22] x86,mmx_32: Remove .fixup usage

[Josh Poimboeuf]

On Thu, Nov 04, 2021 at 05:47:31PM +0100, Peter Zijlstra wrote:
> This code puts an exception table entry on the "PREFIX" instruction to
> overwrite it with a jmp.d8 when it triggers an exception. Except of
> course, our code is no longer writable, also SMP.
> 
> Replace it with ALTERNATIVE, the novel
> 
> XXX: arguably we should just delete this code
> 
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  arch/x86/lib/mmx_32.c |   83 ++++++++++++++++----------------------------------
>  1 file changed, 27 insertions(+), 56 deletions(-)
> 
> --- a/arch/x86/lib/mmx_32.c
> +++ b/arch/x86/lib/mmx_32.c
> @@ -50,23 +50,17 @@ void *_mmx_memcpy(void *to, const void *
>  	kernel_fpu_begin_mask(KFPU_387);
>  
>  	__asm__ __volatile__ (
> -		"1: prefetch (%0)\n"		/* This set is 28 bytes */
> -		"   prefetch 64(%0)\n"
> -		"   prefetch 128(%0)\n"
> -		"   prefetch 192(%0)\n"
> -		"   prefetch 256(%0)\n"
> -		"2:  \n"
> -		".section .fixup, \"ax\"\n"
> -		"3: movw $0x1AEB, 1b\n"	/* jmp on 26 bytes */
> -		"   jmp 2b\n"
> -		".previous\n"
> -			_ASM_EXTABLE(1b, 3b)
> -			: : "r" (from));
> +		ALTERNATIVE "",
> +			    "prefetch (%0)\n"
> +			    "prefetch 64(%0)\n"
> +			    "prefetch 128(%0)\n"
> +			    "prefetch 192(%0)\n"
> +			    "prefetch 256(%0)\n", X86_FEATURE_3DNOW

I think this should instead be X86_FEATURE_3DNOWPREFETCH (which isn't
3DNow-specific and should really just be called X86_FEATURE_PREFETCH
anyway)

> +		: : "r" (from));
>  
>  	for ( ; i > 5; i--) {
>  		__asm__ __volatile__ (
> -		"1:  prefetch 320(%0)\n"
> -		"2:  movq (%0), %%mm0\n"
> +		"  movq (%0), %%mm0\n"

Not sure why this prefetch was removed?  It can also be behind an
alternative just like the first one.

-- 
Josh

Re: [RFC][PATCH 02/22] x86,mmx_32: Remove .fixup usage

[Peter Zijlstra]

On Thu, Nov 04, 2021 at 01:22:20PM -0700, Josh Poimboeuf wrote:
> On Thu, Nov 04, 2021 at 05:47:31PM +0100, Peter Zijlstra wrote:
> > This code puts an exception table entry on the "PREFIX" instruction to
> > overwrite it with a jmp.d8 when it triggers an exception. Except of
> > course, our code is no longer writable, also SMP.
> > 
> > Replace it with ALTERNATIVE, the novel
> > 
> > XXX: arguably we should just delete this code
> > 
> > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> > ---
> >  arch/x86/lib/mmx_32.c |   83 ++++++++++++++++----------------------------------
> >  1 file changed, 27 insertions(+), 56 deletions(-)
> > 
> > --- a/arch/x86/lib/mmx_32.c
> > +++ b/arch/x86/lib/mmx_32.c
> > @@ -50,23 +50,17 @@ void *_mmx_memcpy(void *to, const void *
> >  	kernel_fpu_begin_mask(KFPU_387);
> >  
> >  	__asm__ __volatile__ (
> > -		"1: prefetch (%0)\n"		/* This set is 28 bytes */
> > -		"   prefetch 64(%0)\n"
> > -		"   prefetch 128(%0)\n"
> > -		"   prefetch 192(%0)\n"
> > -		"   prefetch 256(%0)\n"
> > -		"2:  \n"
> > -		".section .fixup, \"ax\"\n"
> > -		"3: movw $0x1AEB, 1b\n"	/* jmp on 26 bytes */
> > -		"   jmp 2b\n"
> > -		".previous\n"
> > -			_ASM_EXTABLE(1b, 3b)
> > -			: : "r" (from));
> > +		ALTERNATIVE "",
> > +			    "prefetch (%0)\n"
> > +			    "prefetch 64(%0)\n"
> > +			    "prefetch 128(%0)\n"
> > +			    "prefetch 192(%0)\n"
> > +			    "prefetch 256(%0)\n", X86_FEATURE_3DNOW
> 
> I think this should instead be X86_FEATURE_3DNOWPREFETCH (which isn't
> 3DNow-specific and should really just be called X86_FEATURE_PREFETCH
> anyway)

Ack.

> > +		: : "r" (from));
> >  
> >  	for ( ; i > 5; i--) {
> >  		__asm__ __volatile__ (
> > -		"1:  prefetch 320(%0)\n"
> > -		"2:  movq (%0), %%mm0\n"
> > +		"  movq (%0), %%mm0\n"
> 
> Not sure why this prefetch was removed?  It can also be behind an
> alternative just like the first one.

Urgh, that was me just plain killing all the prefetch in there and
forgetting to put it back :/ Will fix.

[RFC][PATCH 03/22] x86,copy_user_64: Remove .fixup usage

[Peter Zijlstra]

Place the anonymous .fixup code at the tail of the regular functions.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/x86/lib/copy_user_64.S |   32 +++++++++++---------------------
 1 file changed, 11 insertions(+), 21 deletions(-)

--- a/arch/x86/lib/copy_user_64.S
+++ b/arch/x86/lib/copy_user_64.S
@@ -32,14 +32,10 @@
 	decl %ecx
 	jnz 100b
 102:
-	.section .fixup,"ax"
-103:	addl %ecx,%edx			/* ecx is zerorest also */
-	jmp .Lcopy_user_handle_tail
-	.previous
 
-	_ASM_EXTABLE_CPY(100b, 103b)
-	_ASM_EXTABLE_CPY(101b, 103b)
-	.endm
+	_ASM_EXTABLE_CPY(100b, .Lcopy_user_handle_align)
+	_ASM_EXTABLE_CPY(101b, .Lcopy_user_handle_align)
+.endm
 
 /*
  * copy_user_generic_unrolled - memory copy with exception handling.
@@ -107,7 +103,6 @@ SYM_FUNC_START(copy_user_generic_unrolle
 	ASM_CLAC
 	ret
 
-	.section .fixup,"ax"
 30:	shll $6,%ecx
 	addl %ecx,%edx
 	jmp 60f
@@ -115,7 +110,6 @@ SYM_FUNC_START(copy_user_generic_unrolle
 	jmp 60f
 50:	movl %ecx,%edx
 60:	jmp .Lcopy_user_handle_tail /* ecx is zerorest also */
-	.previous
 
 	_ASM_EXTABLE_CPY(1b, 30b)
 	_ASM_EXTABLE_CPY(2b, 30b)
@@ -166,20 +160,16 @@ SYM_FUNC_START(copy_user_generic_string)
 	movl %edx,%ecx
 	shrl $3,%ecx
 	andl $7,%edx
-1:	rep
-	movsq
+1:	rep movsq
 2:	movl %edx,%ecx
-3:	rep
-	movsb
+3:	rep movsb
 	xorl %eax,%eax
 	ASM_CLAC
 	ret
 
-	.section .fixup,"ax"
 11:	leal (%rdx,%rcx,8),%ecx
 12:	movl %ecx,%edx		/* ecx is zerorest also */
 	jmp .Lcopy_user_handle_tail
-	.previous
 
 	_ASM_EXTABLE_CPY(1b, 11b)
 	_ASM_EXTABLE_CPY(3b, 12b)
@@ -203,16 +193,13 @@ SYM_FUNC_START(copy_user_enhanced_fast_s
 	cmpl $64,%edx
 	jb .L_copy_short_string	/* less then 64 bytes, avoid the costly 'rep' */
 	movl %edx,%ecx
-1:	rep
-	movsb
+1:	rep movsb
 	xorl %eax,%eax
 	ASM_CLAC
 	ret
 
-	.section .fixup,"ax"
 12:	movl %ecx,%edx		/* ecx is zerorest also */
 	jmp .Lcopy_user_handle_tail
-	.previous
 
 	_ASM_EXTABLE_CPY(1b, 12b)
 SYM_FUNC_END(copy_user_enhanced_fast_string)
@@ -240,6 +227,11 @@ SYM_CODE_START_LOCAL(.Lcopy_user_handle_
 	ret
 
 	_ASM_EXTABLE_CPY(1b, 2b)
+
+.Lcopy_user_handle_align:
+	addl %ecx,%edx			/* ecx is zerorest also */
+	jmp .Lcopy_user_handle_tail
+
 SYM_CODE_END(.Lcopy_user_handle_tail)
 
 /*
@@ -350,7 +342,6 @@ SYM_FUNC_START(__copy_user_nocache)
 	sfence
 	ret
 
-	.section .fixup,"ax"
 .L_fixup_4x8b_copy:
 	shll $6,%ecx
 	addl %ecx,%edx
@@ -366,7 +357,6 @@ SYM_FUNC_START(__copy_user_nocache)
 .L_fixup_handle_tail:
 	sfence
 	jmp .Lcopy_user_handle_tail
-	.previous
 
 	_ASM_EXTABLE_CPY(1b, .L_fixup_4x8b_copy)
 	_ASM_EXTABLE_CPY(2b, .L_fixup_4x8b_copy)

[RFC][PATCH 04/22] x86,copy_mc_64: Remove .fixup usage

[Peter Zijlstra]

Place the anonymous .fixup code at the tail of the regular functions.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/x86/lib/copy_mc_64.S |   12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

--- a/arch/x86/lib/copy_mc_64.S
+++ b/arch/x86/lib/copy_mc_64.S
@@ -78,9 +78,7 @@ SYM_FUNC_START(copy_mc_fragile)
 	xorl %eax, %eax
 .L_done:
 	ret
-SYM_FUNC_END(copy_mc_fragile)
 
-	.section .fixup, "ax"
 	/*
 	 * Return number of bytes not copied for any failure. Note that
 	 * there is no "tail" handling since the source buffer is 8-byte
@@ -105,14 +103,14 @@ SYM_FUNC_END(copy_mc_fragile)
 	movl	%ecx, %edx
 	jmp copy_mc_fragile_handle_tail
 
-	.previous
-
 	_ASM_EXTABLE_TYPE(.L_read_leading_bytes, .E_leading_bytes, EX_TYPE_DEFAULT_MCE_SAFE)
 	_ASM_EXTABLE_TYPE(.L_read_words, .E_read_words, EX_TYPE_DEFAULT_MCE_SAFE)
 	_ASM_EXTABLE_TYPE(.L_read_trailing_bytes, .E_trailing_bytes, EX_TYPE_DEFAULT_MCE_SAFE)
 	_ASM_EXTABLE(.L_write_leading_bytes, .E_leading_bytes)
 	_ASM_EXTABLE(.L_write_words, .E_write_words)
 	_ASM_EXTABLE(.L_write_trailing_bytes, .E_trailing_bytes)
+
+SYM_FUNC_END(copy_mc_fragile)
 #endif /* CONFIG_X86_MCE */
 
 /*
@@ -133,9 +131,7 @@ SYM_FUNC_START(copy_mc_enhanced_fast_str
 	/* Copy successful. Return zero */
 	xorl %eax, %eax
 	ret
-SYM_FUNC_END(copy_mc_enhanced_fast_string)
 
-	.section .fixup, "ax"
 .E_copy:
 	/*
 	 * On fault %rcx is updated such that the copy instruction could
@@ -147,7 +143,7 @@ SYM_FUNC_END(copy_mc_enhanced_fast_strin
 	movq %rcx, %rax
 	ret
 
-	.previous
-
 	_ASM_EXTABLE_TYPE(.L_copy, .E_copy, EX_TYPE_DEFAULT_MCE_SAFE)
+
+SYM_FUNC_END(copy_mc_enhanced_fast_string)
 #endif /* !CONFIG_UML */

[RFC][PATCH 05/22] x86,entry_64: Remove .fixup usage

[Peter Zijlstra]

Place the anonymous .fixup code at the tail of the regular functions.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/x86/entry/entry_64.S |   13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

--- a/arch/x86/entry/entry_64.S
+++ b/arch/x86/entry/entry_64.S
@@ -735,13 +735,9 @@ SYM_FUNC_START(asm_load_gs_index)
 	swapgs
 	FRAME_END
 	ret
-SYM_FUNC_END(asm_load_gs_index)
-EXPORT_SYMBOL(asm_load_gs_index)
 
-	_ASM_EXTABLE(.Lgs_change, .Lbad_gs)
-	.section .fixup, "ax"
 	/* running with kernelgs */
-SYM_CODE_START_LOCAL_NOALIGN(.Lbad_gs)
+.Lbad_gs:
 	swapgs					/* switch back to user gs */
 .macro ZAP_GS
 	/* This can't be a string because the preprocessor needs to see it. */
@@ -752,8 +748,11 @@ SYM_CODE_START_LOCAL_NOALIGN(.Lbad_gs)
 	xorl	%eax, %eax
 	movl	%eax, %gs
 	jmp	2b
-SYM_CODE_END(.Lbad_gs)
-	.previous
+
+	_ASM_EXTABLE(.Lgs_change, .Lbad_gs)
+
+SYM_FUNC_END(asm_load_gs_index)
+EXPORT_SYMBOL(asm_load_gs_index)
 
 #ifdef CONFIG_XEN_PV
 /*

[RFC][PATCH 06/22] x86,entry_32: Remove .fixup usage

[Peter Zijlstra]

Where possible, push the .fixup into code, at the tail of functions.

This is hard for macros since they're used in multiple functions,
therefore introduce a new extable handler for pop-segment.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/x86/entry/entry_32.S                  |   28 ++++++++--------------------
 arch/x86/include/asm/extable_fixup_types.h |    2 ++
 arch/x86/mm/extable.c                      |    9 +++++++++
 3 files changed, 19 insertions(+), 20 deletions(-)

--- a/arch/x86/entry/entry_32.S
+++ b/arch/x86/entry/entry_32.S
@@ -270,17 +270,9 @@
 3:	popl	%fs
 	addl	$(4 + \pop), %esp	/* pop the unused "gs" slot */
 	IRET_FRAME
-.pushsection .fixup, "ax"
-4:	movl	$0, (%esp)
-	jmp	1b
-5:	movl	$0, (%esp)
-	jmp	2b
-6:	movl	$0, (%esp)
-	jmp	3b
-.popsection
-	_ASM_EXTABLE(1b, 4b)
-	_ASM_EXTABLE(2b, 5b)
-	_ASM_EXTABLE(3b, 6b)
+	_ASM_EXTABLE_TYPE(1b, 1b, EX_TYPE_POP_SEG)
+	_ASM_EXTABLE_TYPE(2b, 2b, EX_TYPE_POP_SEG)
+	_ASM_EXTABLE_TYPE(3b, 3b, EX_TYPE_POP_SEG)
 .endm
 
 .macro RESTORE_ALL_NMI cr3_reg:req pop=0
@@ -925,10 +917,8 @@ SYM_FUNC_START(entry_SYSENTER_32)
 	sti
 	sysexit
 
-.pushsection .fixup, "ax"
-2:	movl	$0, PT_FS(%esp)
-	jmp	1b
-.popsection
+2:	movl    $0, PT_FS(%esp)
+	jmp     1b
 	_ASM_EXTABLE(1b, 2b)
 
 .Lsysenter_fix_flags:
@@ -996,8 +986,7 @@ SYM_FUNC_START(entry_INT80_32)
 	 */
 	iret
 
-.section .fixup, "ax"
-SYM_CODE_START(asm_iret_error)
+.Lasm_iret_error
 	pushl	$0				# no error code
 	pushl	$iret_error
 
@@ -1014,9 +1003,8 @@ SYM_CODE_START(asm_iret_error)
 #endif
 
 	jmp	handle_exception
-SYM_CODE_END(asm_iret_error)
-.previous
-	_ASM_EXTABLE(.Lirq_return, asm_iret_error)
+
+	_ASM_EXTABLE(.Lirq_return, .Lasm_iret_error)
 SYM_FUNC_END(entry_INT80_32)
 
 .macro FIXUP_ESPFIX_STACK
--- a/arch/x86/include/asm/extable_fixup_types.h
+++ b/arch/x86/include/asm/extable_fixup_types.h
@@ -19,4 +19,6 @@
 #define	EX_TYPE_DEFAULT_MCE_SAFE	12
 #define	EX_TYPE_FAULT_MCE_SAFE		13
 
+#define EX_TYPE_POP_SEG			14
+
 #endif
--- a/arch/x86/mm/extable.c
+++ b/arch/x86/mm/extable.c
@@ -99,6 +99,13 @@ static bool ex_handler_clear_fs(const st
 	return ex_handler_default(fixup, regs);
 }
 
+static bool ex_handler_pop_seg(const struct exception_table_entry *fixup,
+			       struct pt_regs *regs)
+{
+	*((unsigned int *)regs->sp) = 0;
+	return ex_handler_default(fixup, regs);
+}
+
 int ex_get_fixup_type(unsigned long ip)
 {
 	const struct exception_table_entry *e = search_exception_tables(ip);
@@ -156,6 +163,8 @@ int fixup_exception(struct pt_regs *regs
 	case EX_TYPE_WRMSR_IN_MCE:
 		ex_handler_msr_mce(regs, true);
 		break;
+	case EX_TYPE_POP_SEG:
+		return ex_handler_pop_seg(e, regs);
 	}
 	BUG();
 }

Re: [RFC][PATCH 06/22] x86,entry_32: Remove .fixup usage

[Josh Poimboeuf]

On Thu, Nov 04, 2021 at 05:47:35PM +0100, Peter Zijlstra wrote:
> +++ b/arch/x86/include/asm/extable_fixup_types.h
> @@ -19,4 +19,6 @@
>  #define	EX_TYPE_DEFAULT_MCE_SAFE	12
>  #define	EX_TYPE_FAULT_MCE_SAFE		13
>  
> +#define EX_TYPE_POP_SEG			14
> +

This looks funky in the patch (but not in the editor), those other ones
have a tab after '#define'.

>  #endif
> --- a/arch/x86/mm/extable.c
> +++ b/arch/x86/mm/extable.c
> @@ -99,6 +99,13 @@ static bool ex_handler_clear_fs(const st
>  	return ex_handler_default(fixup, regs);
>  }
>  
> +static bool ex_handler_pop_seg(const struct exception_table_entry *fixup,
> +			       struct pt_regs *regs)
> +{
> +	*((unsigned int *)regs->sp) = 0;
> +	return ex_handler_default(fixup, regs);
> +}

Clever.  Should be "unsigned long" in case this ever gets used by
64-bit?  Also, I'd suggest a short comment.

-- 
Josh

Re: [RFC][PATCH 06/22] x86,entry_32: Remove .fixup usage

[Peter Zijlstra]

On Thu, Nov 04, 2021 at 01:39:33PM -0700, Josh Poimboeuf wrote:
> On Thu, Nov 04, 2021 at 05:47:35PM +0100, Peter Zijlstra wrote:
> > +++ b/arch/x86/include/asm/extable_fixup_types.h
> > @@ -19,4 +19,6 @@
> >  #define	EX_TYPE_DEFAULT_MCE_SAFE	12
> >  #define	EX_TYPE_FAULT_MCE_SAFE		13
> >  
> > +#define EX_TYPE_POP_SEG			14
> > +
> 
> This looks funky in the patch (but not in the editor), those other ones
> have a tab after '#define'.

Argh, I thought I'd fixed all those :/ I'll go make it a tab.

> >  #endif
> > --- a/arch/x86/mm/extable.c
> > +++ b/arch/x86/mm/extable.c
> > @@ -99,6 +99,13 @@ static bool ex_handler_clear_fs(const st
> >  	return ex_handler_default(fixup, regs);
> >  }
> >  
> > +static bool ex_handler_pop_seg(const struct exception_table_entry *fixup,
> > +			       struct pt_regs *regs)
> > +{
> > +	*((unsigned int *)regs->sp) = 0;
> > +	return ex_handler_default(fixup, regs);
> > +}
> 
> Clever.  Should be "unsigned long" in case this ever gets used by
> 64-bit?  Also, I'd suggest a short comment.

I'll do both.

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

[Peter Zijlstra]

In order to remove further .fixup usage, extend the extable
infrastructure to take additional information from the extable entry
sites.

Specifically add _ASM_EXTABLE_TYPE_REG() and EX_TYPE_IMM_REG that
extend the existing _ASM_EXTABLE_TYPE() by taking an additional
register argument and encoding that and an s16 immediate into the
existing s32 type field. This limits the actual types to the first
byte, 255 seem plenty.

Also add a few flags into the type word, specifically CLR_AX and
CLR_DX which clear the return and extended return register.

Notes:
 - due to the % in our register names it's hard to make it more
   generally usable as arm64 did.
 - the s16 is far larger than used in these patches, future extentions
   can easily shrink this to get more bits.
 - without the bitfield fix this will not compile, because: 0xFF > -1
   and we can't even extract the TYPE field.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/x86/include/asm/asm.h                 |   27 +++++++++++++
 arch/x86/include/asm/extable_fixup_types.h |   14 ++++++
 arch/x86/mm/extable.c                      |   59 +++++++++++++++++++++++++++--
 3 files changed, 96 insertions(+), 4 deletions(-)

--- a/arch/x86/include/asm/asm.h
+++ b/arch/x86/include/asm/asm.h
@@ -152,6 +152,25 @@
 
 #else /* ! __ASSEMBLY__ */
 
+asm(
+"	.macro extable_type_reg type:req reg:req\n"
+"	.set regnr, 0\n"
+"	.irp rs,rax,rcx,rdx,rbx,rsp,rbp,rsi,rdi,r8,r9,r10,r11,r12,r13,r14,r15\n"
+"	.ifc \\reg, %\\rs\n"
+"	.long \\type + (regnr << 8)\n"
+"	.endif\n"
+"	.set regnr, regnr+1\n"
+"	.endr\n"
+"	.set regnr, 0\n"
+"	.irp rs,eax,ecx,edx,ebx,esp,ebp,esi,edi,r8d,r9d,r10d,r11d,r12d,r13d,r14d,r15d\n"
+"	.ifc \\reg, %\\rs\n"
+"	.long \\type + (regnr << 8)\n"
+"	.endif\n"
+"	.set regnr, regnr+1\n"
+"	.endr\n"
+"	.endm\n"
+);
+
 # define _ASM_EXTABLE_TYPE(from, to, type)			\
 	" .pushsection \"__ex_table\",\"a\"\n"			\
 	" .balign 4\n"						\
@@ -160,6 +179,14 @@
 	" .long " __stringify(type) " \n"			\
 	" .popsection\n"
 
+# define _ASM_EXTABLE_TYPE_REG(from, to, type, reg)				\
+	" .pushsection \"__ex_table\",\"a\"\n"					\
+	" .balign 4\n"								\
+	" .long (" #from ") - .\n"						\
+	" .long (" #to ") - .\n"						\
+	"extable_type_reg reg=" __stringify(reg) ", type=" __stringify(type) " \n"\
+	" .popsection\n"
+
 /* For C file, we already have NOKPROBE_SYMBOL macro */
 
 /*
--- a/arch/x86/include/asm/extable_fixup_types.h
+++ b/arch/x86/include/asm/extable_fixup_types.h
@@ -2,6 +2,19 @@
 #ifndef _ASM_X86_EXTABLE_FIXUP_TYPES_H
 #define _ASM_X86_EXTABLE_FIXUP_TYPES_H
 
+#define EX_TYPE_REG_SHIFT		8
+#define EX_TYPE_FLAG_SHIFT		12
+#define EX_TYPE_IMM_SHIFT		16
+
+#define EX_TYPE_FLAG(flag)		((flag) << EX_TYPE_FLAG_SHIFT)
+#define EX_TYPE_IMM(imm)		((imm) << EX_TYPE_IMM_SHIFT)
+
+/* flags */
+#define EX_FLAG_CLR_AX			EX_TYPE_FLAG(1)
+#define EX_FLAG_CLR_DX			EX_TYPE_FLAG(2)
+#define EX_FLAG_CLR_AX_DX		EX_TYPE_FLAG(3)
+
+/* types */
 #define	EX_TYPE_NONE			 0
 #define	EX_TYPE_DEFAULT			 1
 #define	EX_TYPE_FAULT			 2
@@ -20,5 +33,6 @@
 #define	EX_TYPE_FAULT_MCE_SAFE		13
 
 #define EX_TYPE_POP_SEG			14
+#define EX_TYPE_IMM_REG			15 /* reg := (long)imm */
 
 #endif
--- a/arch/x86/mm/extable.c
+++ b/arch/x86/mm/extable.c
@@ -2,6 +2,7 @@
 #include <linux/extable.h>
 #include <linux/uaccess.h>
 #include <linux/sched/debug.h>
+#include <linux/bitfield.h>
 #include <xen/xen.h>
 
 #include <asm/fpu/api.h>
@@ -9,16 +10,47 @@
 #include <asm/traps.h>
 #include <asm/kdebug.h>
 
+static inline unsigned long *pt_regs_nr(struct pt_regs *regs, int nr)
+{
+	/* because having pt_regs in machine order was too much to ask */
+	switch (nr) {
+	case 0:		return &regs->ax;
+	case 1:		return &regs->cx;
+	case 2:		return &regs->dx;
+	case 3:		return &regs->bx;
+	case 4:		return &regs->sp;
+	case 5:		return &regs->bp;
+	case 6:		return &regs->si;
+	case 7:		return &regs->di;
+#ifdef CONFIG_X86_64
+	case 8:		return &regs->r8;
+	case 9:		return &regs->r9;
+	case 10:	return &regs->r10;
+	case 11:	return &regs->r11;
+	case 12:	return &regs->r12;
+	case 13:	return &regs->r13;
+	case 14:	return &regs->r14;
+	case 15:	return &regs->r15;
+#endif
+	default:	return NULL;
+	}
+}
+
 static inline unsigned long
 ex_fixup_addr(const struct exception_table_entry *x)
 {
 	return (unsigned long)&x->fixup + x->fixup;
 }
 
-static bool ex_handler_default(const struct exception_table_entry *fixup,
+static bool ex_handler_default(const struct exception_table_entry *e,
 			       struct pt_regs *regs)
 {
-	regs->ip = ex_fixup_addr(fixup);
+	if (e->type & EX_FLAG_CLR_AX)
+		regs->ax = 0;
+	if (e->type & EX_FLAG_CLR_DX)
+		regs->dx = 0;
+
+	regs->ip = ex_fixup_addr(e);
 	return true;
 }
 
@@ -106,17 +138,30 @@ static bool ex_handler_pop_seg(const str
 	return ex_handler_default(fixup, regs);
 }
 
+static bool ex_handler_imm_reg(const struct exception_table_entry *fixup,
+			       struct pt_regs *regs, int reg, int imm)
+{
+	*pt_regs_nr(regs, reg) = (long)imm;
+	return ex_handler_default(fixup, regs);
+}
+
+#define EX_TYPE_MASK	0x000000FF
+#define EX_REG_MASK	0x00000F00
+#define EX_FLAG_MASK	0x0000F000
+#define EX_IMM_MASK	0xFFFF0000
+
 int ex_get_fixup_type(unsigned long ip)
 {
 	const struct exception_table_entry *e = search_exception_tables(ip);
 
-	return e ? e->type : EX_TYPE_NONE;
+	return e ? FIELD_GET(EX_TYPE_MASK, e->type) : EX_TYPE_NONE;
 }
 
 int fixup_exception(struct pt_regs *regs, int trapnr, unsigned long error_code,
 		    unsigned long fault_addr)
 {
 	const struct exception_table_entry *e;
+	int type, reg, imm;
 
 #ifdef CONFIG_PNPBIOS
 	if (unlikely(SEGMENT_IS_PNP_CODE(regs->cs))) {
@@ -136,7 +181,11 @@ int fixup_exception(struct pt_regs *regs
 	if (!e)
 		return 0;
 
-	switch (e->type) {
+	type = FIELD_GET(EX_TYPE_MASK, e->type);
+	reg  = FIELD_GET(EX_REG_MASK,  e->type);
+	imm  = FIELD_GET(EX_IMM_MASK,  e->type);
+
+	switch (type) {
 	case EX_TYPE_DEFAULT:
 	case EX_TYPE_DEFAULT_MCE_SAFE:
 		return ex_handler_default(e, regs);
@@ -165,6 +214,8 @@ int fixup_exception(struct pt_regs *regs
 		break;
 	case EX_TYPE_POP_SEG:
 		return ex_handler_pop_seg(e, regs);
+	case EX_TYPE_IMM_REG:
+		return ex_handler_imm_reg(e, regs, reg, imm);
 	}
 	BUG();
 }

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

[Josh Poimboeuf]

On Thu, Nov 04, 2021 at 05:47:36PM +0100, Peter Zijlstra wrote:
> +asm(
> +"	.macro extable_type_reg type:req reg:req\n"
> +"	.set regnr, 0\n"
> +"	.irp rs,rax,rcx,rdx,rbx,rsp,rbp,rsi,rdi,r8,r9,r10,r11,r12,r13,r14,r15\n"
> +"	.ifc \\reg, %\\rs\n"
> +"	.long \\type + (regnr << 8)\n"
> +"	.endif\n"
> +"	.set regnr, regnr+1\n"
> +"	.endr\n"
> +"	.set regnr, 0\n"
> +"	.irp rs,eax,ecx,edx,ebx,esp,ebp,esi,edi,r8d,r9d,r10d,r11d,r12d,r13d,r14d,r15d\n"
> +"	.ifc \\reg, %\\rs\n"
> +"	.long \\type + (regnr << 8)\n"
> +"	.endif\n"
> +"	.set regnr, regnr+1\n"
> +"	.endr\n"
> +"	.endm\n"
> +);

How about some error checking to detect a typo, or a forgotten '%':

diff --git a/arch/x86/include/asm/asm.h b/arch/x86/include/asm/asm.h
index 5d0ff8c60983..95bb23082b87 100644
--- a/arch/x86/include/asm/asm.h
+++ b/arch/x86/include/asm/asm.h
@@ -154,9 +154,11 @@
 
 asm(
 "	.macro extable_type_reg type:req reg:req\n"
+"	.set found, 0\n"
 "	.set regnr, 0\n"
 "	.irp rs,rax,rcx,rdx,rbx,rsp,rbp,rsi,rdi,r8,r9,r10,r11,r12,r13,r14,r15\n"
 "	.ifc \\reg, %\\rs\n"
+"	.set found, found+1\n"
 "	.long \\type + (regnr << 8)\n"
 "	.endif\n"
 "	.set regnr, regnr+1\n"
@@ -164,10 +166,14 @@ asm(
 "	.set regnr, 0\n"
 "	.irp rs,eax,ecx,edx,ebx,esp,ebp,esi,edi,r8d,r9d,r10d,r11d,r12d,r13d,r14d,r15d\n"
 "	.ifc \\reg, %\\rs\n"
+"	.set found, found+1\n"
 "	.long \\type + (regnr << 8)\n"
 "	.endif\n"
 "	.set regnr, regnr+1\n"
 "	.endr\n"
+"	.if (found != 1)\n"
+"	.error \"extable_type_reg: bad register argument\"\n"
+"	.endif\n"
 "	.endm\n"
 );
 
> +#define EX_FLAG_CLR_AX			EX_TYPE_FLAG(1)
> +#define EX_FLAG_CLR_DX			EX_TYPE_FLAG(2)
> +#define EX_FLAG_CLR_AX_DX		EX_TYPE_FLAG(3)

I'd like to buy two vowels: CL̲E̲AR

(I hope that Wheel of Fortune reference isn't too US-centric.)

> +static inline unsigned long *pt_regs_nr(struct pt_regs *regs, int nr)
> +{
> +	/* because having pt_regs in machine order was too much to ask */
> +	switch (nr) {
> +	case 0:		return &regs->ax;
> +	case 1:		return &regs->cx;
> +	case 2:		return &regs->dx;
> +	case 3:		return &regs->bx;
> +	case 4:		return &regs->sp;
> +	case 5:		return &regs->bp;
> +	case 6:		return &regs->si;
> +	case 7:		return &regs->di;
> +#ifdef CONFIG_X86_64
> +	case 8:		return &regs->r8;
> +	case 9:		return &regs->r9;
> +	case 10:	return &regs->r10;
> +	case 11:	return &regs->r11;
> +	case 12:	return &regs->r12;
> +	case 13:	return &regs->r13;
> +	case 14:	return &regs->r14;
> +	case 15:	return &regs->r15;
> +#endif
> +	default:	return NULL;
> +	}
> +}

Instead of all this craziness, why not just admit defeat and put them in
pt_regs order in the 'extable_type_reg' macro?

> +static bool ex_handler_imm_reg(const struct exception_table_entry *fixup,
> +			       struct pt_regs *regs, int reg, int imm)
> +{
> +	*pt_regs_nr(regs, reg) = (long)imm;
> +	return ex_handler_default(fixup, regs);
> +}
> +
> +#define EX_TYPE_MASK	0x000000FF
> +#define EX_REG_MASK	0x00000F00
> +#define EX_FLAG_MASK	0x0000F000
> +#define EX_IMM_MASK	0xFFFF0000

To avoid mismatches these should probably be in the header file next to
EX_TYPE_*_SHIFT?

> +
>  int ex_get_fixup_type(unsigned long ip)
>  {
>  	const struct exception_table_entry *e = search_exception_tables(ip);
>  
> -	return e ? e->type : EX_TYPE_NONE;
> +	return e ? FIELD_GET(EX_TYPE_MASK, e->type) : EX_TYPE_NONE;

Maybe the 'type' field should be renamed, to better represent its new
use, and to try to discourage direct access.  Not that I have any good
ideas.  Some not-so-good ideas: "handler", "flags", "_type".

-- 
Josh

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

[Peter Zijlstra]

On Thu, Nov 04, 2021 at 02:49:35PM -0700, Josh Poimboeuf wrote:
> On Thu, Nov 04, 2021 at 05:47:36PM +0100, Peter Zijlstra wrote:
> > +asm(
> > +"	.macro extable_type_reg type:req reg:req\n"
> > +"	.set regnr, 0\n"
> > +"	.irp rs,rax,rcx,rdx,rbx,rsp,rbp,rsi,rdi,r8,r9,r10,r11,r12,r13,r14,r15\n"
> > +"	.ifc \\reg, %\\rs\n"
> > +"	.long \\type + (regnr << 8)\n"
> > +"	.endif\n"
> > +"	.set regnr, regnr+1\n"
> > +"	.endr\n"
> > +"	.set regnr, 0\n"
> > +"	.irp rs,eax,ecx,edx,ebx,esp,ebp,esi,edi,r8d,r9d,r10d,r11d,r12d,r13d,r14d,r15d\n"
> > +"	.ifc \\reg, %\\rs\n"
> > +"	.long \\type + (regnr << 8)\n"
> > +"	.endif\n"
> > +"	.set regnr, regnr+1\n"
> > +"	.endr\n"
> > +"	.endm\n"
> > +);
> 
> How about some error checking to detect a typo, or a forgotten '%':
> 
> diff --git a/arch/x86/include/asm/asm.h b/arch/x86/include/asm/asm.h
> index 5d0ff8c60983..95bb23082b87 100644
> --- a/arch/x86/include/asm/asm.h
> +++ b/arch/x86/include/asm/asm.h
> @@ -154,9 +154,11 @@
>  
>  asm(
>  "	.macro extable_type_reg type:req reg:req\n"
> +"	.set found, 0\n"
>  "	.set regnr, 0\n"
>  "	.irp rs,rax,rcx,rdx,rbx,rsp,rbp,rsi,rdi,r8,r9,r10,r11,r12,r13,r14,r15\n"
>  "	.ifc \\reg, %\\rs\n"
> +"	.set found, found+1\n"
>  "	.long \\type + (regnr << 8)\n"
>  "	.endif\n"
>  "	.set regnr, regnr+1\n"
> @@ -164,10 +166,14 @@ asm(
>  "	.set regnr, 0\n"
>  "	.irp rs,eax,ecx,edx,ebx,esp,ebp,esi,edi,r8d,r9d,r10d,r11d,r12d,r13d,r14d,r15d\n"
>  "	.ifc \\reg, %\\rs\n"
> +"	.set found, found+1\n"
>  "	.long \\type + (regnr << 8)\n"
>  "	.endif\n"
>  "	.set regnr, regnr+1\n"
>  "	.endr\n"
> +"	.if (found != 1)\n"
> +"	.error \"extable_type_reg: bad register argument\"\n"
> +"	.endif\n"
>  "	.endm\n"
>  );

Ooh, nice! I'd actually triggered that once. At the time it was objtool
complaining .extable size wasn't a multiple of 12. Took me a while to
figure out which one had gone missing.

> > +#define EX_FLAG_CLR_AX			EX_TYPE_FLAG(1)
> > +#define EX_FLAG_CLR_DX			EX_TYPE_FLAG(2)
> > +#define EX_FLAG_CLR_AX_DX		EX_TYPE_FLAG(3)
> 
> I'd like to buy two vowels: CL̲E̲AR

Yes, can do. The macro name was longer earlier on, but in this form we
can add the two characters.

> (I hope that Wheel of Fortune reference isn't too US-centric.)

Sadly not, TV was infested with crap like that here in .nl as well.

> > +static inline unsigned long *pt_regs_nr(struct pt_regs *regs, int nr)
> > +{
> > +	/* because having pt_regs in machine order was too much to ask */
> > +	switch (nr) {
> > +	case 0:		return &regs->ax;
> > +	case 1:		return &regs->cx;
> > +	case 2:		return &regs->dx;
> > +	case 3:		return &regs->bx;
> > +	case 4:		return &regs->sp;
> > +	case 5:		return &regs->bp;
> > +	case 6:		return &regs->si;
> > +	case 7:		return &regs->di;
> > +#ifdef CONFIG_X86_64
> > +	case 8:		return &regs->r8;
> > +	case 9:		return &regs->r9;
> > +	case 10:	return &regs->r10;
> > +	case 11:	return &regs->r11;
> > +	case 12:	return &regs->r12;
> > +	case 13:	return &regs->r13;
> > +	case 14:	return &regs->r14;
> > +	case 15:	return &regs->r15;
> > +#endif
> > +	default:	return NULL;
> > +	}
> > +}
> 
> Instead of all this craziness, why not just admit defeat and put them in
> pt_regs order in the 'extable_type_reg' macro?

That makes the macro different between 32bit and 64bit :/ Also, I just
found another, extant, copy of this function, so I can get rid of it and
use that one, see get_reg_offset() in insn-eval.c

> > +static bool ex_handler_imm_reg(const struct exception_table_entry *fixup,
> > +			       struct pt_regs *regs, int reg, int imm)
> > +{
> > +	*pt_regs_nr(regs, reg) = (long)imm;
> > +	return ex_handler_default(fixup, regs);
> > +}
> > +
> > +#define EX_TYPE_MASK	0x000000FF
> > +#define EX_REG_MASK	0x00000F00
> > +#define EX_FLAG_MASK	0x0000F000
> > +#define EX_IMM_MASK	0xFFFF0000
> 
> To avoid mismatches these should probably be in the header file next to
> EX_TYPE_*_SHIFT?

Can do.

> > +
> >  int ex_get_fixup_type(unsigned long ip)
> >  {
> >  	const struct exception_table_entry *e = search_exception_tables(ip);
> >  
> > -	return e ? e->type : EX_TYPE_NONE;
> > +	return e ? FIELD_GET(EX_TYPE_MASK, e->type) : EX_TYPE_NONE;
> 
> Maybe the 'type' field should be renamed, to better represent its new
> use, and to try to discourage direct access.  Not that I have any good
> ideas.  Some not-so-good ideas: "handler", "flags", "_type".

How about the non-descript: "data" ?

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

[Mark Rutland]

On Fri, Nov 05, 2021 at 08:54:00AM +0100, Peter Zijlstra wrote:
> On Thu, Nov 04, 2021 at 02:49:35PM -0700, Josh Poimboeuf wrote:
> > On Thu, Nov 04, 2021 at 05:47:36PM +0100, Peter Zijlstra wrote:
> > >  int ex_get_fixup_type(unsigned long ip)
> > >  {
> > >  	const struct exception_table_entry *e = search_exception_tables(ip);
> > >  
> > > -	return e ? e->type : EX_TYPE_NONE;
> > > +	return e ? FIELD_GET(EX_TYPE_MASK, e->type) : EX_TYPE_NONE;
> > 
> > Maybe the 'type' field should be renamed, to better represent its new
> > use, and to try to discourage direct access.  Not that I have any good
> > ideas.  Some not-so-good ideas: "handler", "flags", "_type".
> 
> How about the non-descript: "data" ?

FWIW, I was going to have a single `data` or `info` field on arm64, and I only
went with separate 16-bit `type` and `data` fields becuase it was *marginally*
simpler. If we needed more data bits in future I'd probably collapse `type`
into the `data` field. So you'd be in good company. ;)

On arm64 I'd named the masks EX_DATA_* to make the hierarchy clear, so maybe
you'd want to s/EX_TYPE_MASK/EX_DATA_TYPE/ for similar reasons.

Thanks,
Mark.

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

[Sean Christopherson]

On Thu, Nov 04, 2021, Peter Zijlstra wrote:
> +static bool ex_handler_imm_reg(const struct exception_table_entry *fixup,
> +			       struct pt_regs *regs, int reg, int imm)
> +{
> +	*pt_regs_nr(regs, reg) = (long)imm;

This doesn't work for the -EFAULT case because despite being an 'int', @imm is
effectively an 'unsigned short'.  More below.

> +	return ex_handler_default(fixup, regs);
> +}
> +
> +#define EX_TYPE_MASK	0x000000FF
> +#define EX_REG_MASK	0x00000F00
> +#define EX_FLAG_MASK	0x0000F000
> +#define EX_IMM_MASK	0xFFFF0000
> +
>  int ex_get_fixup_type(unsigned long ip)
>  {
>  	const struct exception_table_entry *e = search_exception_tables(ip);
>  
> -	return e ? e->type : EX_TYPE_NONE;
> +	return e ? FIELD_GET(EX_TYPE_MASK, e->type) : EX_TYPE_NONE;
>  }
>  
>  int fixup_exception(struct pt_regs *regs, int trapnr, unsigned long error_code,
>  		    unsigned long fault_addr)
>  {
>  	const struct exception_table_entry *e;
> +	int type, reg, imm;
>  
>  #ifdef CONFIG_PNPBIOS
>  	if (unlikely(SEGMENT_IS_PNP_CODE(regs->cs))) {
> @@ -136,7 +181,11 @@ int fixup_exception(struct pt_regs *regs
>  	if (!e)
>  		return 0;
>  
> -	switch (e->type) {
> +	type = FIELD_GET(EX_TYPE_MASK, e->type);
> +	reg  = FIELD_GET(EX_REG_MASK,  e->type);
> +	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.

This hack makes things work as expected.

diff --git a/arch/x86/mm/extable.c b/arch/x86/mm/extable.c
index 9dc0685366e5..182d62c11404 100644
--- a/arch/x86/mm/extable.c
+++ b/arch/x86/mm/extable.c
@@ -223,7 +223,7 @@ int fixup_exception(struct pt_regs *regs, int trapnr, unsigned long error_code,

        type = FIELD_GET(EX_TYPE_MASK, e->type);
        reg  = FIELD_GET(EX_REG_MASK,  e->type);
-       imm  = FIELD_GET(EX_IMM_MASK,  e->type);
+       imm  = (int)(short)FIELD_GET(EX_IMM_MASK,  e->type);

        switch (type) {
        case EX_TYPE_DEFAULT:


> +
> +	switch (type) {
>  	case EX_TYPE_DEFAULT:
>  	case EX_TYPE_DEFAULT_MCE_SAFE:
>  		return ex_handler_default(e, regs);
> @@ -165,6 +214,8 @@ int fixup_exception(struct pt_regs *regs
>  		break;
>  	case EX_TYPE_POP_SEG:
>  		return ex_handler_pop_seg(e, regs);
> +	case EX_TYPE_IMM_REG:
> +		return ex_handler_imm_reg(e, regs, reg, imm);
>  	}
>  	BUG();
>  }
> 
> 

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

[Peter Zijlstra]

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?

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

[Sean Christopherson]

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:

  E1 has a signed type and a negative value, the resulting value is implementation-defined.

gcc-10 generates a bare "shr", i.e. doesn't special case negative values, and "shr"
is explicitly defined as an unsigned divide.

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

[Peter Zijlstra]

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.

> 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...

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

[Sean Christopherson]

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.

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

[Peter Zijlstra]

On Fri, Nov 05, 2021 at 07:47:22PM +0000, Sean Christopherson wrote:
> On Fri, Nov 05, 2021, Peter Zijlstra wrote:
> > On Fri, Nov 05, 2021 at 07:17:20PM +0000, Sean Christopherson wrote:

> > 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".

Right... I think i remember running into that before. I think I yelled
at people at the time for that not being very consistent. If we
explicitly state we want 2s complement, it had damn well be everywhere.

> > > 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.

Yeah, the canonical pattern for sign extending an n-bit int is:

	(int)(x << (32-n)) >> (32-n)

I think we have macros for that somehwere, but I can never remember what
they're called.

> 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.

Bah, you're right. The compiler is free to interpret a hex constant as
either. At the same time there's only an unsigned suffix, so while we
can force it unsigned, we can't explicitly construct a signed hex
constant.

That's really unfortunate that is... 6.4.4.1 item 5 of the C99 spec
covers this gem :-( I suppose I'll go stick that (int) cast in the
EX_IMM_MASK definition or something.

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

[Peter Zijlstra]

On Fri, Nov 05, 2021 at 09:15:57PM +0100, Peter Zijlstra wrote:

> That's really unfortunate that is... 6.4.4.1 item 5 of the C99 spec
> covers this gem :-( I suppose I'll go stick that (int) cast in the
> EX_IMM_MASK definition or something.

--- a/arch/x86/include/asm/extable_fixup_types.h
+++ b/arch/x86/include/asm/extable_fixup_types.h
@@ -2,10 +2,15 @@
 #ifndef _ASM_X86_EXTABLE_FIXUP_TYPES_H
 #define _ASM_X86_EXTABLE_FIXUP_TYPES_H
 
+/*
+ * Our IMM is signed, as such it must live at the top end of the word. Also,
+ * since C99 hex constants are of ambigious type, force cast the mask to 'int'
+ * so that FIELD_GET() will DTRT and sign extend the value when it extracts it.
+ */
 #define EX_DATA_TYPE_MASK		0x000000FF
 #define EX_DATA_REG_MASK		0x00000F00
 #define EX_DATA_FLAG_MASK		0x0000F000
-#define EX_DATA_IMM_MASK		0xFFFF0000
+#define EX_DATA_IMM_MASK		((int)0xFFFF0000)
 
 #define EX_DATA_REG_SHIFT		8
 #define EX_DATA_FLAG_SHIFT		12

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

[Sean Christopherson]

On Fri, Nov 05, 2021, Peter Zijlstra wrote:
> On Fri, Nov 05, 2021 at 09:15:57PM +0100, Peter Zijlstra wrote:
> 
> > That's really unfortunate that is... 6.4.4.1 item 5 of the C99 spec
> > covers this gem :-( I suppose I'll go stick that (int) cast in the
> > EX_IMM_MASK definition or something.
> 
> --- a/arch/x86/include/asm/extable_fixup_types.h
> +++ b/arch/x86/include/asm/extable_fixup_types.h
> @@ -2,10 +2,15 @@
>  #ifndef _ASM_X86_EXTABLE_FIXUP_TYPES_H
>  #define _ASM_X86_EXTABLE_FIXUP_TYPES_H
>  
> +/*
> + * Our IMM is signed, as such it must live at the top end of the word. Also,
> + * since C99 hex constants are of ambigious type, force cast the mask to 'int'
> + * so that FIELD_GET() will DTRT and sign extend the value when it extracts it.
> + */
>  #define EX_DATA_TYPE_MASK		0x000000FF
>  #define EX_DATA_REG_MASK		0x00000F00
>  #define EX_DATA_FLAG_MASK		0x0000F000
> -#define EX_DATA_IMM_MASK		0xFFFF0000
> +#define EX_DATA_IMM_MASK		((int)0xFFFF0000)
>  
>  #define EX_DATA_REG_SHIFT		8
>  #define EX_DATA_FLAG_SHIFT		12

Works on my end, thanks!

[RFC][PATCH 08/22] x86,msr: Remove .fixup usage

[Peter Zijlstra]

Rework the MSR accessors to remove .fixup usage. Add two new extable
types (to the 4 already existing msr ones) using the new register
infrastructure to record which register should get the error value.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/x86/include/asm/extable_fixup_types.h |   23 ++++++-------
 arch/x86/include/asm/msr.h                 |   26 ++++----------
 arch/x86/mm/extable.c                      |   51 +++++++++++++++--------------
 3 files changed, 47 insertions(+), 53 deletions(-)

--- a/arch/x86/include/asm/extable_fixup_types.h
+++ b/arch/x86/include/asm/extable_fixup_types.h
@@ -22,17 +22,16 @@
 #define	EX_TYPE_COPY			 4
 #define	EX_TYPE_CLEAR_FS		 5
 #define	EX_TYPE_FPU_RESTORE		 6
-#define	EX_TYPE_WRMSR			 7
-#define	EX_TYPE_RDMSR			 8
-#define	EX_TYPE_BPF			 9
-
-#define	EX_TYPE_WRMSR_IN_MCE		10
-#define	EX_TYPE_RDMSR_IN_MCE		11
-
-#define	EX_TYPE_DEFAULT_MCE_SAFE	12
-#define	EX_TYPE_FAULT_MCE_SAFE		13
-
-#define EX_TYPE_POP_SEG			14
-#define EX_TYPE_IMM_REG			15 /* reg := (long)imm */
+#define	EX_TYPE_BPF			 7
+#define	EX_TYPE_WRMSR			 8
+#define	EX_TYPE_RDMSR			 9
+#define	EX_TYPE_WRMSR_SAFE		10 /* reg := -EIO */
+#define	EX_TYPE_RDMSR_SAFE		11 /* reg := -EIO */
+#define	EX_TYPE_WRMSR_IN_MCE		12
+#define	EX_TYPE_RDMSR_IN_MCE		13
+#define	EX_TYPE_DEFAULT_MCE_SAFE	14
+#define	EX_TYPE_FAULT_MCE_SAFE		15
+#define	EX_TYPE_POP_SEG			16
+#define	EX_TYPE_IMM_REG			17 /* reg := (long)imm */
 
 #endif
--- a/arch/x86/include/asm/msr.h
+++ b/arch/x86/include/asm/msr.h
@@ -137,17 +137,11 @@ static inline unsigned long long native_
 {
 	DECLARE_ARGS(val, low, high);
 
-	asm volatile("2: rdmsr ; xor %[err],%[err]\n"
-		     "1:\n\t"
-		     ".section .fixup,\"ax\"\n\t"
-		     "3: mov %[fault],%[err]\n\t"
-		     "xorl %%eax, %%eax\n\t"
-		     "xorl %%edx, %%edx\n\t"
-		     "jmp 1b\n\t"
-		     ".previous\n\t"
-		     _ASM_EXTABLE(2b, 3b)
+	asm volatile("1: rdmsr ; xor %[err],%[err]\n"
+		     "2:\n\t"
+		     _ASM_EXTABLE_TYPE_REG(1b, 2b, EX_TYPE_RDMSR_SAFE, %[err])
 		     : [err] "=r" (*err), EAX_EDX_RET(val, low, high)
-		     : "c" (msr), [fault] "i" (-EIO));
+		     : "c" (msr));
 	if (tracepoint_enabled(read_msr))
 		do_trace_read_msr(msr, EAX_EDX_VAL(val, low, high), *err);
 	return EAX_EDX_VAL(val, low, high);
@@ -169,15 +163,11 @@ native_write_msr_safe(unsigned int msr,
 {
 	int err;
 
-	asm volatile("2: wrmsr ; xor %[err],%[err]\n"
-		     "1:\n\t"
-		     ".section .fixup,\"ax\"\n\t"
-		     "3:  mov %[fault],%[err] ; jmp 1b\n\t"
-		     ".previous\n\t"
-		     _ASM_EXTABLE(2b, 3b)
+	asm volatile("1: wrmsr ; xor %[err],%[err]\n"
+		     "2:\n\t"
+		     _ASM_EXTABLE_TYPE_REG(1b, 2b, EX_TYPE_WRMSR_SAFE, %[err])
 		     : [err] "=a" (err)
-		     : "c" (msr), "0" (low), "d" (high),
-		       [fault] "i" (-EIO)
+		     : "c" (msr), "0" (low), "d" (high)
 		     : "memory");
 	if (tracepoint_enabled(write_msr))
 		do_trace_write_msr(msr, ((u64)high << 32 | low), err);
--- a/arch/x86/mm/extable.c
+++ b/arch/x86/mm/extable.c
@@ -97,28 +97,29 @@ static bool ex_handler_copy(const struct
 	return ex_handler_fault(fixup, regs, trapnr);
 }
 
-static bool ex_handler_rdmsr_unsafe(const struct exception_table_entry *fixup,
-				    struct pt_regs *regs)
+static bool ex_handler_msr(const struct exception_table_entry *fixup,
+			   struct pt_regs *regs, bool wrmsr, bool safe, int reg)
 {
-	if (pr_warn_once("unchecked MSR access error: RDMSR from 0x%x at rIP: 0x%lx (%pS)\n",
+	if (!safe && wrmsr &&
+	    pr_warn_once("unchecked MSR access error: WRMSR to 0x%x (tried to write 0x%08x%08x) at rIP: 0x%lx (%pS)\n",
+			 (unsigned int)regs->cx, (unsigned int)regs->dx,
+			 (unsigned int)regs->ax,  regs->ip, (void *)regs->ip))
+		show_stack_regs(regs);
+
+	if (!safe && !wrmsr &&
+	    pr_warn_once("unchecked MSR access error: RDMSR from 0x%x at rIP: 0x%lx (%pS)\n",
 			 (unsigned int)regs->cx, regs->ip, (void *)regs->ip))
 		show_stack_regs(regs);
 
-	/* Pretend that the read succeeded and returned 0. */
-	regs->ax = 0;
-	regs->dx = 0;
-	return ex_handler_default(fixup, regs);
-}
+	if (!wrmsr) {
+		/* Pretend that the read succeeded and returned 0. */
+		regs->ax = 0;
+		regs->dx = 0;
+	}
 
-static bool ex_handler_wrmsr_unsafe(const struct exception_table_entry *fixup,
-				    struct pt_regs *regs)
-{
-	if (pr_warn_once("unchecked MSR access error: WRMSR to 0x%x (tried to write 0x%08x%08x) at rIP: 0x%lx (%pS)\n",
-			 (unsigned int)regs->cx, (unsigned int)regs->dx,
-			 (unsigned int)regs->ax,  regs->ip, (void *)regs->ip))
-		show_stack_regs(regs);
+	if (safe)
+		*pt_regs_nr(regs, reg) = -EIO;
 
-	/* Pretend that the write succeeded. */
 	return ex_handler_default(fixup, regs);
 }
 
@@ -200,18 +201,22 @@ int fixup_exception(struct pt_regs *regs
 		return ex_handler_clear_fs(e, regs);
 	case EX_TYPE_FPU_RESTORE:
 		return ex_handler_fprestore(e, regs);
-	case EX_TYPE_RDMSR:
-		return ex_handler_rdmsr_unsafe(e, regs);
-	case EX_TYPE_WRMSR:
-		return ex_handler_wrmsr_unsafe(e, regs);
 	case EX_TYPE_BPF:
 		return ex_handler_bpf(e, regs);
-	case EX_TYPE_RDMSR_IN_MCE:
-		ex_handler_msr_mce(regs, false);
-		break;
+	case EX_TYPE_WRMSR:
+		return ex_handler_msr(e, regs, true, false, reg);
+	case EX_TYPE_RDMSR:
+		return ex_handler_msr(e, regs, false, false, reg);
+	case EX_TYPE_WRMSR_SAFE:
+		return ex_handler_msr(e, regs, true, true, reg);
+	case EX_TYPE_RDMSR_SAFE:
+		return ex_handler_msr(e, regs, false, true, reg);
 	case EX_TYPE_WRMSR_IN_MCE:
 		ex_handler_msr_mce(regs, true);
 		break;
+	case EX_TYPE_RDMSR_IN_MCE:
+		ex_handler_msr_mce(regs, false);
+		break;
 	case EX_TYPE_POP_SEG:
 		return ex_handler_pop_seg(e, regs);
 	case EX_TYPE_IMM_REG:

[RFC][PATCH 09/22] x86,futex: Remove .fixup usage

[Peter Zijlstra]

Use the new EX_TYPE_IMM_REG to store -EFAULT into the designated 'ret'
register, this removes the need for anonymous .fixup code.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/x86/include/asm/extable_fixup_types.h |    2 ++
 arch/x86/include/asm/futex.h               |   28 ++++++++--------------------
 2 files changed, 10 insertions(+), 20 deletions(-)

--- a/arch/x86/include/asm/extable_fixup_types.h
+++ b/arch/x86/include/asm/extable_fixup_types.h
@@ -32,6 +32,8 @@
 #define	EX_TYPE_DEFAULT_MCE_SAFE	14
 #define	EX_TYPE_FAULT_MCE_SAFE		15
 #define	EX_TYPE_POP_SEG			16
+
 #define	EX_TYPE_IMM_REG			17 /* reg := (long)imm */
+#define	EX_TYPE_EFAULT_REG		(EX_TYPE_IMM_REG | EX_TYPE_IMM(-EFAULT))
 
 #endif
--- a/arch/x86/include/asm/futex.h
+++ b/arch/x86/include/asm/futex.h
@@ -17,13 +17,9 @@ do {								\
 	int oldval = 0, ret;					\
 	asm volatile("1:\t" insn "\n"				\
 		     "2:\n"					\
-		     "\t.section .fixup,\"ax\"\n"		\
-		     "3:\tmov\t%3, %1\n"			\
-		     "\tjmp\t2b\n"				\
-		     "\t.previous\n"				\
-		     _ASM_EXTABLE_UA(1b, 3b)			\
+		     _ASM_EXTABLE_TYPE_REG(1b, 2b, EX_TYPE_EFAULT_REG, %1) \
 		     : "=r" (oldval), "=r" (ret), "+m" (*uaddr)	\
-		     : "i" (-EFAULT), "0" (oparg), "1" (0));	\
+		     : "0" (oparg), "1" (0));	\
 	if (ret)						\
 		goto label;					\
 	*oval = oldval;						\
@@ -39,15 +35,11 @@ do {								\
 		     "3:\t" LOCK_PREFIX "cmpxchgl %3, %2\n"	\
 		     "\tjnz\t2b\n"				\
 		     "4:\n"					\
-		     "\t.section .fixup,\"ax\"\n"		\
-		     "5:\tmov\t%5, %1\n"			\
-		     "\tjmp\t4b\n"				\
-		     "\t.previous\n"				\
-		     _ASM_EXTABLE_UA(1b, 5b)			\
-		     _ASM_EXTABLE_UA(3b, 5b)			\
+		     _ASM_EXTABLE_TYPE_REG(1b, 4b, EX_TYPE_EFAULT_REG, %1) \
+		     _ASM_EXTABLE_TYPE_REG(3b, 4b, EX_TYPE_EFAULT_REG, %1) \
 		     : "=&a" (oldval), "=&r" (ret),		\
 		       "+m" (*uaddr), "=&r" (tem)		\
-		     : "r" (oparg), "i" (-EFAULT), "1" (0));	\
+		     : "r" (oparg), "1" (0));			\
 	if (ret)						\
 		goto label;					\
 	*oval = oldval;						\
@@ -95,15 +87,11 @@ static inline int futex_atomic_cmpxchg_i
 	if (!user_access_begin(uaddr, sizeof(u32)))
 		return -EFAULT;
 	asm volatile("\n"
-		"1:\t" LOCK_PREFIX "cmpxchgl %4, %2\n"
+		"1:\t" LOCK_PREFIX "cmpxchgl %3, %2\n"
 		"2:\n"
-		"\t.section .fixup, \"ax\"\n"
-		"3:\tmov     %3, %0\n"
-		"\tjmp     2b\n"
-		"\t.previous\n"
-		_ASM_EXTABLE_UA(1b, 3b)
+		_ASM_EXTABLE_TYPE_REG(1b, 2b, EX_TYPE_EFAULT_REG, %0) \
 		: "+r" (ret), "=a" (oldval), "+m" (*uaddr)
-		: "i" (-EFAULT), "r" (newval), "1" (oldval)
+		: "r" (newval), "1" (oldval)
 		: "memory"
 	);
 	user_access_end();

[RFC][PATCH 10/22] x86,uaccess: Remove .fixup usage

[Peter Zijlstra]

For the !CC_AS_ASM_GOTO_OUTPUT (aka. the legacy codepath), remove the
.fixup usage by employing both EX_TYPE_EFAULT_REG and EX_FLAG_CLR.
Like was already done for X86_32's version of __get_user_asm_u64()
use the "a" register for output, specifically so we can use CLR_AX.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/x86/include/asm/uaccess.h |   35 +++++++++++++++--------------------
 1 file changed, 15 insertions(+), 20 deletions(-)

--- a/arch/x86/include/asm/uaccess.h
+++ b/arch/x86/include/asm/uaccess.h
@@ -351,24 +351,22 @@ do {									\
 		     "1:	movl %[lowbits],%%eax\n"		\
 		     "2:	movl %[highbits],%%edx\n"		\
 		     "3:\n"						\
-		     ".section .fixup,\"ax\"\n"				\
-		     "4:	mov %[efault],%[errout]\n"		\
-		     "	xorl %%eax,%%eax\n"				\
-		     "	xorl %%edx,%%edx\n"				\
-		     "	jmp 3b\n"					\
-		     ".previous\n"					\
-		     _ASM_EXTABLE_UA(1b, 4b)				\
-		     _ASM_EXTABLE_UA(2b, 4b)				\
+		     _ASM_EXTABLE_TYPE_REG(1b, 3b, EX_TYPE_EFAULT_REG |	\
+					   EX_FLAG_CLR_AX_DX,		\
+					   %[errout])			\
+		     _ASM_EXTABLE_TYPE_REG(2b, 3b, EX_TYPE_EFAULT_REG |	\
+					   EX_FLAG_CLR_AX_DX,		\
+					   %[errout])			\
 		     : [errout] "=r" (retval),				\
 		       [output] "=&A"(x)				\
 		     : [lowbits] "m" (__m(__ptr)),			\
 		       [highbits] "m" __m(((u32 __user *)(__ptr)) + 1),	\
-		       [efault] "i" (-EFAULT), "0" (retval));		\
+		       "0" (retval));					\
 })
 
 #else
 #define __get_user_asm_u64(x, ptr, retval) \
-	 __get_user_asm(x, ptr, retval, "q", "=r")
+	 __get_user_asm(x, ptr, retval, "q", "=a")
 #endif
 
 #define __get_user_size(x, ptr, size, retval)				\
@@ -379,14 +377,14 @@ do {									\
 	__chk_user_ptr(ptr);						\
 	switch (size) {							\
 	case 1:								\
-		__get_user_asm(x_u8__, ptr, retval, "b", "=q");		\
+		__get_user_asm(x_u8__, ptr, retval, "b", "=a");		\
 		(x) = x_u8__;						\
 		break;							\
 	case 2:								\
-		__get_user_asm(x, ptr, retval, "w", "=r");		\
+		__get_user_asm(x, ptr, retval, "w", "=a");		\
 		break;							\
 	case 4:								\
-		__get_user_asm(x, ptr, retval, "l", "=r");		\
+		__get_user_asm(x, ptr, retval, "l", "=a");		\
 		break;							\
 	case 8:								\
 		__get_user_asm_u64(x, ptr, retval);			\
@@ -400,16 +398,13 @@ do {									\
 	asm volatile("\n"						\
 		     "1:	mov"itype" %[umem],%[output]\n"		\
 		     "2:\n"						\
-		     ".section .fixup,\"ax\"\n"				\
-		     "3:	mov %[efault],%[errout]\n"		\
-		     "	xorl %k[output],%k[output]\n"			\
-		     "	jmp 2b\n"					\
-		     ".previous\n"					\
-		     _ASM_EXTABLE_UA(1b, 3b)				\
+		     _ASM_EXTABLE_TYPE_REG(1b, 2b, EX_TYPE_EFAULT_REG | \
+					   EX_FLAG_CLR_AX,		\
+					   %[errout])			\
 		     : [errout] "=r" (err),				\
 		       [output] ltype(x)				\
 		     : [umem] "m" (__m(addr)),				\
-		       [efault] "i" (-EFAULT), "0" (err))
+		       "0" (err))
 
 #endif // CONFIG_CC_HAS_ASM_GOTO_OUTPUT
 

Re: [RFC][PATCH 10/22] x86,uaccess: Remove .fixup usage

[Josh Poimboeuf]

On Thu, Nov 04, 2021 at 05:47:39PM +0100, Peter Zijlstra wrote:
>  #define __get_user_asm_u64(x, ptr, retval) \
> -	 __get_user_asm(x, ptr, retval, "q", "=r")
> +	 __get_user_asm(x, ptr, retval, "q", "=a")
>  #endif
>  
>  #define __get_user_size(x, ptr, size, retval)				\
> @@ -379,14 +377,14 @@ do {									\
>  	__chk_user_ptr(ptr);						\
>  	switch (size) {							\
>  	case 1:								\
> -		__get_user_asm(x_u8__, ptr, retval, "b", "=q");		\
> +		__get_user_asm(x_u8__, ptr, retval, "b", "=a");		\
>  		(x) = x_u8__;						\
>  		break;							\
>  	case 2:								\
> -		__get_user_asm(x, ptr, retval, "w", "=r");		\
> +		__get_user_asm(x, ptr, retval, "w", "=a");		\
>  		break;							\
>  	case 4:								\
> -		__get_user_asm(x, ptr, retval, "l", "=r");		\
> +		__get_user_asm(x, ptr, retval, "l", "=a");		\
>  		break;							\
>  	case 8:								\
>  		__get_user_asm_u64(x, ptr, retval);			\
> @@ -400,16 +398,13 @@ do {									\
>  	asm volatile("\n"						\
>  		     "1:	mov"itype" %[umem],%[output]\n"		\
>  		     "2:\n"						\
> -		     ".section .fixup,\"ax\"\n"				\
> -		     "3:	mov %[efault],%[errout]\n"		\
> -		     "	xorl %k[output],%k[output]\n"			\
> -		     "	jmp 2b\n"					\
> -		     ".previous\n"					\
> -		     _ASM_EXTABLE_UA(1b, 3b)				\
> +		     _ASM_EXTABLE_TYPE_REG(1b, 2b, EX_TYPE_EFAULT_REG | \
> +					   EX_FLAG_CLR_AX,		\
> +					   %[errout])			\
>  		     : [errout] "=r" (err),				\
>  		       [output] ltype(x)				\
>  		     : [umem] "m" (__m(addr)),				\
> -		       [efault] "i" (-EFAULT), "0" (err))
> +		       "0" (err))

Since this legacy __get_user_asm() now has a hard dependency on "=a",
might as well just make that explicit by hard-coding the constraint here
and removing the 'ltype' arg.

-- 
Josh

[RFC][PATCH 11/22] x86,xen: Remove .fixup usage

[Peter Zijlstra]

Employ the fancy new EX_TYPE_IMM_REG to create EX_TYPE_NEG_REG to
store '-1' into the designated register and use this to remove some
Xen .fixup usage.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/x86/include/asm/extable_fixup_types.h |    1 +
 arch/x86/include/asm/xen/page.h            |   12 ++----------
 2 files changed, 3 insertions(+), 10 deletions(-)

--- a/arch/x86/include/asm/extable_fixup_types.h
+++ b/arch/x86/include/asm/extable_fixup_types.h
@@ -35,5 +35,6 @@
 
 #define	EX_TYPE_IMM_REG			17 /* reg := (long)imm */
 #define	EX_TYPE_EFAULT_REG		(EX_TYPE_IMM_REG | EX_TYPE_IMM(-EFAULT))
+#define	EX_TYPE_NEG_REG			(EX_TYPE_IMM_REG | EX_TYPE_IMM(-1))
 
 #endif
--- a/arch/x86/include/asm/xen/page.h
+++ b/arch/x86/include/asm/xen/page.h
@@ -96,11 +96,7 @@ static inline int xen_safe_write_ulong(u
 
 	asm volatile("1: mov %[val], %[ptr]\n"
 		     "2:\n"
-		     ".section .fixup, \"ax\"\n"
-		     "3: sub $1, %[ret]\n"
-		     "   jmp 2b\n"
-		     ".previous\n"
-		     _ASM_EXTABLE(1b, 3b)
+		     _ASM_EXTABLE_TYPE_REG(1b, 2b, EX_TYPE_NEG_REG, %[ret])
 		     : [ret] "+r" (ret), [ptr] "=m" (*addr)
 		     : [val] "r" (val));
 
@@ -115,11 +111,7 @@ static inline int xen_safe_read_ulong(co
 
 	asm volatile("1: mov %[ptr], %[rval]\n"
 		     "2:\n"
-		     ".section .fixup, \"ax\"\n"
-		     "3: sub $1, %[ret]\n"
-		     "   jmp 2b\n"
-		     ".previous\n"
-		     _ASM_EXTABLE(1b, 3b)
+		     _ASM_EXTABLE_TYPE_REG(1b, 2b, EX_TYPE_NEG_REG, %[ret])
 		     : [ret] "+r" (ret), [rval] "+r" (rval)
 		     : [ptr] "m" (*addr));
 	*val = rval;

Re: [RFC][PATCH 11/22] x86,xen: Remove .fixup usage

[Josh Poimboeuf]

On Thu, Nov 04, 2021 at 05:47:40PM +0100, Peter Zijlstra wrote:
> Employ the fancy new EX_TYPE_IMM_REG to create EX_TYPE_NEG_REG to
> store '-1' into the designated register and use this to remove some
> Xen .fixup usage.
> 
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  arch/x86/include/asm/extable_fixup_types.h |    1 +
>  arch/x86/include/asm/xen/page.h            |   12 ++----------
>  2 files changed, 3 insertions(+), 10 deletions(-)
> 
> --- a/arch/x86/include/asm/extable_fixup_types.h
> +++ b/arch/x86/include/asm/extable_fixup_types.h
> @@ -35,5 +35,6 @@
>  
>  #define	EX_TYPE_IMM_REG			17 /* reg := (long)imm */
>  #define	EX_TYPE_EFAULT_REG		(EX_TYPE_IMM_REG | EX_TYPE_IMM(-EFAULT))
> +#define	EX_TYPE_NEG_REG			(EX_TYPE_IMM_REG | EX_TYPE_IMM(-1))
>  
>  #endif
> --- a/arch/x86/include/asm/xen/page.h
> +++ b/arch/x86/include/asm/xen/page.h
> @@ -96,11 +96,7 @@ static inline int xen_safe_write_ulong(u
>  
>  	asm volatile("1: mov %[val], %[ptr]\n"
>  		     "2:\n"
> -		     ".section .fixup, \"ax\"\n"
> -		     "3: sub $1, %[ret]\n"
> -		     "   jmp 2b\n"
> -		     ".previous\n"
> -		     _ASM_EXTABLE(1b, 3b)
> +		     _ASM_EXTABLE_TYPE_REG(1b, 2b, EX_TYPE_NEG_REG, %[ret])
>  		     : [ret] "+r" (ret), [ptr] "=m" (*addr)
>  		     : [val] "r" (val));
>  
> @@ -115,11 +111,7 @@ static inline int xen_safe_read_ulong(co
>  
>  	asm volatile("1: mov %[ptr], %[rval]\n"
>  		     "2:\n"
> -		     ".section .fixup, \"ax\"\n"
> -		     "3: sub $1, %[ret]\n"
> -		     "   jmp 2b\n"
> -		     ".previous\n"
> -		     _ASM_EXTABLE(1b, 3b)
> +		     _ASM_EXTABLE_TYPE_REG(1b, 2b, EX_TYPE_NEG_REG, %[ret])
>  		     : [ret] "+r" (ret), [rval] "+r" (rval)
>  		     : [ptr] "m" (*addr));
>  	*val = rval;

-EFAULT is also negative, and presumably more appropriate than -1.

Could we just use EX_TYPE_EFAULT_REG?

-- 
Josh

Re: [RFC][PATCH 11/22] x86,xen: Remove .fixup usage

[Peter Zijlstra]

On Thu, Nov 04, 2021 at 03:31:51PM -0700, Josh Poimboeuf wrote:
> On Thu, Nov 04, 2021 at 05:47:40PM +0100, Peter Zijlstra wrote:
> > Employ the fancy new EX_TYPE_IMM_REG to create EX_TYPE_NEG_REG to
> > store '-1' into the designated register and use this to remove some
> > Xen .fixup usage.
> > 
> > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> > ---
> >  arch/x86/include/asm/extable_fixup_types.h |    1 +
> >  arch/x86/include/asm/xen/page.h            |   12 ++----------
> >  2 files changed, 3 insertions(+), 10 deletions(-)
> > 
> > --- a/arch/x86/include/asm/extable_fixup_types.h
> > +++ b/arch/x86/include/asm/extable_fixup_types.h
> > @@ -35,5 +35,6 @@
> >  
> >  #define	EX_TYPE_IMM_REG			17 /* reg := (long)imm */
> >  #define	EX_TYPE_EFAULT_REG		(EX_TYPE_IMM_REG | EX_TYPE_IMM(-EFAULT))
> > +#define	EX_TYPE_NEG_REG			(EX_TYPE_IMM_REG | EX_TYPE_IMM(-1))
> >  
> >  #endif
> > --- a/arch/x86/include/asm/xen/page.h
> > +++ b/arch/x86/include/asm/xen/page.h
> > @@ -96,11 +96,7 @@ static inline int xen_safe_write_ulong(u
> >  
> >  	asm volatile("1: mov %[val], %[ptr]\n"
> >  		     "2:\n"
> > -		     ".section .fixup, \"ax\"\n"
> > -		     "3: sub $1, %[ret]\n"
> > -		     "   jmp 2b\n"
> > -		     ".previous\n"
> > -		     _ASM_EXTABLE(1b, 3b)
> > +		     _ASM_EXTABLE_TYPE_REG(1b, 2b, EX_TYPE_NEG_REG, %[ret])
> >  		     : [ret] "+r" (ret), [ptr] "=m" (*addr)
> >  		     : [val] "r" (val));
> >  
> > @@ -115,11 +111,7 @@ static inline int xen_safe_read_ulong(co
> >  
> >  	asm volatile("1: mov %[ptr], %[rval]\n"
> >  		     "2:\n"
> > -		     ".section .fixup, \"ax\"\n"
> > -		     "3: sub $1, %[ret]\n"
> > -		     "   jmp 2b\n"
> > -		     ".previous\n"
> > -		     _ASM_EXTABLE(1b, 3b)
> > +		     _ASM_EXTABLE_TYPE_REG(1b, 2b, EX_TYPE_NEG_REG, %[ret])
> >  		     : [ret] "+r" (ret), [rval] "+r" (rval)
> >  		     : [ptr] "m" (*addr));
> >  	*val = rval;
> 
> -EFAULT is also negative, and presumably more appropriate than -1.

Possibly, means I'll have to go audit all it's users. I'll put it on the
todo list.

[RFC][PATCH 12/22] x86,fpu: Remove .fixup usage

[Peter Zijlstra]

Employ EX_TYPE_NEG_REG to store '-1' into the %[err] register on
exception.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/x86/kernel/fpu/legacy.h |    6 +-----
 arch/x86/kernel/fpu/xstate.h |    6 +-----
 2 files changed, 2 insertions(+), 10 deletions(-)

--- a/arch/x86/kernel/fpu/legacy.h
+++ b/arch/x86/kernel/fpu/legacy.h
@@ -35,11 +35,7 @@ static inline void ldmxcsr(u32 mxcsr)
 	int err;							\
 	asm volatile("1:" #insn "\n\t"					\
 		     "2:\n"						\
-		     ".section .fixup,\"ax\"\n"				\
-		     "3:  movl $-1,%[err]\n"				\
-		     "    jmp  2b\n"					\
-		     ".previous\n"					\
-		     _ASM_EXTABLE(1b, 3b)				\
+		     _ASM_EXTABLE_TYPE_REG(1b, 2b, EX_TYPE_NEG_REG, %[err]) \
 		     : [err] "=r" (err), output				\
 		     : "0"(0), input);					\
 	err;								\
--- a/arch/x86/kernel/fpu/xstate.h
+++ b/arch/x86/kernel/fpu/xstate.h
@@ -107,11 +107,7 @@ static inline u64 xfeatures_mask_indepen
 		     "\n"						\
 		     "xor %[err], %[err]\n"				\
 		     "3:\n"						\
-		     ".pushsection .fixup,\"ax\"\n"			\
-		     "4: movl $-2, %[err]\n"				\
-		     "jmp 3b\n"						\
-		     ".popsection\n"					\
-		     _ASM_EXTABLE(661b, 4b)				\
+		     _ASM_EXTABLE_TYPE_REG(661b, 3b, EX_TYPE_NEG_REG, %[err]) \
 		     : [err] "=r" (err)					\
 		     : "D" (st), "m" (*st), "a" (lmask), "d" (hmask)	\
 		     : "memory")

[RFC][PATCH 13/22] x86,segment: Remove .fixup usage

[Peter Zijlstra]

Create and use EX_TYPE_ZERO_REG to clear the register and retry the
segment load on exception.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/x86/include/asm/extable_fixup_types.h |    1 +
 arch/x86/include/asm/segment.h             |    9 +--------
 2 files changed, 2 insertions(+), 8 deletions(-)

--- a/arch/x86/include/asm/extable_fixup_types.h
+++ b/arch/x86/include/asm/extable_fixup_types.h
@@ -36,5 +36,6 @@
 #define	EX_TYPE_IMM_REG			17 /* reg := (long)imm */
 #define	EX_TYPE_EFAULT_REG		(EX_TYPE_IMM_REG | EX_TYPE_IMM(-EFAULT))
 #define	EX_TYPE_NEG_REG			(EX_TYPE_IMM_REG | EX_TYPE_IMM(-1))
+#define	EX_TYPE_ZERO_REG		(EX_TYPE_IMM_REG | EX_TYPE_IMM(0))
 
 #endif
--- a/arch/x86/include/asm/segment.h
+++ b/arch/x86/include/asm/segment.h
@@ -307,14 +307,7 @@ do {									\
 									\
 	asm volatile("						\n"	\
 		     "1:	movl %k0,%%" #seg "		\n"	\
-									\
-		     ".section .fixup,\"ax\"			\n"	\
-		     "2:	xorl %k0,%k0			\n"	\
-		     "		jmp 1b				\n"	\
-		     ".previous					\n"	\
-									\
-		     _ASM_EXTABLE(1b, 2b)				\
-									\
+		     _ASM_EXTABLE_TYPE_REG(1b, 1b, EX_TYPE_ZERO_REG, %k0)\
 		     : "+r" (__val) : : "memory");			\
 } while (0)
 

[RFC][PATCH 14/22] x86,ftrace: Remove .fixup usage

[Peter Zijlstra]

Create and use EX_TYPE_ONE_REG to load 1 into the %[faulted] register
on exception.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/x86/include/asm/extable_fixup_types.h |    1 +
 arch/x86/kernel/ftrace.c                   |    9 ++-------
 2 files changed, 3 insertions(+), 7 deletions(-)

--- a/arch/x86/include/asm/extable_fixup_types.h
+++ b/arch/x86/include/asm/extable_fixup_types.h
@@ -37,5 +37,6 @@
 #define	EX_TYPE_EFAULT_REG		(EX_TYPE_IMM_REG | EX_TYPE_IMM(-EFAULT))
 #define	EX_TYPE_NEG_REG			(EX_TYPE_IMM_REG | EX_TYPE_IMM(-1))
 #define	EX_TYPE_ZERO_REG		(EX_TYPE_IMM_REG | EX_TYPE_IMM(0))
+#define	EX_TYPE_ONE_REG			(EX_TYPE_IMM_REG | EX_TYPE_IMM(1))
 
 #endif
--- a/arch/x86/kernel/ftrace.c
+++ b/arch/x86/kernel/ftrace.c
@@ -661,13 +661,8 @@ void prepare_ftrace_return(unsigned long
 		"   movl $0, %[faulted]\n"
 		"3:\n"
 
-		".section .fixup, \"ax\"\n"
-		"4: movl $1, %[faulted]\n"
-		"   jmp 3b\n"
-		".previous\n"
-
-		_ASM_EXTABLE(1b, 4b)
-		_ASM_EXTABLE(2b, 4b)
+		_ASM_EXTABLE_TYPE_REG(1b, 3b, EX_TYPE_ONE_REG, %[faulted])
+		_ASM_EXTABLE_TYPE_REG(2b, 3b, EX_TYPE_ONE_REG, %[faulted])
 
 		: [old] "=&r" (old), [faulted] "=r" (faulted)
 		: [parent] "r" (parent), [return_hooker] "r" (return_hooker)

Re: [RFC][PATCH 14/22] x86,ftrace: Remove .fixup usage

[Josh Poimboeuf]

On Thu, Nov 04, 2021 at 05:47:43PM +0100, Peter Zijlstra wrote:
> Create and use EX_TYPE_ONE_REG to load 1 into the %[faulted] register
> on exception.
> 
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  arch/x86/include/asm/extable_fixup_types.h |    1 +
>  arch/x86/kernel/ftrace.c                   |    9 ++-------
>  2 files changed, 3 insertions(+), 7 deletions(-)
> 
> --- a/arch/x86/include/asm/extable_fixup_types.h
> +++ b/arch/x86/include/asm/extable_fixup_types.h
> @@ -37,5 +37,6 @@
>  #define	EX_TYPE_EFAULT_REG		(EX_TYPE_IMM_REG | EX_TYPE_IMM(-EFAULT))
>  #define	EX_TYPE_NEG_REG			(EX_TYPE_IMM_REG | EX_TYPE_IMM(-1))
>  #define	EX_TYPE_ZERO_REG		(EX_TYPE_IMM_REG | EX_TYPE_IMM(0))
> +#define	EX_TYPE_ONE_REG			(EX_TYPE_IMM_REG | EX_TYPE_IMM(1))
>  
>  #endif
> --- a/arch/x86/kernel/ftrace.c
> +++ b/arch/x86/kernel/ftrace.c
> @@ -661,13 +661,8 @@ void prepare_ftrace_return(unsigned long
>  		"   movl $0, %[faulted]\n"
>  		"3:\n"
>  
> -		".section .fixup, \"ax\"\n"
> -		"4: movl $1, %[faulted]\n"
> -		"   jmp 3b\n"
> -		".previous\n"
> -
> -		_ASM_EXTABLE(1b, 4b)
> -		_ASM_EXTABLE(2b, 4b)
> +		_ASM_EXTABLE_TYPE_REG(1b, 3b, EX_TYPE_ONE_REG, %[faulted])
> +		_ASM_EXTABLE_TYPE_REG(2b, 3b, EX_TYPE_ONE_REG, %[faulted])
>  
>  		: [old] "=&r" (old), [faulted] "=r" (faulted)
>  		: [parent] "r" (parent), [return_hooker] "r" (return_hooker)

This fixup code seems to no longer exist in the latest.

-- 
Josh

Re: [RFC][PATCH 14/22] x86,ftrace: Remove .fixup usage

[Peter Zijlstra]

On Thu, Nov 04, 2021 at 03:35:50PM -0700, Josh Poimboeuf wrote:
> On Thu, Nov 04, 2021 at 05:47:43PM +0100, Peter Zijlstra wrote:
> > Create and use EX_TYPE_ONE_REG to load 1 into the %[faulted] register
> > on exception.
> > 
> > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> > ---
> >  arch/x86/include/asm/extable_fixup_types.h |    1 +
> >  arch/x86/kernel/ftrace.c                   |    9 ++-------
> >  2 files changed, 3 insertions(+), 7 deletions(-)
> > 
> > --- a/arch/x86/include/asm/extable_fixup_types.h
> > +++ b/arch/x86/include/asm/extable_fixup_types.h
> > @@ -37,5 +37,6 @@
> >  #define	EX_TYPE_EFAULT_REG		(EX_TYPE_IMM_REG | EX_TYPE_IMM(-EFAULT))
> >  #define	EX_TYPE_NEG_REG			(EX_TYPE_IMM_REG | EX_TYPE_IMM(-1))
> >  #define	EX_TYPE_ZERO_REG		(EX_TYPE_IMM_REG | EX_TYPE_IMM(0))
> > +#define	EX_TYPE_ONE_REG			(EX_TYPE_IMM_REG | EX_TYPE_IMM(1))
> >  
> >  #endif
> > --- a/arch/x86/kernel/ftrace.c
> > +++ b/arch/x86/kernel/ftrace.c
> > @@ -661,13 +661,8 @@ void prepare_ftrace_return(unsigned long
> >  		"   movl $0, %[faulted]\n"
> >  		"3:\n"
> >  
> > -		".section .fixup, \"ax\"\n"
> > -		"4: movl $1, %[faulted]\n"
> > -		"   jmp 3b\n"
> > -		".previous\n"
> > -
> > -		_ASM_EXTABLE(1b, 4b)
> > -		_ASM_EXTABLE(2b, 4b)
> > +		_ASM_EXTABLE_TYPE_REG(1b, 3b, EX_TYPE_ONE_REG, %[faulted])
> > +		_ASM_EXTABLE_TYPE_REG(2b, 3b, EX_TYPE_ONE_REG, %[faulted])
> >  
> >  		: [old] "=&r" (old), [faulted] "=r" (faulted)
> >  		: [parent] "r" (parent), [return_hooker] "r" (return_hooker)
> 
> This fixup code seems to no longer exist in the latest.

\o/, less patches. That's the risk of doing this stuff during the merge
window I suppose :-)

[RFC][PATCH 15/22] x86,vmx: Remove .fixup usage

[Peter Zijlstra]

In the vmread exceptin path, use the, thus far, unused output register
to push the @fault argument onto the stack. This, in turn, enables the
exception handler to not do pushes and only modify that register when
an exception does occur.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/x86/kvm/vmx/vmx_ops.h |   14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

--- a/arch/x86/kvm/vmx/vmx_ops.h
+++ b/arch/x86/kvm/vmx/vmx_ops.h
@@ -80,9 +80,11 @@ static __always_inline unsigned long __v
 		      * @field, and bounce through the trampoline to preserve
 		      * volatile registers.
 		      */
-		     "push $0\n\t"
+		     "xorl %k1, %k1\n\t"
+		     "2:\n\t"
+		     "push %1\n\t"
 		     "push %2\n\t"
-		     "2:call vmread_error_trampoline\n\t"
+		     "call vmread_error_trampoline\n\t"
 
 		     /*
 		      * Unwind the stack.  Note, the trampoline zeros out the
@@ -93,12 +95,8 @@ static __always_inline unsigned long __v
 		     "3:\n\t"
 
 		     /* VMREAD faulted.  As above, except push '1' for @fault. */
-		     ".pushsection .fixup, \"ax\"\n\t"
-		     "4: push $1\n\t"
-		     "push %2\n\t"
-		     "jmp 2b\n\t"
-		     ".popsection\n\t"
-		     _ASM_EXTABLE(1b, 4b)
+		     _ASM_EXTABLE_TYPE_REG(1b, 2b, EX_TYPE_ONE_REG, %1)
+
 		     : ASM_CALL_CONSTRAINT, "=r"(value) : "r"(field) : "cc");
 	return value;
 }

Re: [RFC][PATCH 15/22] x86,vmx: Remove .fixup usage

[Paolo Bonzini]

On 11/4/21 17:47, Peter Zijlstra wrote:
> In the vmread exceptin path, use the, thus far, unused output register
> to push the @fault argument onto the stack. This, in turn, enables the
> exception handler to not do pushes and only modify that register when
> an exception does occur.
> 
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>   arch/x86/kvm/vmx/vmx_ops.h |   14 ++++++--------
>   1 file changed, 6 insertions(+), 8 deletions(-)
> 
> --- a/arch/x86/kvm/vmx/vmx_ops.h
> +++ b/arch/x86/kvm/vmx/vmx_ops.h
> @@ -80,9 +80,11 @@ static __always_inline unsigned long __v
>   		      * @field, and bounce through the trampoline to preserve
>   		      * volatile registers.
>   		      */
> -		     "push $0\n\t"
> +		     "xorl %k1, %k1\n\t"
> +		     "2:\n\t"
> +		     "push %1\n\t"
>   		     "push %2\n\t"
> -		     "2:call vmread_error_trampoline\n\t"
> +		     "call vmread_error_trampoline\n\t"
>   
>   		     /*
>   		      * Unwind the stack.  Note, the trampoline zeros out the
> @@ -93,12 +95,8 @@ static __always_inline unsigned long __v
>   		     "3:\n\t"
>   
>   		     /* VMREAD faulted.  As above, except push '1' for @fault. */
> -		     ".pushsection .fixup, \"ax\"\n\t"
> -		     "4: push $1\n\t"
> -		     "push %2\n\t"
> -		     "jmp 2b\n\t"
> -		     ".popsection\n\t"
> -		     _ASM_EXTABLE(1b, 4b)
> +		     _ASM_EXTABLE_TYPE_REG(1b, 2b, EX_TYPE_ONE_REG, %1)
> +
>   		     : ASM_CALL_CONSTRAINT, "=r"(value) : "r"(field) : "cc");
>   	return value;
>   }
> 
> 

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Re: [RFC][PATCH 15/22] x86,vmx: Remove .fixup usage

[Sean Christopherson]

On Thu, Nov 04, 2021, Peter Zijlstra wrote:
> In the vmread exceptin path, use the, thus far, unused output register
> to push the @fault argument onto the stack. This, in turn, enables the
> exception handler to not do pushes and only modify that register when
> an exception does occur.
> 
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  arch/x86/kvm/vmx/vmx_ops.h |   14 ++++++--------
>  1 file changed, 6 insertions(+), 8 deletions(-)
> 
> --- a/arch/x86/kvm/vmx/vmx_ops.h
> +++ b/arch/x86/kvm/vmx/vmx_ops.h
> @@ -80,9 +80,11 @@ static __always_inline unsigned long __v
>  		      * @field, and bounce through the trampoline to preserve
>  		      * volatile registers.
>  		      */
> -		     "push $0\n\t"
> +		     "xorl %k1, %k1\n\t"
> +		     "2:\n\t"
> +		     "push %1\n\t"
>  		     "push %2\n\t"

This trick doesn't work if the compiler selects the same GPR for %1 and %2, as
the "field" will get lost.

   0x00000000000005a2 <+66>:	0f 78 c0	vmread %rax,%rax
   0x00000000000005a5 <+69>:	3e 77 0b	ja,pt  0x5b3 <vmx_read_guest_seg_selector+83>
   0x00000000000005a8 <+72>:	31 c0	xor    %eax,%eax
   0x00000000000005aa <+74>:	50	push   %rax
   0x00000000000005ab <+75>:	50	push   %rax
   0x00000000000005ac <+76>:	e8 00 00 00 00	callq  0x5b1 <vmx_read_guest_seg_selector+81>
   0x00000000000005b1 <+81>:	58	pop    %rax
   0x00000000000005b2 <+82>:	58	pop    %rax

I'm struggling to think of a not-awful way to handle this without custom fixup.
Once "asm goto with output" is ubiquitous, we can get rid of this mess entirely,
but until then...

The best idea I can come up with is to abuse the current spec and use bits N:16
to indicate a fault, i.e. use EX_TYPE_EFAULT_REG.  VMCS fields are technically
32-bit values, but bits 31:15 are reserved.  That would yield a false positive
if KVM really screwed up and/or state was corrupted and the VMREAD actually tried
to read the non-existed field -EFAULT, but that's very, very unlikely (-1 would
be slightly more likey, hence EX_TYPE_EFAULT_REG instead of EX_TYPE_NEG_REG).
The fault patch would also lose the field, but vmread_error() doesn't consume
that info, I can't think of a reason that KVM would want to, and if the debugger
cares, most of the time the field is hardcoded at the call site.

Another horrific idea would be to encode the fault information in EFLAGS.  On
VM-Fail, the arithmetic flags are all set to specific values.  That would require
yet another flavor of _ASM_EXTABLE_ though, and would make the asm subroutine
even more ugly than it already is.  Probably not worth it.

Forcing different registers would be another option, but I would strongly prefer
to give the compiler free reign to optimize the happy path.

Tested all paths of this on 64-bit and 32-bit KVM.

From a4d602649ac7ea2e5136b642a57291fc5658d859 Mon Sep 17 00:00:00 2001
From: Sean Christopherson <seanjc@google.com>
Date: Fri, 5 Nov 2021 10:40:21 -0700
Subject: [PATCH] KVM: VMX: Remove .fixup usage in VMREAD fault path

Rework the VMREAD magic to eliminate usage of custom .fixup as a step
toward eliminating .fixup throughout the kernel.  Because the compiler
can use the same register for both the input and output, e.g. generate
VMREAD %rax, %rax, hijacking the output register to store the "fault"
flag is not an option.  To avoid yet another flavor of fixup, overload
"field" to treat -EFAULT as a magic value.

Because -EFAULT is (obviously) a negative value, collision with a VMCS
field encoding is impossible on 64-bit kernels due to the encodings being
32-bit values.  And for 32-bit fields, no known VMCS field can collide
with the magic value because despite field encodings being 32-bit values,
bits 31:15 are currently reserved and must be zero.  These shenanigans
can be revisited in the extremely unlikely event that a future CPU
supports a "negative" VMCS encoding.  And hopefully this entire mess will
go away before that happens, as the trampoline shenanigans are needed
only because the minimum compiler version doesn't guarantee support for
asm goto with outputs.

Alternatively, the fault information could be encoded in EFLAGS since
VM-Fail sets arithmetic flags to well-known values.  However, that would
require yet another flavor of fixup and would complicate the already ugly
asm trampoline.

Forcing different output registers is undesirable as doing so would
prevent the compiler from fully optimizing the happy path, which is what
KVM really cares about since faults and failures on VMREAD occur if and
only if there is a KVM bug or hardware error, i.e. are very rare events.

Because a false positive is theoretically possible, e.g. if KVM executed
VMREAD with -EFAULT as the target field, WARN and log a suspected fault
but don't call kvm_spurious_fault().  If VMREAD really did fault, either
KVM is all bug guaranteed to hit a fault on another VMX instruction due
to VMX being disabled/broken, or the fault was specific to _that_ VMREAD,
e.g. due to a bad memory operand, in which case killing KVM isn't
necessary since the faulting VMREAD will "return" '0'.

Cc: Peter Zijlstra (Intel) <peterz@infradead.org>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/vmx/vmenter.S | 13 +++++--------
 arch/x86/kvm/vmx/vmx.c     | 13 ++++++++++---
 arch/x86/kvm/vmx/vmx_ops.h | 30 ++++++++++++------------------
 3 files changed, 27 insertions(+), 29 deletions(-)

diff --git a/arch/x86/kvm/vmx/vmenter.S b/arch/x86/kvm/vmx/vmenter.S
index 3a6461694fc2..f0f5487de077 100644
--- a/arch/x86/kvm/vmx/vmenter.S
+++ b/arch/x86/kvm/vmx/vmenter.S
@@ -240,8 +240,7 @@ SYM_FUNC_END(__vmx_vcpu_run)

 /**
  * vmread_error_trampoline - Trampoline from inline asm to vmread_error()
- * @field:	VMCS field encoding that failed
- * @fault:	%true if the VMREAD faulted, %false if it failed
+ * @field:	VMCS field encoding that failed, -EFAULT on fault

  * Save and restore volatile registers across a call to vmread_error().  Note,
  * all parameters are passed on the stack.
@@ -262,23 +261,21 @@ SYM_FUNC_START(vmread_error_trampoline)
 	push %r11
 #endif
 #ifdef CONFIG_X86_64
-	/* Load @field and @fault to arg1 and arg2 respectively. */
-	mov 3*WORD_SIZE(%rbp), %_ASM_ARG2
+	/* Load @field arg1. */
 	mov 2*WORD_SIZE(%rbp), %_ASM_ARG1
 #else
 	/* Parameters are passed on the stack for 32-bit (see asmlinkage). */
-	push 3*WORD_SIZE(%ebp)
 	push 2*WORD_SIZE(%ebp)
 #endif

 	call vmread_error

 #ifndef CONFIG_X86_64
-	add $8, %esp
+	add $4, %esp
 #endif

-	/* Zero out @fault, which will be popped into the result register. */
-	_ASM_MOV $0, 3*WORD_SIZE(%_ASM_BP)
+	/* Zero out @field, which will be popped into the result register. */
+	_ASM_MOV $0, 2*WORD_SIZE(%_ASM_BP)

 #ifdef CONFIG_X86_64
 	pop %r11
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 9a979279a37b..ddce443baa9c 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -373,10 +373,17 @@ do {					\
 	pr_warn_ratelimited(fmt);	\
 } while (0)

-asmlinkage void vmread_error(unsigned long field, bool fault)
+asmlinkage void vmread_error(unsigned long field)
 {
-	if (fault)
-		kvm_spurious_fault();
+	/*
+	 * @field is stuff with -EFAULT if VMREAD faulted.  There is a teeny
+	 * tiny chance of a false positive, i.e. KVM could attempt to VMREAD
+	 * -EFAULT if things go sidways, so just log the fault instead of
+	 * doing kvm_spurious_fault().  If VMREAD did fault, KVM is all but
+	 * guaranteed to die on a VMWRITE, VMRESUME, etc... fault soon enough.
+	 */
+	if (field == (unsigned long)-EFAULT)
+		vmx_insn_failed("kvm: vmread faulted\n");
 	else
 		vmx_insn_failed("kvm: vmread failed: field=%lx\n", field);
 }
diff --git a/arch/x86/kvm/vmx/vmx_ops.h b/arch/x86/kvm/vmx/vmx_ops.h
index 9e9ef47e988c..5e5113d2b324 100644
--- a/arch/x86/kvm/vmx/vmx_ops.h
+++ b/arch/x86/kvm/vmx/vmx_ops.h
@@ -10,9 +10,8 @@
 #include "vmcs.h"
 #include "x86.h"

-asmlinkage void vmread_error(unsigned long field, bool fault);
-__attribute__((regparm(0))) void vmread_error_trampoline(unsigned long field,
-							 bool fault);
+asmlinkage void vmread_error(unsigned long field);
+__attribute__((regparm(0))) void vmread_error_trampoline(unsigned long field);
 void vmwrite_error(unsigned long field, unsigned long value);
 void vmclear_error(struct vmcs *vmcs, u64 phys_addr);
 void vmptrld_error(struct vmcs *vmcs, u64 phys_addr);
@@ -76,29 +75,24 @@ static __always_inline unsigned long __vmcs_readl(unsigned long field)
 		     "ja 3f\n\t"

 		     /*
-		      * VMREAD failed.  Push '0' for @fault, push the failing
-		      * @field, and bounce through the trampoline to preserve
-		      * volatile registers.
+		      * VMREAD failed, push the failing @field, and bounce
+		      * through the trampoline to preserve volatile registers.
+		      * If VMREAD faults, this will push -FAULT (see below).
 		      */
-		     "push $0\n\t"
-		     "push %2\n\t"
-		     "2:call vmread_error_trampoline\n\t"
+		     "2: push %2\n\t"
+		     "call vmread_error_trampoline\n\t"

 		     /*
 		      * Unwind the stack.  Note, the trampoline zeros out the
-		      * memory for @fault so that the result is '0' on error.
+		      * memory for @field so that the result is '0' on error,
+		      * hence the pop to %1, not %2.
 		      */
-		     "pop %2\n\t"
 		     "pop %1\n\t"
 		     "3:\n\t"

-		     /* VMREAD faulted.  As above, except push '1' for @fault. */
-		     ".pushsection .fixup, \"ax\"\n\t"
-		     "4: push $1\n\t"
-		     "push %2\n\t"
-		     "jmp 2b\n\t"
-		     ".popsection\n\t"
-		     _ASM_EXTABLE(1b, 4b)
+		     /* VMREAD faulted.  As above, except push '-EFAULT' for @fault. */
+		     _ASM_EXTABLE_TYPE_REG(1b, 2b, EX_TYPE_EFAULT_REG, %1)
+
 		     : ASM_CALL_CONSTRAINT, "=r"(value) : "r"(field) : "cc");
 	return value;
 }
--
2.34.0.rc0.344.g81b53c2807-goog

Re: [RFC][PATCH 15/22] x86,vmx: Remove .fixup usage

[Peter Zijlstra]

On Fri, Nov 05, 2021 at 06:17:53PM +0000, Sean Christopherson wrote:
> On Thu, Nov 04, 2021, Peter Zijlstra wrote:
> > In the vmread exceptin path, use the, thus far, unused output register
> > to push the @fault argument onto the stack. This, in turn, enables the
> > exception handler to not do pushes and only modify that register when
> > an exception does occur.
> > 
> > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> > ---
> >  arch/x86/kvm/vmx/vmx_ops.h |   14 ++++++--------
> >  1 file changed, 6 insertions(+), 8 deletions(-)
> > 
> > --- a/arch/x86/kvm/vmx/vmx_ops.h
> > +++ b/arch/x86/kvm/vmx/vmx_ops.h
> > @@ -80,9 +80,11 @@ static __always_inline unsigned long __v
> >  		      * @field, and bounce through the trampoline to preserve
> >  		      * volatile registers.
> >  		      */
> > -		     "push $0\n\t"
> > +		     "xorl %k1, %k1\n\t"
> > +		     "2:\n\t"
> > +		     "push %1\n\t"
> >  		     "push %2\n\t"
> 
> This trick doesn't work if the compiler selects the same GPR for %1 and %2, as
> the "field" will get lost.
> 
>    0x00000000000005a2 <+66>:	0f 78 c0	vmread %rax,%rax
>    0x00000000000005a5 <+69>:	3e 77 0b	ja,pt  0x5b3 <vmx_read_guest_seg_selector+83>
>    0x00000000000005a8 <+72>:	31 c0	xor    %eax,%eax
>    0x00000000000005aa <+74>:	50	push   %rax
>    0x00000000000005ab <+75>:	50	push   %rax

*argh* I misread that thing, %2 is an input reg :/ Thanks for your
patch.

Re: [RFC][PATCH 15/22] x86,vmx: Remove .fixup usage

[Peter Zijlstra]

On Fri, Nov 05, 2021 at 06:17:53PM +0000, Sean Christopherson wrote:

> diff --git a/arch/x86/kvm/vmx/vmx_ops.h b/arch/x86/kvm/vmx/vmx_ops.h
> index 9e9ef47e988c..5e5113d2b324 100644
> --- a/arch/x86/kvm/vmx/vmx_ops.h
> +++ b/arch/x86/kvm/vmx/vmx_ops.h

> @@ -76,29 +75,24 @@ static __always_inline unsigned long __vmcs_readl(unsigned long field)
>  		     "ja 3f\n\t"
> 
>  		     /*
> -		      * VMREAD failed.  Push '0' for @fault, push the failing
> -		      * @field, and bounce through the trampoline to preserve
> -		      * volatile registers.
> +		      * VMREAD failed, push the failing @field, and bounce
> +		      * through the trampoline to preserve volatile registers.
> +		      * If VMREAD faults, this will push -FAULT (see below).
>  		      */
> -		     "push $0\n\t"
> -		     "push %2\n\t"
> -		     "2:call vmread_error_trampoline\n\t"
> +		     "2: push %2\n\t"
> +		     "call vmread_error_trampoline\n\t"
> 
>  		     /*
>  		      * Unwind the stack.  Note, the trampoline zeros out the
> -		      * memory for @fault so that the result is '0' on error.
> +		      * memory for @field so that the result is '0' on error,
> +		      * hence the pop to %1, not %2.
>  		      */
> -		     "pop %2\n\t"
>  		     "pop %1\n\t"
>  		     "3:\n\t"
> 
> -		     /* VMREAD faulted.  As above, except push '1' for @fault. */
> -		     ".pushsection .fixup, \"ax\"\n\t"
> -		     "4: push $1\n\t"
> -		     "push %2\n\t"
> -		     "jmp 2b\n\t"
> -		     ".popsection\n\t"
> -		     _ASM_EXTABLE(1b, 4b)
> +		     /* VMREAD faulted.  As above, except push '-EFAULT' for @fault. */
> +		     _ASM_EXTABLE_TYPE_REG(1b, 2b, EX_TYPE_EFAULT_REG, %1)
> +
>  		     : ASM_CALL_CONSTRAINT, "=r"(value) : "r"(field) : "cc");
>  	return value;
>  }

A different option is something like the below; down side is that it
increases the amount of text, while your version decreases the amount of
useless text (the gunk between vmread and 3:, all of which should
ideally live out-of-line).

For now I'll stick with your patch.

diff --git a/arch/x86/kvm/vmx/vmx_ops.h b/arch/x86/kvm/vmx/vmx_ops.h
index 9e9ef47e988c..99fc1f34fbd4 100644
--- a/arch/x86/kvm/vmx/vmx_ops.h
+++ b/arch/x86/kvm/vmx/vmx_ops.h
@@ -81,8 +81,8 @@ static __always_inline unsigned long __vmcs_readl(unsigned long field)
 		      * volatile registers.
 		      */
 		     "push $0\n\t"
-		     "push %2\n\t"
-		     "2:call vmread_error_trampoline\n\t"
+		     "2: push %2\n\t"
+		     "call vmread_error_trampoline\n\t"
 
 		     /*
 		      * Unwind the stack.  Note, the trampoline zeros out the
@@ -90,14 +90,14 @@ static __always_inline unsigned long __vmcs_readl(unsigned long field)
 		      */
 		     "pop %2\n\t"
 		     "pop %1\n\t"
-		     "3:\n\t"
+		     "jmp 3f\n\t"
 
-		     /* VMREAD faulted.  As above, except push '1' for @fault. */
-		     ".pushsection .fixup, \"ax\"\n\t"
 		     "4: push $1\n\t"
-		     "push %2\n\t"
 		     "jmp 2b\n\t"
-		     ".popsection\n\t"
+
+		     "3:\n\t"
+
+		     /* VMREAD faulted.  As above, except push '1' for @fault. */
 		     _ASM_EXTABLE(1b, 4b)
 		     : ASM_CALL_CONSTRAINT, "=r"(value) : "r"(field) : "cc");
 	return value;

Re: [RFC][PATCH 15/22] x86,vmx: Remove .fixup usage

[Sean Christopherson]

On Fri, Nov 05, 2021, Peter Zijlstra wrote:
> On Fri, Nov 05, 2021 at 06:17:53PM +0000, Sean Christopherson wrote:
> A different option is something like the below; down side is that it
> increases the amount of text, while your version decreases the amount of
> useless text (the gunk between vmread and 3:, all of which should
> ideally live out-of-line).

Yeah :-/

> For now I'll stick with your patch.

No strong preference, I'll defer to Paolo.

Re: [RFC][PATCH 15/22] x86,vmx: Remove .fixup usage

[Paolo Bonzini]

On 11/5/21 19:17, Sean Christopherson wrote:
> On Thu, Nov 04, 2021, Peter Zijlstra wrote:
>> In the vmread exceptin path, use the, thus far, unused output register
>> to push the @fault argument onto the stack. This, in turn, enables the
>> exception handler to not do pushes and only modify that register when
>> an exception does occur.
>>
>> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
>> ---
>>   arch/x86/kvm/vmx/vmx_ops.h |   14 ++++++--------
>>   1 file changed, 6 insertions(+), 8 deletions(-)
>>
>> --- a/arch/x86/kvm/vmx/vmx_ops.h
>> +++ b/arch/x86/kvm/vmx/vmx_ops.h
>> @@ -80,9 +80,11 @@ static __always_inline unsigned long __v
>>   		      * @field, and bounce through the trampoline to preserve
>>   		      * volatile registers.
>>   		      */
>> -		     "push $0\n\t"
>> +		     "xorl %k1, %k1\n\t"
>> +		     "2:\n\t"
>> +		     "push %1\n\t"
>>   		     "push %2\n\t"
> 
> This trick doesn't work if the compiler selects the same GPR for %1 and %2, as
> the "field" will get lost.

Ouch, good catch.  It should be actually very simple to fix it, just 
mark "value" as an "early clobber" output:

       : ASM_CALL_CONSTRAINT, "=&r"(value) : "r"(field) : "cc");

That's an output which is written before the instruction is finished 
using the input operands.  The manual even says "this operand may not 
lie in a register that is read by the instruction or as part of any 
memory address", which is exactly what you caught with %1 and %2 both 
being the same GPR.

Paolo

Re: [RFC][PATCH 15/22] x86,vmx: Remove .fixup usage

[Peter Zijlstra]

On Sat, Nov 06, 2021 at 08:05:51AM +0100, Paolo Bonzini wrote:
> On 11/5/21 19:17, Sean Christopherson wrote:
> > On Thu, Nov 04, 2021, Peter Zijlstra wrote:
> > > In the vmread exceptin path, use the, thus far, unused output register
> > > to push the @fault argument onto the stack. This, in turn, enables the
> > > exception handler to not do pushes and only modify that register when
> > > an exception does occur.
> > > 
> > > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> > > ---
> > >   arch/x86/kvm/vmx/vmx_ops.h |   14 ++++++--------
> > >   1 file changed, 6 insertions(+), 8 deletions(-)
> > > 
> > > --- a/arch/x86/kvm/vmx/vmx_ops.h
> > > +++ b/arch/x86/kvm/vmx/vmx_ops.h
> > > @@ -80,9 +80,11 @@ static __always_inline unsigned long __v
> > >   		      * @field, and bounce through the trampoline to preserve
> > >   		      * volatile registers.
> > >   		      */
> > > -		     "push $0\n\t"
> > > +		     "xorl %k1, %k1\n\t"
> > > +		     "2:\n\t"
> > > +		     "push %1\n\t"
> > >   		     "push %2\n\t"
> > 
> > This trick doesn't work if the compiler selects the same GPR for %1 and %2, as
> > the "field" will get lost.
> 
> Ouch, good catch.  It should be actually very simple to fix it, just mark
> "value" as an "early clobber" output:
> 
>       : ASM_CALL_CONSTRAINT, "=&r"(value) : "r"(field) : "cc");
> 
> That's an output which is written before the instruction is finished using
> the input operands.  The manual even says "this operand may not lie in a
> register that is read by the instruction or as part of any memory address",
> which is exactly what you caught with %1 and %2 both being the same GPR.

Yes, but as Sean points out, that will negatively affect code-gen on the
happy path. But perhaps that's acceptable if we add the asm-goto-output
variant?

Re: [RFC][PATCH 15/22] x86,vmx: Remove .fixup usage

[Paolo Bonzini]

On 11/6/21 09:36, Peter Zijlstra wrote:
>> Ouch, good catch.  It should be actually very simple to fix it, just mark
>> "value" as an "early clobber" output:
>>
>>        : ASM_CALL_CONSTRAINT, "=&r"(value) : "r"(field) : "cc");
>>
>> That's an output which is written before the instruction is finished using
>> the input operands.  The manual even says "this operand may not lie in a
>> register that is read by the instruction or as part of any memory address",
>> which is exactly what you caught with %1 and %2 both being the same GPR.
> Yes, but as Sean points out, that will negatively affect code-gen on the
> happy path. But perhaps that's acceptable if we add the asm-goto-output
> variant?

I think it's acceptable even without the #ifdef.  Forcing registers 
using the a/b/c/d/S/D constraints takes away some freedom from the 
compiler, but using two "r" registers is going to give as good assembly 
as anything else.  Most callers of __vmcs_readl call it either at the 
beginning or at the end of a function, where there aren't many live 
registers anyway.

Paolo

Re: [RFC][PATCH 15/22] x86,vmx: Remove .fixup usage

[Peter Zijlstra]

On Fri, Nov 05, 2021 at 06:17:53PM +0000, Sean Christopherson wrote:

> And hopefully this entire mess will
> go away before that happens, as the trampoline shenanigans are needed
> only because the minimum compiler version doesn't guarantee support for
> asm goto with outputs.

We can at least have those who use sane compilers get sane code..

Something like this, right?

--- a/arch/x86/kvm/vmx/vmx_ops.h
+++ b/arch/x86/kvm/vmx/vmx_ops.h
@@ -70,6 +70,31 @@ static __always_inline unsigned long __v
 {
 	unsigned long value;
 
+#ifdef CONFIG_CC_HAS_ASM_GOTO_OUTPUT
+
+	asm_volatile_goto("1: vmread %[field], %[output]\n\t"
+			  "jna %l[do_fail]\n\t"
+
+			  _ASM_EXTABLE(1b, %l[do_exception])
+
+			  : ASM_CALL_CONSTRAINT, [output] "=r" (value)
+			  : [field] "r" (field)
+			  : "cc"
+			  : do_fail, do_exception);
+
+	return value;
+
+do_fail: __cold;
+	WARN_ONCE(1, "kvm: vmread failed: field=%lx\n", field);
+	pr_warn_ratelimited("kvm: vmread failed: field=%lx\n", field);
+	return 0;
+
+do_exception: __cold;
+	kvm_spurious_fault();
+	return 0;
+
+#else /* !CONFIG_CC_HAS_ASM_GOTO_OUTPUT */
+
 	asm volatile("1: vmread %2, %1\n\t"
 		     ".byte 0x3e\n\t" /* branch taken hint */
 		     "ja 3f\n\t"
@@ -94,7 +119,10 @@ static __always_inline unsigned long __v
 		     _ASM_EXTABLE_TYPE_REG(1b, 2b, EX_TYPE_EFAULT_REG, %1)
 
 		     : ASM_CALL_CONSTRAINT, "=r"(value) : "r"(field) : "cc");
+
 	return value;
+
+#endif /* CONFIG_CC_HAS_ASM_GOTO_OUTPUT */
 }
 
 static __always_inline u16 vmcs_read16(unsigned long field)

[RFC][PATCH 16/22] x86,checksum_32: Remove .fixup usage

[Peter Zijlstra]

Simply add EX_FLAG_CLR_AX to do as the .fixup used to do.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/x86/lib/checksum_32.S |   19 +++----------------
 1 file changed, 3 insertions(+), 16 deletions(-)

--- a/arch/x86/lib/checksum_32.S
+++ b/arch/x86/lib/checksum_32.S
@@ -260,9 +260,9 @@ unsigned int csum_partial_copy_generic (
  * Copy from ds while checksumming, otherwise like csum_partial
  */
 
-#define EXC(y...)			\
-	9999: y;			\
-	_ASM_EXTABLE_UA(9999b, 6001f)
+#define EXC(y...)						\
+	9999: y;						\
+	_ASM_EXTABLE_TYPE(9999b, 7f, EX_TYPE_UACCESS | EX_FLAG_CLR_AX)
 
 #ifndef CONFIG_X86_USE_PPRO_CHECKSUM
 
@@ -358,15 +358,6 @@ EXC(	movb %cl, (%edi)	)
 	adcl $0, %eax
 7:
 
-# Exception handler:
-.section .fixup, "ax"							
-
-6001:
-	xorl %eax, %eax
-	jmp 7b
-
-.previous
-
 	popl %ebx
 	popl %esi
 	popl %edi
@@ -439,10 +430,6 @@ EXC(	movb %dl, (%edi)         )
 6:	addl %edx, %eax
 	adcl $0, %eax
 7:
-.section .fixup, "ax"
-6001:	xorl %eax, %eax
-	jmp  7b			
-.previous				
 
 	popl %esi
 	popl %edi

[RFC][PATCH 17/22] x86,sgx: Remove .fixup usage

[Peter Zijlstra]

Create EX_TYPE_FAULT_SGX which does as EX_TYPE_FAULT does, except adds
this extra bit that SGX really fanies having.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/x86/include/asm/extable_fixup_types.h |    2 +
 arch/x86/include/asm/sgx.h                 |   18 ++++++++++++++
 arch/x86/kernel/cpu/sgx/encls.h            |   36 ++++-------------------------
 arch/x86/mm/extable.c                      |   10 ++++++++
 4 files changed, 35 insertions(+), 31 deletions(-)

--- a/arch/x86/include/asm/extable_fixup_types.h
+++ b/arch/x86/include/asm/extable_fixup_types.h
@@ -39,4 +39,6 @@
 #define	EX_TYPE_ZERO_REG		(EX_TYPE_IMM_REG | EX_TYPE_IMM(0))
 #define	EX_TYPE_ONE_REG			(EX_TYPE_IMM_REG | EX_TYPE_IMM(1))
 
+#define	EX_TYPE_FAULT_SGX		18
+
 #endif
--- a/arch/x86/include/asm/sgx.h
+++ b/arch/x86/include/asm/sgx.h
@@ -46,6 +46,24 @@ enum sgx_encls_function {
 };
 
 /**
+ * SGX_ENCLS_FAULT_FLAG - flag signifying an ENCLS return code is a trapnr
+ *
+ * ENCLS has its own (positive value) error codes and also generates
+ * ENCLS specific #GP and #PF faults.  And the ENCLS values get munged
+ * with system error codes as everything percolates back up the stack.
+ * Unfortunately (for us), we need to precisely identify each unique
+ * error code, e.g. the action taken if EWB fails varies based on the
+ * type of fault and on the exact SGX error code, i.e. we can't simply
+ * convert all faults to -EFAULT.
+ *
+ * To make all three error types coexist, we set bit 30 to identify an
+ * ENCLS fault.  Bit 31 (technically bits N:31) is used to differentiate
+ * between positive (faults and SGX error codes) and negative (system
+ * error codes) values.
+ */
+#define SGX_ENCLS_FAULT_FLAG 0x40000000
+
+/**
  * enum sgx_return_code - The return code type for ENCLS, ENCLU and ENCLV
  * %SGX_NOT_TRACKED:		Previous ETRACK's shootdown sequence has not
  *				been completed yet.
--- a/arch/x86/kernel/cpu/sgx/encls.h
+++ b/arch/x86/kernel/cpu/sgx/encls.h
@@ -11,26 +11,8 @@
 #include <asm/traps.h>
 #include "sgx.h"
 
-/**
- * ENCLS_FAULT_FLAG - flag signifying an ENCLS return code is a trapnr
- *
- * ENCLS has its own (positive value) error codes and also generates
- * ENCLS specific #GP and #PF faults.  And the ENCLS values get munged
- * with system error codes as everything percolates back up the stack.
- * Unfortunately (for us), we need to precisely identify each unique
- * error code, e.g. the action taken if EWB fails varies based on the
- * type of fault and on the exact SGX error code, i.e. we can't simply
- * convert all faults to -EFAULT.
- *
- * To make all three error types coexist, we set bit 30 to identify an
- * ENCLS fault.  Bit 31 (technically bits N:31) is used to differentiate
- * between positive (faults and SGX error codes) and negative (system
- * error codes) values.
- */
-#define ENCLS_FAULT_FLAG 0x40000000
-
 /* Retrieve the encoded trapnr from the specified return code. */
-#define ENCLS_TRAPNR(r) ((r) & ~ENCLS_FAULT_FLAG)
+#define ENCLS_TRAPNR(r) ((r) & ~SGX_ENCLS_FAULT_FLAG)
 
 /* Issue a WARN() about an ENCLS function. */
 #define ENCLS_WARN(r, name) {						  \
@@ -50,7 +32,7 @@
  */
 static inline bool encls_faulted(int ret)
 {
-	return ret & ENCLS_FAULT_FLAG;
+	return ret & SGX_ENCLS_FAULT_FLAG;
 }
 
 /**
@@ -88,11 +70,7 @@ static inline bool encls_failed(int ret)
 	asm volatile(						\
 	"1: .byte 0x0f, 0x01, 0xcf;\n\t"			\
 	"2:\n"							\
-	".section .fixup,\"ax\"\n"				\
-	"3: orl $"__stringify(ENCLS_FAULT_FLAG)",%%eax\n"	\
-	"   jmp 2b\n"						\
-	".previous\n"						\
-	_ASM_EXTABLE_FAULT(1b, 3b)				\
+	_ASM_EXTABLE_TYPE(1b, 2b, EX_TYPE_FAULT_SGX)		\
 	: "=a"(ret)						\
 	: "a"(rax), inputs					\
 	: "memory", "cc");					\
@@ -127,7 +105,7 @@ static inline bool encls_failed(int ret)
  *
  * Return:
  *   0 on success,
- *   trapnr with ENCLS_FAULT_FLAG set on fault
+ *   trapnr with SGX_ENCLS_FAULT_FLAG set on fault
  */
 #define __encls_N(rax, rbx_out, inputs...)			\
 	({							\
@@ -136,11 +114,7 @@ static inline bool encls_failed(int ret)
 	"1: .byte 0x0f, 0x01, 0xcf;\n\t"			\
 	"   xor %%eax,%%eax;\n"					\
 	"2:\n"							\
-	".section .fixup,\"ax\"\n"				\
-	"3: orl $"__stringify(ENCLS_FAULT_FLAG)",%%eax\n"	\
-	"   jmp 2b\n"						\
-	".previous\n"						\
-	_ASM_EXTABLE_FAULT(1b, 3b)				\
+	_ASM_EXTABLE_TYPE(1b, 2b, EX_TYPE_FAULT_SGX)		\
 	: "=a"(ret), "=b"(rbx_out)				\
 	: "a"(rax), inputs					\
 	: "memory");						\
--- a/arch/x86/mm/extable.c
+++ b/arch/x86/mm/extable.c
@@ -9,6 +9,7 @@
 #include <asm/sev.h>
 #include <asm/traps.h>
 #include <asm/kdebug.h>
+#include <asm/sgx.h>
 
 static inline unsigned long *pt_regs_nr(struct pt_regs *regs, int nr)
 {
@@ -146,6 +147,13 @@ static bool ex_handler_imm_reg(const str
 	return ex_handler_default(fixup, regs);
 }
 
+static bool ex_handler_sgx(const struct exception_table_entry *fixup,
+			   struct pt_regs *regs, int trapnr)
+{
+	regs->ax = trapnr | SGX_ENCLS_FAULT_FLAG;
+	return ex_handler_default(fixup, regs);
+}
+
 #define EX_TYPE_MASK	0x000000FF
 #define EX_REG_MASK	0x00000F00
 #define EX_FLAG_MASK	0x0000F000
@@ -221,6 +229,8 @@ int fixup_exception(struct pt_regs *regs
 		return ex_handler_pop_seg(e, regs);
 	case EX_TYPE_IMM_REG:
 		return ex_handler_imm_reg(e, regs, reg, imm);
+	case EX_TYPE_FAULT_SGX:
+		return ex_handler_sgx(e, regs, trapnr);
 	}
 	BUG();
 }

[RFC][PATCH 18/22] x86,kvm: Remove .fixup usage

[Peter Zijlstra]

KVM instruction emulation has a gnarly hack where the .fixup does a
return, luckily exceptions can easily emulate a return so create a new
extable type to fully do the FASTOP magic.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/x86/include/asm/extable_fixup_types.h |    1 +
 arch/x86/kvm/emulate.c                     |   14 +++-----------
 arch/x86/mm/extable.c                      |   11 +++++++++++
 3 files changed, 15 insertions(+), 11 deletions(-)

--- a/arch/x86/include/asm/extable_fixup_types.h
+++ b/arch/x86/include/asm/extable_fixup_types.h
@@ -40,5 +40,6 @@
 #define	EX_TYPE_ONE_REG			(EX_TYPE_IMM_REG | EX_TYPE_IMM(1))
 
 #define	EX_TYPE_FAULT_SGX		18
+#define	EX_TYPE_KVM_FASTOP		19
 
 #endif
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -344,7 +344,7 @@ static int fastop(struct x86_emulate_ctx
 	__FOP_RET(#op "_" #dst)
 
 #define FOP1EEX(op,  dst) \
-	FOP1E(op, dst) _ASM_EXTABLE(10b, kvm_fastop_exception)
+	FOP1E(op, dst) _ASM_EXTABLE_TYPE(10b, 10b, EX_TYPE_KVM_FASTOP)
 
 #define FASTOP1(op) \
 	FOP_START(op) \
@@ -434,10 +434,6 @@ static int fastop(struct x86_emulate_ctx
 	#op " %al \n\t" \
 	__FOP_RET(#op)
 
-asm(".pushsection .fixup, \"ax\"\n"
-    "kvm_fastop_exception: xor %esi, %esi; ret\n"
-    ".popsection");
-
 FOP_START(setcc)
 FOP_SETCC(seto)
 FOP_SETCC(setno)
@@ -473,12 +469,8 @@ FOP_END;
  \
 	asm volatile("1:" insn "\n" \
 	             "2:\n" \
-	             ".pushsection .fixup, \"ax\"\n" \
-	             "3: movl $1, %[_fault]\n" \
-	             "   jmp  2b\n" \
-	             ".popsection\n" \
-	             _ASM_EXTABLE(1b, 3b) \
-	             : [_fault] "+qm"(_fault) inoutclob ); \
+		     _ASM_EXTABLE_TYPE_REG(1b, 2b, EX_TYPE_ONE_REG, %[_fault]) \
+	             : [_fault] "+r"(_fault) inoutclob ); \
  \
 	_fault ? X86EMUL_UNHANDLEABLE : X86EMUL_CONTINUE; \
 })
--- a/arch/x86/mm/extable.c
+++ b/arch/x86/mm/extable.c
@@ -10,6 +10,7 @@
 #include <asm/traps.h>
 #include <asm/kdebug.h>
 #include <asm/sgx.h>
+#include <asm/text-patching.h>
 
 static inline unsigned long *pt_regs_nr(struct pt_regs *regs, int nr)
 {
@@ -154,6 +155,14 @@ static bool ex_handler_sgx(const struct
 	return ex_handler_default(fixup, regs);
 }
 
+static bool ex_handler_kvm_fastop(const struct exception_table_entry *fixup,
+				  struct pt_regs *regs)
+{
+	regs->si = 0;
+	int3_emulate_ret(regs);
+	return true;
+}
+
 #define EX_TYPE_MASK	0x000000FF
 #define EX_REG_MASK	0x00000F00
 #define EX_FLAG_MASK	0x0000F000
@@ -231,6 +240,8 @@ int fixup_exception(struct pt_regs *regs
 		return ex_handler_imm_reg(e, regs, reg, imm);
 	case EX_TYPE_FAULT_SGX:
 		return ex_handler_sgx(e, regs, trapnr);
+	case EX_TYPE_KVM_FASTOP:
+		return ex_handler_kvm_fastop(e, regs);
 	}
 	BUG();
 }

Re: [RFC][PATCH 18/22] x86,kvm: Remove .fixup usage

[Paolo Bonzini]

On 11/4/21 17:47, Peter Zijlstra wrote:
> KVM instruction emulation has a gnarly hack where the .fixup does a
> return, luckily exceptions can easily emulate a return so create a new
> extable type to fully do the FASTOP magic.
> 
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>   arch/x86/include/asm/extable_fixup_types.h |    1 +
>   arch/x86/kvm/emulate.c                     |   14 +++-----------
>   arch/x86/mm/extable.c                      |   11 +++++++++++
>   3 files changed, 15 insertions(+), 11 deletions(-)
> 
> --- a/arch/x86/include/asm/extable_fixup_types.h
> +++ b/arch/x86/include/asm/extable_fixup_types.h
> @@ -40,5 +40,6 @@
>   #define	EX_TYPE_ONE_REG			(EX_TYPE_IMM_REG | EX_TYPE_IMM(1))
>   
>   #define	EX_TYPE_FAULT_SGX		18
> +#define	EX_TYPE_KVM_FASTOP		19
>   
>   #endif
> --- a/arch/x86/kvm/emulate.c
> +++ b/arch/x86/kvm/emulate.c
> @@ -344,7 +344,7 @@ static int fastop(struct x86_emulate_ctx
>   	__FOP_RET(#op "_" #dst)
>   
>   #define FOP1EEX(op,  dst) \
> -	FOP1E(op, dst) _ASM_EXTABLE(10b, kvm_fastop_exception)
> +	FOP1E(op, dst) _ASM_EXTABLE_TYPE(10b, 10b, EX_TYPE_KVM_FASTOP)

There's a ret right after the 10b label, so I think you can just use this:

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 493511efa3dc..f382c03c5954 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -315,7 +315,7 @@ static int fastop(struct x86_emulate_ctxt *ctxt, fastop_t fop);
  	__FOP_FUNC(#name)
  
  #define __FOP_RET(name) \
-	"ret \n\t" \
+	"11: ret \n\t" \
  	".size " name ", .-" name "\n\t"
  
  #define FOP_RET(name) \
@@ -344,7 +344,7 @@ static int fastop(struct x86_emulate_ctxt *ctxt, fastop_t fop);
  	__FOP_RET(#op "_" #dst)
  
  #define FOP1EEX(op,  dst) \
-	FOP1E(op, dst) _ASM_EXTABLE(10b, kvm_fastop_exception)
+	FOP1E(op, dst) _ASM_EXTABLE_TYPE_REG(10b, 11b, EX_TYPE_ZERO_REG, %esi)
  
  #define FASTOP1(op) \
  	FOP_START(op) \

Paolo

Re: [RFC][PATCH 18/22] x86,kvm: Remove .fixup usage

[Peter Zijlstra]

On Thu, Nov 04, 2021 at 07:50:36PM +0100, Paolo Bonzini wrote:

> > +	FOP1E(op, dst) _ASM_EXTABLE_TYPE(10b, 10b, EX_TYPE_KVM_FASTOP)
> 
> There's a ret right after the 10b label, so I think you can just use this:
> 
> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> index 493511efa3dc..f382c03c5954 100644
> --- a/arch/x86/kvm/emulate.c
> +++ b/arch/x86/kvm/emulate.c
> @@ -315,7 +315,7 @@ static int fastop(struct x86_emulate_ctxt *ctxt, fastop_t fop);
>  	__FOP_FUNC(#name)
>  #define __FOP_RET(name) \
> -	"ret \n\t" \
> +	"11: ret \n\t" \
>  	".size " name ", .-" name "\n\t"
>  #define FOP_RET(name) \
> @@ -344,7 +344,7 @@ static int fastop(struct x86_emulate_ctxt *ctxt, fastop_t fop);
>  	__FOP_RET(#op "_" #dst)
>  #define FOP1EEX(op,  dst) \
> -	FOP1E(op, dst) _ASM_EXTABLE(10b, kvm_fastop_exception)
> +	FOP1E(op, dst) _ASM_EXTABLE_TYPE_REG(10b, 11b, EX_TYPE_ZERO_REG, %esi)
>  #define FASTOP1(op) \
>  	FOP_START(op) \

That's much nicer, thanks!

[RFC][PATCH 19/22] x86,usercopy_32: Simplify..

[Peter Zijlstra]

Have an exception jump to a .fixup to only immediately jump out is
daft, jump to the right place in one go.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/x86/lib/usercopy_32.c |   40 ++++++++++++++++++++--------------------
 1 file changed, 20 insertions(+), 20 deletions(-)

--- a/arch/x86/lib/usercopy_32.c
+++ b/arch/x86/lib/usercopy_32.c
@@ -257,28 +257,28 @@ static unsigned long __copy_user_intel_n
 	       "8:\n"
 	       ".section .fixup,\"ax\"\n"
 	       "9:      lea 0(%%eax,%0,4),%0\n"
-	       "16:     jmp 8b\n"
+	       "        jmp 8b\n"
 	       ".previous\n"
-	       _ASM_EXTABLE_UA(0b, 16b)
-	       _ASM_EXTABLE_UA(1b, 16b)
-	       _ASM_EXTABLE_UA(2b, 16b)
-	       _ASM_EXTABLE_UA(21b, 16b)
-	       _ASM_EXTABLE_UA(3b, 16b)
-	       _ASM_EXTABLE_UA(31b, 16b)
-	       _ASM_EXTABLE_UA(4b, 16b)
-	       _ASM_EXTABLE_UA(41b, 16b)
-	       _ASM_EXTABLE_UA(10b, 16b)
-	       _ASM_EXTABLE_UA(51b, 16b)
-	       _ASM_EXTABLE_UA(11b, 16b)
-	       _ASM_EXTABLE_UA(61b, 16b)
-	       _ASM_EXTABLE_UA(12b, 16b)
-	       _ASM_EXTABLE_UA(71b, 16b)
-	       _ASM_EXTABLE_UA(13b, 16b)
-	       _ASM_EXTABLE_UA(81b, 16b)
-	       _ASM_EXTABLE_UA(14b, 16b)
-	       _ASM_EXTABLE_UA(91b, 16b)
+	       _ASM_EXTABLE_UA(0b, 8b)
+	       _ASM_EXTABLE_UA(1b, 8b)
+	       _ASM_EXTABLE_UA(2b, 8b)
+	       _ASM_EXTABLE_UA(21b, 8b)
+	       _ASM_EXTABLE_UA(3b, 8b)
+	       _ASM_EXTABLE_UA(31b, 8b)
+	       _ASM_EXTABLE_UA(4b, 8b)
+	       _ASM_EXTABLE_UA(41b, 8b)
+	       _ASM_EXTABLE_UA(10b, 8b)
+	       _ASM_EXTABLE_UA(51b, 8b)
+	       _ASM_EXTABLE_UA(11b, 8b)
+	       _ASM_EXTABLE_UA(61b, 8b)
+	       _ASM_EXTABLE_UA(12b, 8b)
+	       _ASM_EXTABLE_UA(71b, 8b)
+	       _ASM_EXTABLE_UA(13b, 8b)
+	       _ASM_EXTABLE_UA(81b, 8b)
+	       _ASM_EXTABLE_UA(14b, 8b)
+	       _ASM_EXTABLE_UA(91b, 8b)
 	       _ASM_EXTABLE_UA(6b, 9b)
-	       _ASM_EXTABLE_UA(7b, 16b)
+	       _ASM_EXTABLE_UA(7b, 8b)
 	       : "=&c"(size), "=&D" (d0), "=&S" (d1)
 	       :  "1"(to), "2"(from), "0"(size)
 	       : "eax", "edx", "memory");

[RFC][PATCH 20/22] x86,usercopy: Remove .fixup usage

[Peter Zijlstra]

Typically usercopy does whole word copies followed by a number of byte
copies to finish the tail. This means that on exception it needs to
compute the remaining length as: words*sizeof(long) + bytes.

Create a new extable handler to do just this.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/x86/include/asm/extable_fixup_types.h |    5 +++++
 arch/x86/lib/usercopy_32.c                 |   28 +++++-----------------------
 arch/x86/lib/usercopy_64.c                 |    8 +++-----
 arch/x86/mm/extable.c                      |    9 +++++++++
 4 files changed, 22 insertions(+), 28 deletions(-)

--- a/arch/x86/include/asm/extable_fixup_types.h
+++ b/arch/x86/include/asm/extable_fixup_types.h
@@ -42,4 +42,9 @@
 #define	EX_TYPE_FAULT_SGX		18
 #define	EX_TYPE_KVM_FASTOP		19
 
+#define	EX_TYPE_UACCESS_LEN		20 /* cx := reg + imm*cx */
+#define	EX_TYPE_UACCESS_LEN1		(EX_TYPE_UACCESS_LEN | EX_TYPE_IMM(1))
+#define	EX_TYPE_UACCESS_LEN4		(EX_TYPE_UACCESS_LEN | EX_TYPE_IMM(4))
+#define	EX_TYPE_UACCESS_LEN8		(EX_TYPE_UACCESS_LEN | EX_TYPE_IMM(8))
+
 #endif
--- a/arch/x86/lib/usercopy_32.c
+++ b/arch/x86/lib/usercopy_32.c
@@ -43,11 +43,7 @@ do {									\
 		"	movl %2,%0\n"					\
 		"1:	rep; stosb\n"					\
 		"2: " ASM_CLAC "\n"					\
-		".section .fixup,\"ax\"\n"				\
-		"3:	lea 0(%2,%0,4),%0\n"				\
-		"	jmp 2b\n"					\
-		".previous\n"						\
-		_ASM_EXTABLE_UA(0b, 3b)					\
+		_ASM_EXTABLE_TYPE_REG(0b, 2b, EX_TYPE_UACCESS_LEN4, %2)	\
 		_ASM_EXTABLE_UA(1b, 2b)					\
 		: "=&c"(size), "=&D" (__d0)				\
 		: "r"(size & 3), "0"(size / 4), "1"(addr), "a"(0));	\
@@ -149,10 +145,6 @@ __copy_user_intel(void __user *to, const
 		       "36:    movl %%eax, %0\n"
 		       "37:    rep; movsb\n"
 		       "100:\n"
-		       ".section .fixup,\"ax\"\n"
-		       "101:   lea 0(%%eax,%0,4),%0\n"
-		       "       jmp 100b\n"
-		       ".previous\n"
 		       _ASM_EXTABLE_UA(1b, 100b)
 		       _ASM_EXTABLE_UA(2b, 100b)
 		       _ASM_EXTABLE_UA(3b, 100b)
@@ -190,7 +182,7 @@ __copy_user_intel(void __user *to, const
 		       _ASM_EXTABLE_UA(35b, 100b)
 		       _ASM_EXTABLE_UA(36b, 100b)
 		       _ASM_EXTABLE_UA(37b, 100b)
-		       _ASM_EXTABLE_UA(99b, 101b)
+		       _ASM_EXTABLE_TYPE_REG(99b, 100b, EX_TYPE_UACCESS_LEN4, %eax)
 		       : "=&c"(size), "=&D" (d0), "=&S" (d1)
 		       :  "1"(to), "2"(from), "0"(size)
 		       : "eax", "edx", "memory");
@@ -255,10 +247,6 @@ static unsigned long __copy_user_intel_n
 	       "        movl %%eax,%0\n"
 	       "7:      rep; movsb\n"
 	       "8:\n"
-	       ".section .fixup,\"ax\"\n"
-	       "9:      lea 0(%%eax,%0,4),%0\n"
-	       "        jmp 8b\n"
-	       ".previous\n"
 	       _ASM_EXTABLE_UA(0b, 8b)
 	       _ASM_EXTABLE_UA(1b, 8b)
 	       _ASM_EXTABLE_UA(2b, 8b)
@@ -277,7 +265,7 @@ static unsigned long __copy_user_intel_n
 	       _ASM_EXTABLE_UA(81b, 8b)
 	       _ASM_EXTABLE_UA(14b, 8b)
 	       _ASM_EXTABLE_UA(91b, 8b)
-	       _ASM_EXTABLE_UA(6b, 9b)
+	       _ASM_EXTABLE_TYPE_REG(6b, 8b, EX_TYPE_UACCESS_LEN4, %eax)
 	       _ASM_EXTABLE_UA(7b, 8b)
 	       : "=&c"(size), "=&D" (d0), "=&S" (d1)
 	       :  "1"(to), "2"(from), "0"(size)
@@ -315,14 +303,8 @@ do {									\
 		"	movl %3,%0\n"					\
 		"1:	rep; movsb\n"					\
 		"2:\n"							\
-		".section .fixup,\"ax\"\n"				\
-		"5:	addl %3,%0\n"					\
-		"	jmp 2b\n"					\
-		"3:	lea 0(%3,%0,4),%0\n"				\
-		"	jmp 2b\n"					\
-		".previous\n"						\
-		_ASM_EXTABLE_UA(4b, 5b)					\
-		_ASM_EXTABLE_UA(0b, 3b)					\
+		_ASM_EXTABLE_TYPE_REG(4b, 2b, EX_TYPE_UACCESS_LEN1, %3)	\
+		_ASM_EXTABLE_TYPE_REG(0b, 2b, EX_TYPE_UACCESS_LEN4, %3)	\
 		_ASM_EXTABLE_UA(1b, 2b)					\
 		: "=&c"(size), "=&D" (__d0), "=&S" (__d1), "=r"(__d2)	\
 		: "3"(size), "0"(size), "1"(to), "2"(from)		\
--- a/arch/x86/lib/usercopy_64.c
+++ b/arch/x86/lib/usercopy_64.c
@@ -35,12 +35,10 @@ unsigned long __clear_user(void __user *
 		"	incq   %[dst]\n"
 		"	decl %%ecx ; jnz  1b\n"
 		"2:\n"
-		".section .fixup,\"ax\"\n"
-		"3:	lea 0(%[size1],%[size8],8),%[size8]\n"
-		"	jmp 2b\n"
-		".previous\n"
-		_ASM_EXTABLE_UA(0b, 3b)
+
+		_ASM_EXTABLE_TYPE_REG(0b, 2b, EX_TYPE_UACCESS_LEN8, %[size1])
 		_ASM_EXTABLE_UA(1b, 2b)
+
 		: [size8] "=&c"(size), [dst] "=&D" (__d0)
 		: [size1] "r"(size & 7), "[size8]" (size / 8), "[dst]"(addr));
 	clac();
--- a/arch/x86/mm/extable.c
+++ b/arch/x86/mm/extable.c
@@ -163,6 +163,13 @@ static bool ex_handler_kvm_fastop(const
 	return true;
 }
 
+static bool ex_handler_uaccess_len(const struct exception_table_entry *fixup,
+				   struct pt_regs *regs, int trapnr, int reg, int imm)
+{
+	regs->cx = imm * regs->cx + *pt_regs_nr(regs, reg);
+	return ex_handler_uaccess(fixup, regs, trapnr);
+}
+
 #define EX_TYPE_MASK	0x000000FF
 #define EX_REG_MASK	0x00000F00
 #define EX_FLAG_MASK	0x0000F000
@@ -242,6 +249,8 @@ int fixup_exception(struct pt_regs *regs
 		return ex_handler_sgx(e, regs, trapnr);
 	case EX_TYPE_KVM_FASTOP:
 		return ex_handler_kvm_fastop(e, regs);
+	case EX_TYPE_UACCESS_LEN:
+		return ex_handler_uaccess_len(e, regs, trapnr, reg, imm);
 	}
 	BUG();
 }

[RFC][PATCH 21/22] x86,word-at-a-time: Remove .fixup usage

[Peter Zijlstra]

Push the load_unaligned_zeropad() exception into exception context by
adding a new extable type. This however requires we have both the
address and the output register. Since we can only have a single
register argument, use the same for both.

This also means the source can no longer use "m" constraint.

XXX: I'm not really happy with this patch

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/x86/include/asm/extable_fixup_types.h |    2 ++
 arch/x86/include/asm/word-at-a-time.h      |   27 +++++++--------------------
 arch/x86/mm/extable.c                      |   17 +++++++++++++++++
 3 files changed, 26 insertions(+), 20 deletions(-)

--- a/arch/x86/include/asm/extable_fixup_types.h
+++ b/arch/x86/include/asm/extable_fixup_types.h
@@ -47,4 +47,6 @@
 #define	EX_TYPE_UACCESS_LEN4		(EX_TYPE_UACCESS_LEN | EX_TYPE_IMM(4))
 #define	EX_TYPE_UACCESS_LEN8		(EX_TYPE_UACCESS_LEN | EX_TYPE_IMM(8))
 
+#define	EX_TYPE_LOAD_UNALIGNED		21 /* reg := (reg) */
+
 #endif
--- a/arch/x86/include/asm/word-at-a-time.h
+++ b/arch/x86/include/asm/word-at-a-time.h
@@ -79,27 +79,14 @@ static inline unsigned long find_zero(un
  */
 static inline unsigned long load_unaligned_zeropad(const void *addr)
 {
-	unsigned long ret, dummy;
+	unsigned long ret;
+
+	asm("1:\tmov (%0),%0\n"
+	    "2:\n"
+	    _ASM_EXTABLE_TYPE_REG(1b, 2b, EX_TYPE_LOAD_UNALIGNED, %0)
+	    : "=&r" (ret)
+	    : "0" ((unsigned long)addr));
 
-	asm(
-		"1:\tmov %2,%0\n"
-		"2:\n"
-		".section .fixup,\"ax\"\n"
-		"3:\t"
-		"lea %2,%1\n\t"
-		"and %3,%1\n\t"
-		"mov (%1),%0\n\t"
-		"leal %2,%%ecx\n\t"
-		"andl %4,%%ecx\n\t"
-		"shll $3,%%ecx\n\t"
-		"shr %%cl,%0\n\t"
-		"jmp 2b\n"
-		".previous\n"
-		_ASM_EXTABLE(1b, 3b)
-		:"=&r" (ret),"=&c" (dummy)
-		:"m" (*(unsigned long *)addr),
-		 "i" (-sizeof(unsigned long)),
-		 "i" (sizeof(unsigned long)-1));
 	return ret;
 }
 
--- a/arch/x86/mm/extable.c
+++ b/arch/x86/mm/extable.c
@@ -170,6 +170,21 @@ static bool ex_handler_uaccess_len(const
 	return ex_handler_uaccess(fixup, regs, trapnr);
 }
 
+static bool ex_handler_load_unaligned_zeropad(const struct exception_table_entry *fixup,
+					      struct pt_regs *regs, int reg)
+{
+	unsigned long addr, offset, data;
+
+	addr = *pt_regs_nr(regs, reg);
+	offset = addr & (sizeof(unsigned long) - 1);
+	addr &= ~(sizeof(unsigned long) - 1);
+	data = *(unsigned long *)addr;
+	data >>= offset*8;
+	*pt_regs_nr(regs, reg) = data;
+
+	return ex_handler_default(fixup, regs);
+}
+
 #define EX_TYPE_MASK	0x000000FF
 #define EX_REG_MASK	0x00000F00
 #define EX_FLAG_MASK	0x0000F000
@@ -251,6 +266,8 @@ int fixup_exception(struct pt_regs *regs
 		return ex_handler_kvm_fastop(e, regs);
 	case EX_TYPE_UACCESS_LEN:
 		return ex_handler_uaccess_len(e, regs, trapnr, reg, imm);
+	case EX_TYPE_LOAD_UNALIGNED:
+		return ex_handler_load_unaligned_zeropad(e, regs, reg);
 	}
 	BUG();
 }

Re: [RFC][PATCH 21/22] x86,word-at-a-time: Remove .fixup usage

[Josh Poimboeuf]

On Thu, Nov 04, 2021 at 05:47:50PM +0100, Peter Zijlstra wrote:
> XXX: I'm not really happy with this patch
>
>  static inline unsigned long load_unaligned_zeropad(const void *addr)
>  {
> -	unsigned long ret, dummy;
> +	unsigned long ret;
> +
> +	asm("1:\tmov (%0),%0\n"
> +	    "2:\n"
> +	    _ASM_EXTABLE_TYPE_REG(1b, 2b, EX_TYPE_LOAD_UNALIGNED, %0)
> +	    : "=&r" (ret)
> +	    : "0" ((unsigned long)addr));
>  
> -	asm(
> -		"1:\tmov %2,%0\n"
> -		"2:\n"
> -		".section .fixup,\"ax\"\n"
> -		"3:\t"
> -		"lea %2,%1\n\t"
> -		"and %3,%1\n\t"
> -		"mov (%1),%0\n\t"
> -		"leal %2,%%ecx\n\t"
> -		"andl %4,%%ecx\n\t"
> -		"shll $3,%%ecx\n\t"
> -		"shr %%cl,%0\n\t"
> -		"jmp 2b\n"
> -		".previous\n"
> -		_ASM_EXTABLE(1b, 3b)
> -		:"=&r" (ret),"=&c" (dummy)
> -		:"m" (*(unsigned long *)addr),
> -		 "i" (-sizeof(unsigned long)),
> -		 "i" (sizeof(unsigned long)-1));
>  	return ret;
>  }

Yeah, it hurts code generation and I guess it's a hot path.

Maybe put the fixup code in the function.  In case of
CONFIG_CC_HAS_ASM_GOTO_OUTPUT, it could be at a label at the end of the
function.  Otherwise it'd have to be inline.

-- 
Josh

Re: [RFC][PATCH 21/22] x86,word-at-a-time: Remove .fixup usage

[Peter Zijlstra]

On Thu, Nov 04, 2021 at 04:33:10PM -0700, Josh Poimboeuf wrote:
> On Thu, Nov 04, 2021 at 05:47:50PM +0100, Peter Zijlstra wrote:
> > XXX: I'm not really happy with this patch
> >
> >  static inline unsigned long load_unaligned_zeropad(const void *addr)
> >  {
> > -	unsigned long ret, dummy;
> > +	unsigned long ret;
> > +
> > +	asm("1:\tmov (%0),%0\n"
> > +	    "2:\n"
> > +	    _ASM_EXTABLE_TYPE_REG(1b, 2b, EX_TYPE_LOAD_UNALIGNED, %0)
> > +	    : "=&r" (ret)
> > +	    : "0" ((unsigned long)addr));
> >  
> > -	asm(
> > -		"1:\tmov %2,%0\n"
> > -		"2:\n"
> > -		".section .fixup,\"ax\"\n"
> > -		"3:\t"
> > -		"lea %2,%1\n\t"
> > -		"and %3,%1\n\t"
> > -		"mov (%1),%0\n\t"
> > -		"leal %2,%%ecx\n\t"
> > -		"andl %4,%%ecx\n\t"
> > -		"shll $3,%%ecx\n\t"
> > -		"shr %%cl,%0\n\t"
> > -		"jmp 2b\n"
> > -		".previous\n"
> > -		_ASM_EXTABLE(1b, 3b)
> > -		:"=&r" (ret),"=&c" (dummy)
> > -		:"m" (*(unsigned long *)addr),
> > -		 "i" (-sizeof(unsigned long)),
> > -		 "i" (sizeof(unsigned long)-1));
> >  	return ret;
> >  }
> 
> Yeah, it hurts code generation and I guess it's a hot path.
> 
> Maybe put the fixup code in the function.  In case of
> CONFIG_CC_HAS_ASM_GOTO_OUTPUT, it could be at a label at the end of the
> function.  Otherwise it'd have to be inline.

Ooh, yes. If we use ASM_GOTO_OUTPUT then it'll work just right, and the
legacy code will get worse, but I can live with that (everybody is
already building their kernels with gcc-11/clang anyway :-))

[RFC][PATCH 22/22] x86: Remove .fixup section

[Peter Zijlstra]

No moar user, kill it dead.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/x86/entry/vdso/vdso-layout.lds.S |    1 -
 arch/x86/kernel/vmlinux.lds.S         |    1 -
 2 files changed, 2 deletions(-)

--- a/arch/x86/entry/vdso/vdso-layout.lds.S
+++ b/arch/x86/entry/vdso/vdso-layout.lds.S
@@ -77,7 +77,6 @@ SECTIONS
 
 	.text		: {
 		*(.text*)
-		*(.fixup)
 	}						:text	=0x90909090,
 
 
--- a/arch/x86/kernel/vmlinux.lds.S
+++ b/arch/x86/kernel/vmlinux.lds.S
@@ -137,7 +137,6 @@ SECTIONS
 		ALIGN_ENTRY_TEXT_END
 		SOFTIRQENTRY_TEXT
 		STATIC_CALL_TEXT
-		*(.fixup)
 		*(.gnu.warning)
 
 #ifdef CONFIG_RETPOLINE

Re: [RFC][PATCH 22/22] x86: Remove .fixup section

[Josh Poimboeuf]

On Thu, Nov 04, 2021 at 05:47:51PM +0100, Peter Zijlstra wrote:
> No moar user, kill it dead.
> 
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  arch/x86/entry/vdso/vdso-layout.lds.S |    1 -
>  arch/x86/kernel/vmlinux.lds.S         |    1 -
>  2 files changed, 2 deletions(-)
> 
> --- a/arch/x86/entry/vdso/vdso-layout.lds.S
> +++ b/arch/x86/entry/vdso/vdso-layout.lds.S
> @@ -77,7 +77,6 @@ SECTIONS
>  
>  	.text		: {
>  		*(.text*)
> -		*(.fixup)
>  	}						:text	=0x90909090,
>  
>  
> --- a/arch/x86/kernel/vmlinux.lds.S
> +++ b/arch/x86/kernel/vmlinux.lds.S
> @@ -137,7 +137,6 @@ SECTIONS
>  		ALIGN_ENTRY_TEXT_END
>  		SOFTIRQENTRY_TEXT
>  		STATIC_CALL_TEXT
> -		*(.fixup)
>  		*(.gnu.warning)
>  
>  #ifdef CONFIG_RETPOLINE

Objtool also has a reference to .fixup.

-- 
Josh