Re: [PATCH] x86: Add support for 64bit get_user() on x86-32

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Dec 12, 2012 at 08:15:12AM -0800, H. Peter Anvin wrote:
> Can we worry about this after the merge window?

Sure. The only use I have for this is in my drm/kms atomic
modeset/pageflip patch set, and that's not 3.8 material.

> 
> ville.syrjala@linux.intel.com wrote:
> 
> >From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> >Implement __get_user_8() for x86-32. It will return the
> >64bit result in edx:eax register pair, and ecx is used
> >to pass in the address and return the error value.
> >
> >For consistency, change the register assignment for all
> >other __get_user_x() variants, so that address is passed in
> >ecx/rcx, the error value is returned in ecx/rcx, and eax/rax
> >contains the actual value.
> >
> >This is a partial refresh of a patch [1] by Jamie Lokier from
> >2004. Only the minimal changes to implement 64bit get_user()
> >were picked from the original patch.
> >
> >[1] http://article.gmane.org/gmane.linux.kernel/198823
> >
> >Cc: Jamie Lokier <jamie@shareable.org>
> >Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >---
> > arch/x86/include/asm/uaccess.h  |   17 ++++++--
> > arch/x86/kernel/i386_ksyms_32.c |    1 +
> >arch/x86/lib/getuser.S          |   82
> >++++++++++++++++++++++++++------------
> > 3 files changed, 69 insertions(+), 31 deletions(-)
> >
> >diff --git a/arch/x86/include/asm/uaccess.h
> >b/arch/x86/include/asm/uaccess.h
> >index 7ccf8d1..3f4387e 100644
> >--- a/arch/x86/include/asm/uaccess.h
> >+++ b/arch/x86/include/asm/uaccess.h
> >@@ -127,7 +127,7 @@ extern int __get_user_bad(void);
> > 
> > #define __get_user_x(size, ret, x, ptr)		      \
> > 	asm volatile("call __get_user_" #size	      \
> >-		     : "=a" (ret), "=d" (x)	      \
> >+		     : "=c" (ret), "=a" (x)	      \
> > 		     : "0" (ptr))		      \
> > 
> > /* Careful: we have to cast the result to the type of the pointer
> >@@ -151,8 +151,11 @@ extern int __get_user_bad(void);
> >  * On error, the variable @x is set to zero.
> >  */
> > #ifdef CONFIG_X86_32
> >-#define __get_user_8(__ret_gu, __val_gu, ptr)				\
> >-		__get_user_x(X, __ret_gu, __val_gu, ptr)
> >+#define __get_user_8(ret, x, ptr)		      \
> >+	asm volatile("call __get_user_8"	      \
> >+		     : "=c" (ret), "=A" (x)	      \
> >+		     : "0" (ptr))		      \
> >+
> > #else
> > #define __get_user_8(__ret_gu, __val_gu, ptr)				\
> > 		__get_user_x(8, __ret_gu, __val_gu, ptr)
> >@@ -162,6 +165,7 @@ extern int __get_user_bad(void);
> > ({									\
> > 	int __ret_gu;							\
> > 	unsigned long __val_gu;						\
> >+	unsigned long long __val_gu8;					\
> > 	__chk_user_ptr(ptr);						\
> > 	might_fault();							\
> > 	switch (sizeof(*(ptr))) {					\
> >@@ -175,13 +179,16 @@ extern int __get_user_bad(void);
> > 		__get_user_x(4, __ret_gu, __val_gu, ptr);		\
> > 		break;							\
> > 	case 8:								\
> >-		__get_user_8(__ret_gu, __val_gu, ptr);			\
> >+		__get_user_8(__ret_gu, __val_gu8, ptr);			\
> > 		break;							\
> > 	default:							\
> > 		__get_user_x(X, __ret_gu, __val_gu, ptr);		\
> > 		break;							\
> > 	}								\
> >-	(x) = (__typeof__(*(ptr)))__val_gu;				\
> >+	if (sizeof(*(ptr)) == 8)					\
> >+		(x) = (__typeof__(*(ptr)))__val_gu8;			\
> >+	else								\
> >+		(x) = (__typeof__(*(ptr)))__val_gu;			\
> > 	__ret_gu;							\
> > })
> > 
> >diff --git a/arch/x86/kernel/i386_ksyms_32.c
> >b/arch/x86/kernel/i386_ksyms_32.c
> >index 9c3bd4a..0fa6912 100644
> >--- a/arch/x86/kernel/i386_ksyms_32.c
> >+++ b/arch/x86/kernel/i386_ksyms_32.c
> >@@ -26,6 +26,7 @@ EXPORT_SYMBOL(csum_partial_copy_generic);
> > EXPORT_SYMBOL(__get_user_1);
> > EXPORT_SYMBOL(__get_user_2);
> > EXPORT_SYMBOL(__get_user_4);
> >+EXPORT_SYMBOL(__get_user_8);
> > 
> > EXPORT_SYMBOL(__put_user_1);
> > EXPORT_SYMBOL(__put_user_2);
> >diff --git a/arch/x86/lib/getuser.S b/arch/x86/lib/getuser.S
> >index 156b9c8..38afef0 100644
> >--- a/arch/x86/lib/getuser.S
> >+++ b/arch/x86/lib/getuser.S
> >@@ -14,12 +14,11 @@
> > /*
> >  * __get_user_X
> >  *
> >- * Inputs:	%[r|e]ax contains the address.
> >- *		The register is modified, but all changes are undone
> >- *		before returning because the C code doesn't know about it.
> >+ * Inputs:	%[r|e]cx contains the address.
> >  *
> >- * Outputs:	%[r|e]ax is error code (0 or -EFAULT)
> >- *		%[r|e]dx contains zero-extended value
> >+ * Outputs:	%[r|e]cx is error code (0 or -EFAULT)
> >+ *		%[r|e]ax contains zero-extended value
> >+ *		%edx contains the high bits of the value for __get_user_8 on
> >x86-32
> >  *
> >  *
> >  * These functions should not modify any other registers,
> >@@ -38,12 +37,12 @@
> > 	.text
> > ENTRY(__get_user_1)
> > 	CFI_STARTPROC
> >-	GET_THREAD_INFO(%_ASM_DX)
> >-	cmp TI_addr_limit(%_ASM_DX),%_ASM_AX
> >+	GET_THREAD_INFO(%_ASM_AX)
> >+	cmp TI_addr_limit(%_ASM_AX),%_ASM_CX
> > 	jae bad_get_user
> > 	ASM_STAC
> >-1:	movzb (%_ASM_AX),%edx
> >-	xor %eax,%eax
> >+1:	movzb (%_ASM_CX),%eax
> >+	xor %ecx,%ecx
> > 	ASM_CLAC
> > 	ret
> > 	CFI_ENDPROC
> >@@ -51,14 +50,14 @@ ENDPROC(__get_user_1)
> > 
> > ENTRY(__get_user_2)
> > 	CFI_STARTPROC
> >-	add $1,%_ASM_AX
> >+	add $1,%_ASM_CX
> > 	jc bad_get_user
> >-	GET_THREAD_INFO(%_ASM_DX)
> >-	cmp TI_addr_limit(%_ASM_DX),%_ASM_AX
> >+	GET_THREAD_INFO(%_ASM_AX)
> >+	cmp TI_addr_limit(%_ASM_AX),%_ASM_CX
> > 	jae bad_get_user
> > 	ASM_STAC
> >-2:	movzwl -1(%_ASM_AX),%edx
> >-	xor %eax,%eax
> >+2:	movzwl -1(%_ASM_CX),%eax
> >+	xor %ecx,%ecx
> > 	ASM_CLAC
> > 	ret
> > 	CFI_ENDPROC
> >@@ -66,14 +65,14 @@ ENDPROC(__get_user_2)
> > 
> > ENTRY(__get_user_4)
> > 	CFI_STARTPROC
> >-	add $3,%_ASM_AX
> >+	add $3,%_ASM_CX
> > 	jc bad_get_user
> >-	GET_THREAD_INFO(%_ASM_DX)
> >-	cmp TI_addr_limit(%_ASM_DX),%_ASM_AX
> >+	GET_THREAD_INFO(%_ASM_AX)
> >+	cmp TI_addr_limit(%_ASM_AX),%_ASM_CX
> > 	jae bad_get_user
> > 	ASM_STAC
> >-3:	mov -3(%_ASM_AX),%edx
> >-	xor %eax,%eax
> >+3:	mov -3(%_ASM_CX),%eax
> >+	xor %ecx,%ecx
> > 	ASM_CLAC
> > 	ret
> > 	CFI_ENDPROC
> >@@ -82,14 +81,30 @@ ENDPROC(__get_user_4)
> > #ifdef CONFIG_X86_64
> > ENTRY(__get_user_8)
> > 	CFI_STARTPROC
> >-	add $7,%_ASM_AX
> >+	add $7,%_ASM_CX
> > 	jc bad_get_user
> >-	GET_THREAD_INFO(%_ASM_DX)
> >-	cmp TI_addr_limit(%_ASM_DX),%_ASM_AX
> >+	GET_THREAD_INFO(%_ASM_AX)
> >+	cmp TI_addr_limit(%_ASM_AX),%_ASM_CX
> > 	jae	bad_get_user
> > 	ASM_STAC
> >-4:	movq -7(%_ASM_AX),%_ASM_DX
> >-	xor %eax,%eax
> >+4:	movq -7(%_ASM_CX),%_ASM_AX
> >+	xor %ecx,%ecx
> >+	ASM_CLAC
> >+	ret
> >+	CFI_ENDPROC
> >+ENDPROC(__get_user_8)
> >+#else
> >+ENTRY(__get_user_8)
> >+	CFI_STARTPROC
> >+	add $7,%_ASM_CX
> >+	jc bad_get_user_8
> >+	GET_THREAD_INFO(%_ASM_AX)
> >+	cmp TI_addr_limit(%_ASM_AX),%_ASM_CX
> >+	jae	bad_get_user_8
> >+	ASM_STAC
> >+4:	mov -7(%_ASM_CX),%eax
> >+5:	mov -3(%_ASM_CX),%edx
> >+	xor %ecx,%ecx
> > 	ASM_CLAC
> > 	ret
> > 	CFI_ENDPROC
> >@@ -98,16 +113,31 @@ ENDPROC(__get_user_8)
> > 
> > bad_get_user:
> > 	CFI_STARTPROC
> >-	xor %edx,%edx
> >-	mov $(-EFAULT),%_ASM_AX
> >+	xor %eax,%eax
> >+	mov $(-EFAULT),%_ASM_CX
> > 	ASM_CLAC
> > 	ret
> > 	CFI_ENDPROC
> > END(bad_get_user)
> > 
> >+#ifdef CONFIG_X86_32
> >+bad_get_user_8:
> >+	CFI_STARTPROC
> >+	xor %eax,%eax
> >+	xor %edx,%edx
> >+	mov $(-EFAULT),%_ASM_CX
> >+	ASM_CLAC
> >+	ret
> >+	CFI_ENDPROC
> >+END(bad_get_user_8)
> >+#endif
> >+
> > 	_ASM_EXTABLE(1b,bad_get_user)
> > 	_ASM_EXTABLE(2b,bad_get_user)
> > 	_ASM_EXTABLE(3b,bad_get_user)
> > #ifdef CONFIG_X86_64
> > 	_ASM_EXTABLE(4b,bad_get_user)
> >+#else
> >+	_ASM_EXTABLE(4b,bad_get_user_8)
> >+	_ASM_EXTABLE(5b,bad_get_user_8)
> > #endif
> 
> -- 
> Sent from my mobile phone. Please excuse brevity and lack of formatting.

-- 
Ville Syrjälä
Intel OTC