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

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

[ville.syrjala]

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

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

[H. Peter Anvin]

Can we worry about this after the merge window?

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.

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

[Ville Syrjälä]

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

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

[H. Peter Anvin]

It's a good change -- and one which opens up even more unification -- but right now we're worried about 3.8.

"Ville Syrjälä" <ville.syrjala@linux.intel.com> wrote:

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

-- 
Sent from my mobile phone. Please excuse brevity and lack of formatting.

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

[Ville Syrjälä]

On Wed, Dec 12, 2012 at 01:34:03PM +0200, 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

Ping. I pretty much forgot about this patch since I posted it.

Based on the initial response there seems to be some interest towards
it at least. So I'm wondering what's the next step. Is it OK as is,
or does it need more work, or would people want to extend it to include
more of the original work?

I have quite a lot of other stuff on my plate currently, so it'd
actually be nice if I could find someone to adopt this patch,
especially if there's interest in increasing the scope.

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

-- 
Ville Syrjälä
Intel OTC

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

[H. Peter Anvin]

On 02/07/2013 08:53 AM, Ville Syrjälä wrote:
> 
> Based on the initial response there seems to be some interest towards
> it at least. So I'm wondering what's the next step. Is it OK as is,
> or does it need more work, or would people want to extend it to include
> more of the original work?
> 
> I have quite a lot of other stuff on my plate currently, so it'd
> actually be nice if I could find someone to adopt this patch,
> especially if there's interest in increasing the scope.
> 

Well, we can put it in and see what happens.  It looks good from what I
can see.

How have you tested it?

	-hpa

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

[Ville Syrjälä]

On Thu, Feb 07, 2013 at 09:59:00AM -0800, H. Peter Anvin wrote:
> On 02/07/2013 08:53 AM, Ville Syrjälä wrote:
> > 
> > Based on the initial response there seems to be some interest towards
> > it at least. So I'm wondering what's the next step. Is it OK as is,
> > or does it need more work, or would people want to extend it to include
> > more of the original work?
> > 
> > I have quite a lot of other stuff on my plate currently, so it'd
> > actually be nice if I could find someone to adopt this patch,
> > especially if there's interest in increasing the scope.
> > 
> 
> Well, we can put it in and see what happens.  It looks good from what I
> can see.
> 
> How have you tested it?

I've tried it with my drm/kms atomic pageflip/modeset code. I also had
a small test module that did a couple of different sized get_user()
calls, but I'll be damned if I can find it again.

-- 
Ville Syrjälä
Intel OTC

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

[H. Peter Anvin]

On 02/08/2013 08:24 AM, Ville Syrjälä wrote:
>>
>> How have you tested it?
> 
> I've tried it with my drm/kms atomic pageflip/modeset code. I also had
> a small test module that did a couple of different sized get_user()
> calls, but I'll be damned if I can find it again.
> 

Did you see the warning spewage for pointers when you built it?

	-hpa

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

[Ville Syrjälä]

On Fri, Feb 08, 2013 at 09:30:05AM -0800, H. Peter Anvin wrote:
> On 02/08/2013 08:24 AM, Ville Syrjälä wrote:
> >>
> >> How have you tested it?
> > 
> > I've tried it with my drm/kms atomic pageflip/modeset code. I also had
> > a small test module that did a couple of different sized get_user()
> > calls, but I'll be damned if I can find it again.
> > 
> 
> Did you see the warning spewage for pointers when you built it?

Apparently my config was too small to genenerate enough of those to catch
my eye. Most of those seem to come from drivers that have pointers in their
ioctl structs, no?

-- 
Ville Syrjälä
Intel OTC

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

[H. Peter Anvin]

Yes, or anything else getting a pointer in memory from user space.

"Ville Syrjälä" <ville.syrjala@linux.intel.com> wrote:

>On Fri, Feb 08, 2013 at 09:30:05AM -0800, H. Peter Anvin wrote:
>> On 02/08/2013 08:24 AM, Ville Syrjälä wrote:
>> >>
>> >> How have you tested it?
>> > 
>> > I've tried it with my drm/kms atomic pageflip/modeset code. I also
>had
>> > a small test module that did a couple of different sized get_user()
>> > calls, but I'll be damned if I can find it again.
>> > 
>> 
>> Did you see the warning spewage for pointers when you built it?
>
>Apparently my config was too small to genenerate enough of those to
>catch
>my eye. Most of those seem to come from drivers that have pointers in
>their
>ioctl structs, no?

-- 
Sent from my mobile phone. Please excuse brevity and lack of formatting.

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

[Borislav Petkov]

On Fri, Feb 08, 2013 at 11:08:52AM -0800, H. Peter Anvin wrote:
> Yes, or anything else getting a pointer in memory from user space.

Here are some more from a 32-bit build here:

fs/exec.c: In function ‘get_user_arg_ptr’:
fs/exec.c:414:6: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
fs/splice.c: In function ‘vmsplice_to_user’:
fs/splice.c:1556:11: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
ipc/syscall.c: In function ‘sys_ipc’:
ipc/syscall.c:39:7: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]

Thanks.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

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

[Russell King - ARM Linux]

On Sat, Feb 09, 2013 at 11:41:42AM +0100, Borislav Petkov wrote:
> On Fri, Feb 08, 2013 at 11:08:52AM -0800, H. Peter Anvin wrote:
> > Yes, or anything else getting a pointer in memory from user space.
> 
> Here are some more from a 32-bit build here:
> 
> fs/exec.c: In function ‘get_user_arg_ptr’:
> fs/exec.c:414:6: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
> fs/splice.c: In function ‘vmsplice_to_user’:
> fs/splice.c:1556:11: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
> ipc/syscall.c: In function ‘sys_ipc’:
> ipc/syscall.c:39:7: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]

Note that there's no need to build the entire tree to check for these -
you just need to have enough test cases which cover those found in the
kernel.

The set of test functions I replied with on the previous thread covers
all the cases I'm aware of in the kernel that matter, and should be
warning free except for the final test function (which is there to check
that the typechecking in get_user() does work.)

[tip:x86/mm] x86, mm: Use a bitfield to mask nuisance get_user() warnings

[tip-bot for H. Peter Anvin]

Commit-ID:  b390784dc1649f6e6c5e66e5f53c21e715ccf39b
Gitweb:     http://git.kernel.org/tip/b390784dc1649f6e6c5e66e5f53c21e715ccf39b
Author:     H. Peter Anvin <hpa@zytor.com>
AuthorDate: Mon, 11 Feb 2013 16:27:28 -0800
Committer:  H. Peter Anvin <hpa@zytor.com>
CommitDate: Mon, 11 Feb 2013 17:26:51 -0800

x86, mm: Use a bitfield to mask nuisance get_user() warnings

Even though it is never executed, gcc wants to warn for casting from
a large integer to a pointer.  Furthermore, using a variable with
__typeof__() doesn't work because __typeof__ retains storage
specifiers (const, restrict, volatile).

However, we can declare a bitfield using sizeof(), which is legal
because sizeof() is a constant expression.  This quiets the warning,
although the code generated isn't 100% identical from the baseline
before 96477b4 x86-32: Add support for 64bit get_user():

[x86-mb is baseline, x86-mm is this commit]

   text      data        bss     filename
113716147  15858380   35037184   tip.x86-mb/o.i386-allconfig/vmlinux
113716145  15858380   35037184   tip.x86-mm/o.i386-allconfig/vmlinux
 12989837   3597944   12255232   tip.x86-mb/o.i386-modconfig/vmlinux
 12989831   3597944   12255232   tip.x86-mm/o.i386-modconfig/vmlinux
  1462784    237608    1401988   tip.x86-mb/o.i386-noconfig/vmlinux
  1462837    237608    1401964   tip.x86-mm/o.i386-noconfig/vmlinux
  7938994    553688    7639040   tip.x86-mb/o.i386-pae/vmlinux
  7943136    557784    7639040   tip.x86-mm/o.i386-pae/vmlinux
  7186126    510572    6574080   tip.x86-mb/o.i386/vmlinux
  7186124    510572    6574080   tip.x86-mm/o.i386/vmlinux
103747269  33578856   65888256   tip.x86-mb/o.x86_64-allconfig/vmlinux
103746949  33578856   65888256   tip.x86-mm/o.x86_64-allconfig/vmlinux
 12116695  11035832   20160512   tip.x86-mb/o.x86_64-modconfig/vmlinux
 12116567  11035832   20160512   tip.x86-mm/o.x86_64-modconfig/vmlinux
  1700790    380524     511808   tip.x86-mb/o.x86_64-noconfig/vmlinux
  1700790    380524     511808   tip.x86-mm/o.x86_64-noconfig/vmlinux
 12413612   1133376    1101824   tip.x86-mb/o.x86_64/vmlinux
 12413484   1133376    1101824   tip.x86-mm/o.x86_64/vmlinux

Cc: Jamie Lokier <jamie@shareable.org>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Russell King <linux@arm.linux.org.uk>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Link: http://lkml.kernel.org/r/20130209110031.GA17833@n2100.arm.linux.org.uk
Signed-off-by: H. Peter Anvin <hpa@zytor.com>
---
 arch/x86/include/asm/uaccess.h | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h
index 1e96326..a8d1265 100644
--- a/arch/x86/include/asm/uaccess.h
+++ b/arch/x86/include/asm/uaccess.h
@@ -168,31 +168,29 @@ do {						      \
 #define get_user(x, ptr)						\
 ({									\
 	int __ret_gu;							\
-	unsigned long __val_gu;						\
-	unsigned long long __val_gu8;					\
+	struct {							\
+		unsigned long long __val_n : 8*sizeof(*(ptr));		\
+	} __val_gu;							\
 	__chk_user_ptr(ptr);						\
 	might_fault();							\
 	switch (sizeof(*(ptr))) {					\
 	case 1:								\
-		__get_user_x(1, __ret_gu, __val_gu, ptr);		\
+		__get_user_x(1, __ret_gu, __val_gu.__val_n, ptr);	\
 		break;							\
 	case 2:								\
-		__get_user_x(2, __ret_gu, __val_gu, ptr);		\
+		__get_user_x(2, __ret_gu, __val_gu.__val_n, ptr);	\
 		break;							\
 	case 4:								\
-		__get_user_x(4, __ret_gu, __val_gu, ptr);		\
+		__get_user_x(4, __ret_gu, __val_gu.__val_n, ptr);	\
 		break;							\
 	case 8:								\
-		__get_user_8(__ret_gu, __val_gu8, ptr);			\
+		__get_user_8(__ret_gu, __val_gu.__val_n, ptr);		\
 		break;							\
 	default:							\
-		__get_user_x(X, __ret_gu, __val_gu, ptr);		\
+		__get_user_x(X, __ret_gu, __val_gu.__val_n, ptr);	\
 		break;							\
 	}								\
-	if (sizeof(*(ptr)) == 8)					\
-		(x) = (__typeof__(*(ptr)))__val_gu8;			\
-	else								\
-		(x) = (__typeof__(*(ptr)))__val_gu;			\
+	(x) = (__typeof__(*(ptr)))__val_gu.__val_n;			\
 	__ret_gu;							\
 })
 

Re: [tip:x86/mm] x86, mm: Use a bitfield to mask nuisance get_user() warnings

[Linus Torvalds]

On Mon, Feb 11, 2013 at 5:37 PM, tip-bot for H. Peter Anvin
<hpa@zytor.com> wrote:
>
> However, we can declare a bitfield using sizeof(), which is legal
> because sizeof() is a constant expression.  This quiets the warning,
> although the code generated isn't 100% identical from the baseline
> before 96477b4 x86-32: Add support for 64bit get_user():

Christ. This is so ugly that it's almost a work of art.

Has anybody run this past any gcc developers? And if so, did they run
away screaming?

                 Linus

Re: [tip:x86/mm] x86, mm: Use a bitfield to mask nuisance get_user() warnings

[H. Peter Anvin]

[-- Attachment #1: Type: text/plain, Size: 623 bytes --]

On 02/11/2013 07:33 PM, Linus Torvalds wrote:
> On Mon, Feb 11, 2013 at 5:37 PM, tip-bot for H. Peter Anvin
> <hpa@zytor.com> wrote:
>>
>> However, we can declare a bitfield using sizeof(), which is legal
>> because sizeof() is a constant expression.  This quiets the warning,
>> although the code generated isn't 100% identical from the baseline
>> before 96477b4 x86-32: Add support for 64bit get_user():
> 
> Christ. This is so ugly that it's almost a work of art.

:)

> Has anybody run this past any gcc developers? And if so, did they run
> away screaming?

I haven't no... H.J., any comments on this patch?

	-hpa



[-- Attachment #2: Attached Message --]
[-- Type: message/rfc822, Size: 6168 bytes --]

From: "tip-bot for H. Peter Anvin" <hpa@zytor.com>
To: linux-tip-commits@vger.kernel.org
Cc: linux-kernel@vger.kernel.org, hpa@zytor.com, mingo@kernel.org, torvalds@linux-foundation.org, jamie@shareable.org, ville.syrjala@linux.intel.com, bp@alien8.de, linux@arm.linux.org.uk, tglx@linutronix.de
Subject: [tip:x86/mm] x86, mm: Use a bitfield to mask nuisance get_user() warnings
Date: Mon, 11 Feb 2013 17:37:52 -0800
Message-ID: <tip-b390784dc1649f6e6c5e66e5f53c21e715ccf39b@git.kernel.org>

Commit-ID:  b390784dc1649f6e6c5e66e5f53c21e715ccf39b
Gitweb:     http://git.kernel.org/tip/b390784dc1649f6e6c5e66e5f53c21e715ccf39b
Author:     H. Peter Anvin <hpa@zytor.com>
AuthorDate: Mon, 11 Feb 2013 16:27:28 -0800
Committer:  H. Peter Anvin <hpa@zytor.com>
CommitDate: Mon, 11 Feb 2013 17:26:51 -0800

x86, mm: Use a bitfield to mask nuisance get_user() warnings

Even though it is never executed, gcc wants to warn for casting from
a large integer to a pointer.  Furthermore, using a variable with
__typeof__() doesn't work because __typeof__ retains storage
specifiers (const, restrict, volatile).

However, we can declare a bitfield using sizeof(), which is legal
because sizeof() is a constant expression.  This quiets the warning,
although the code generated isn't 100% identical from the baseline
before 96477b4 x86-32: Add support for 64bit get_user():

[x86-mb is baseline, x86-mm is this commit]

   text      data        bss     filename
113716147  15858380   35037184   tip.x86-mb/o.i386-allconfig/vmlinux
113716145  15858380   35037184   tip.x86-mm/o.i386-allconfig/vmlinux
 12989837   3597944   12255232   tip.x86-mb/o.i386-modconfig/vmlinux
 12989831   3597944   12255232   tip.x86-mm/o.i386-modconfig/vmlinux
  1462784    237608    1401988   tip.x86-mb/o.i386-noconfig/vmlinux
  1462837    237608    1401964   tip.x86-mm/o.i386-noconfig/vmlinux
  7938994    553688    7639040   tip.x86-mb/o.i386-pae/vmlinux
  7943136    557784    7639040   tip.x86-mm/o.i386-pae/vmlinux
  7186126    510572    6574080   tip.x86-mb/o.i386/vmlinux
  7186124    510572    6574080   tip.x86-mm/o.i386/vmlinux
103747269  33578856   65888256   tip.x86-mb/o.x86_64-allconfig/vmlinux
103746949  33578856   65888256   tip.x86-mm/o.x86_64-allconfig/vmlinux
 12116695  11035832   20160512   tip.x86-mb/o.x86_64-modconfig/vmlinux
 12116567  11035832   20160512   tip.x86-mm/o.x86_64-modconfig/vmlinux
  1700790    380524     511808   tip.x86-mb/o.x86_64-noconfig/vmlinux
  1700790    380524     511808   tip.x86-mm/o.x86_64-noconfig/vmlinux
 12413612   1133376    1101824   tip.x86-mb/o.x86_64/vmlinux
 12413484   1133376    1101824   tip.x86-mm/o.x86_64/vmlinux

Cc: Jamie Lokier <jamie@shareable.org>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Russell King <linux@arm.linux.org.uk>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Link: http://lkml.kernel.org/r/20130209110031.GA17833@n2100.arm.linux.org.uk
Signed-off-by: H. Peter Anvin <hpa@zytor.com>
---
 arch/x86/include/asm/uaccess.h | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h
index 1e96326..a8d1265 100644
--- a/arch/x86/include/asm/uaccess.h
+++ b/arch/x86/include/asm/uaccess.h
@@ -168,31 +168,29 @@ do {						      \
 #define get_user(x, ptr)						\
 ({									\
 	int __ret_gu;							\
-	unsigned long __val_gu;						\
-	unsigned long long __val_gu8;					\
+	struct {							\
+		unsigned long long __val_n : 8*sizeof(*(ptr));		\
+	} __val_gu;							\
 	__chk_user_ptr(ptr);						\
 	might_fault();							\
 	switch (sizeof(*(ptr))) {					\
 	case 1:								\
-		__get_user_x(1, __ret_gu, __val_gu, ptr);		\
+		__get_user_x(1, __ret_gu, __val_gu.__val_n, ptr);	\
 		break;							\
 	case 2:								\
-		__get_user_x(2, __ret_gu, __val_gu, ptr);		\
+		__get_user_x(2, __ret_gu, __val_gu.__val_n, ptr);	\
 		break;							\
 	case 4:								\
-		__get_user_x(4, __ret_gu, __val_gu, ptr);		\
+		__get_user_x(4, __ret_gu, __val_gu.__val_n, ptr);	\
 		break;							\
 	case 8:								\
-		__get_user_8(__ret_gu, __val_gu8, ptr);			\
+		__get_user_8(__ret_gu, __val_gu.__val_n, ptr);		\
 		break;							\
 	default:							\
-		__get_user_x(X, __ret_gu, __val_gu, ptr);		\
+		__get_user_x(X, __ret_gu, __val_gu.__val_n, ptr);	\
 		break;							\
 	}								\
-	if (sizeof(*(ptr)) == 8)					\
-		(x) = (__typeof__(*(ptr)))__val_gu8;			\
-	else								\
-		(x) = (__typeof__(*(ptr)))__val_gu;			\
+	(x) = (__typeof__(*(ptr)))__val_gu.__val_n;			\
 	__ret_gu;							\
 })
 

Re: [tip:x86/mm] x86, mm: Use a bitfield to mask nuisance get_user() warnings

[Linus Torvalds]

On Mon, Feb 11, 2013 at 8:21 PM, H. Peter Anvin <hpa@zytor.com> wrote:
> On 02/11/2013 07:33 PM, Linus Torvalds wrote:
>
>> Has anybody run this past any gcc developers? And if so, did they run
>> away screaming?
>
> I haven't no... H.J., any comments on this patch?

I'd be most worried about any known pitfalls about bitfield code
generation. Looking at your code size numbers, it actually seems to
*improve* code generation except for the odd i386.pae case (bigger
code but also a different data size - odd) and i386 noconfig
(different bss, bigger code).

The code/data changes makes me wonder if the variable sometimes gets
flushed to memory as a 8-byte entry, and maybe there are things gcc
people can suggest..

But I don't see anything fundamentally wrong with it. Certainly it
looks much better than the disgusting and warning-prone

    unsigned long long __val_gu8

thing.

               Linus

Re: [tip:x86/mm] x86, mm: Use a bitfield to mask nuisance get_user() warnings

[Linus Torvalds]

On Mon, Feb 11, 2013 at 8:42 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> But I don't see anything fundamentally wrong with it. Certainly it
> looks much better than the disgusting and warning-prone
>
>     unsigned long long __val_gu8
>
> thing.

Oh. I just realized. That was your _baseline_ in the comparisons, wasn't it?

Can you please make the baseline be the current mainline git version
of <asm/uaccess.h>, not the first "unsigned long long __val_gu8"
version of the 64-bit get_user()?

Because we should compare against the straightforward code, not the
one that could have messed things up already..

              Linus

Re: [tip:x86/mm] x86, mm: Use a bitfield to mask nuisance get_user() warnings

[H. Peter Anvin]

On 02/11/2013 08:47 PM, Linus Torvalds wrote:
> On Mon, Feb 11, 2013 at 8:42 PM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
>>
>> But I don't see anything fundamentally wrong with it. Certainly it
>> looks much better than the disgusting and warning-prone
>>
>>      unsigned long long __val_gu8
>>
>> thing.
>
> Oh. I just realized. That was your _baseline_ in the comparisons, wasn't it?
>
> Can you please make the baseline be the current mainline git version
> of <asm/uaccess.h>, not the first "unsigned long long __val_gu8"
> version of the 64-bit get_user()?
>
> Because we should compare against the straightforward code, not the
> one that could have messed things up already..
>

No, the baseline was x86/mm before *any* of the 64-bit get_user() stuff 
were applied.

Very small differences can often be slight differences in strings (which 
end up in .rodata and thus count as text as far as size is concerned ... 
things like pathnames and dates.)  I am unclear about why the i386-pae 
build case stood out like that.

	-hpa


-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: [tip:x86/mm] x86, mm: Use a bitfield to mask nuisance get_user() warnings

[H. Peter Anvin]

Just to be sure, I re-ran the comparison using gcc 4.6.3 instead of gcc
4.7.2.  With gcc 4.6.3 I consistently get a few hundred bytes longer
with the bitfield variant than with the pre-get_user() baseline.

I looked at some of the diffed disassembly, and the differences seem to
be in the code generated downstream of __get_user_1 and __get_user_2,
which I guess is to be expected, mostly in the form of padding.

Annoyingly enough in *both* cases I found unnecessary instructions like:

+c12f6fbb:      0f b7 d2                movzwl %dx,%edx

	-hpa

Re: [tip:x86/mm] x86, mm: Use a bitfield to mask nuisance get_user() warnings

[Ingo Molnar]

* H. Peter Anvin <hpa@zytor.com> wrote:

> On 02/11/2013 07:33 PM, Linus Torvalds wrote:
> > On Mon, Feb 11, 2013 at 5:37 PM, tip-bot for H. Peter Anvin
> > <hpa@zytor.com> wrote:
> >>
> >> However, we can declare a bitfield using sizeof(), which is legal
> >> because sizeof() is a constant expression.  This quiets the warning,
> >> although the code generated isn't 100% identical from the baseline
> >> before 96477b4 x86-32: Add support for 64bit get_user():
> > 
> > Christ. This is so ugly that it's almost a work of art.
> 
> :)

A real possibility would be that if this ever breaks in a GCC 
version, or generates some really crappy code, we might get a 
"you got what you asked for" shrug from GCC developers?

It's not like we could start a revolution over that.

Thanks,

	Ingo

Re: [tip:x86/mm] x86, mm: Use a bitfield to mask nuisance get_user() warnings

[H.J. Lu]

On Mon, Feb 11, 2013 at 8:21 PM, H. Peter Anvin <hpa@zytor.com> wrote:
> On 02/11/2013 07:33 PM, Linus Torvalds wrote:
>> On Mon, Feb 11, 2013 at 5:37 PM, tip-bot for H. Peter Anvin
>> <hpa@zytor.com> wrote:
>>>
>>> However, we can declare a bitfield using sizeof(), which is legal
>>> because sizeof() is a constant expression.  This quiets the warning,
>>> although the code generated isn't 100% identical from the baseline
>>> before 96477b4 x86-32: Add support for 64bit get_user():
>>
>> Christ. This is so ugly that it's almost a work of art.
>
> :)
>
>> Has anybody run this past any gcc developers? And if so, did they run
>> away screaming?
>
> I haven't no... H.J., any comments on this patch?
>

Can you do something similar to what we did in glibc:

http://sourceware.org/git/?p=glibc.git;a=patch;h=c515fb5148f1d81d5f7736825e14c7502c15432a

-- 
H.J.

Re: [tip:x86/mm] x86, mm: Use a bitfield to mask nuisance get_user() warnings

[Linus Torvalds]

On Tue, Feb 12, 2013 at 8:38 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
>
> Can you do something similar to what we did in glibc:

No. Because we use macros to be type-independent (i e"get_user()"
works *regardless* of type), so casting to "uintptr_t" doesn't work.
It throws away the type information, and truncates 64-bit values on
32-bit architectures.

The whole point of the bitmask thing is that it doesn't have that
issue, and gets the size correct automatically. It's not pretty, but
it allows the rest of the sources to be readable.

                Linus

Re: [tip:x86/mm] x86, mm: Use a bitfield to mask nuisance get_user() warnings

[H. Peter Anvin]

On 02/12/2013 09:00 AM, Linus Torvalds wrote:
> On Tue, Feb 12, 2013 at 8:38 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>
>> Can you do something similar to what we did in glibc:
> 
> No. Because we use macros to be type-independent (i e"get_user()"
> works *regardless* of type), so casting to "uintptr_t" doesn't work.
> It throws away the type information, and truncates 64-bit values on
> 32-bit architectures.
> 
> The whole point of the bitmask thing is that it doesn't have that
> issue, and gets the size correct automatically. It's not pretty, but
> it allows the rest of the sources to be readable.
> 

No, I think what he is talking about it this bit:

/* 1 if 'type' is a pointer type, 0 otherwise.  */
# define __pointer_type(type) (__builtin_classify_type ((type) 0) == 5)

/* __intptr_t if P is true, or T if P is false.  */
# define __integer_if_pointer_type_sub(T, P) \
  __typeof__ (*(0 ? (__typeof__ (0 ? (T *) 0 : (void *) (P))) 0 \
		  : (__typeof__ (0 ? (__intptr_t *) 0 : (void *)(!(P)))) 0))

/* __intptr_t if EXPR has a pointer type, or the type of EXPR otherwise.  */
# define __integer_if_pointer_type(expr) \
  __integer_if_pointer_type_sub(__typeof__ ((__typeof__ (expr)) 0), \
				__pointer_type (__typeof__ (expr)))

/* Cast an integer or a pointer VAL to integer with proper type.  */
# define cast_to_integer(val) ((__integer_if_pointer_type (val)) (val))

Good grief, this makes the bitfield look like Mona Lisa.  On the other
hand, it relies on the *entirely* undocumented __builtin_classify_type()
-- there appears to be absolutely no reference to it in gcc documentation.

H.J., do you know what the bounds on the __builtin_classify_type() are
(gcc versions available and so on)?  Sadly I don't think one can use
__builtin_types_compatible_p() instead.

	-hpa

Re: [tip:x86/mm] x86, mm: Use a bitfield to mask nuisance get_user() warnings

[H.J. Lu]

On Tue, Feb 12, 2013 at 9:14 AM, H. Peter Anvin <hpa@zytor.com> wrote:
> On 02/12/2013 09:00 AM, Linus Torvalds wrote:
>> On Tue, Feb 12, 2013 at 8:38 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>>
>>> Can you do something similar to what we did in glibc:
>>
>> No. Because we use macros to be type-independent (i e"get_user()"
>> works *regardless* of type), so casting to "uintptr_t" doesn't work.
>> It throws away the type information, and truncates 64-bit values on
>> 32-bit architectures.
>>
>> The whole point of the bitmask thing is that it doesn't have that
>> issue, and gets the size correct automatically. It's not pretty, but
>> it allows the rest of the sources to be readable.
>>
>
> No, I think what he is talking about it this bit:
>
> /* 1 if 'type' is a pointer type, 0 otherwise.  */
> # define __pointer_type(type) (__builtin_classify_type ((type) 0) == 5)
>
> /* __intptr_t if P is true, or T if P is false.  */
> # define __integer_if_pointer_type_sub(T, P) \
>   __typeof__ (*(0 ? (__typeof__ (0 ? (T *) 0 : (void *) (P))) 0 \
>                   : (__typeof__ (0 ? (__intptr_t *) 0 : (void *)(!(P)))) 0))
>
> /* __intptr_t if EXPR has a pointer type, or the type of EXPR otherwise.  */
> # define __integer_if_pointer_type(expr) \
>   __integer_if_pointer_type_sub(__typeof__ ((__typeof__ (expr)) 0), \
>                                 __pointer_type (__typeof__ (expr)))
>
> /* Cast an integer or a pointer VAL to integer with proper type.  */
> # define cast_to_integer(val) ((__integer_if_pointer_type (val)) (val))
>
> Good grief, this makes the bitfield look like Mona Lisa.  On the other
> hand, it relies on the *entirely* undocumented __builtin_classify_type()
> -- there appears to be absolutely no reference to it in gcc documentation.
>
> H.J., do you know what the bounds on the __builtin_classify_type() are
> (gcc versions available and so on)?  Sadly I don't think one can use
> __builtin_types_compatible_p() instead.
>
>         -hpa
>

__builtin_classify_type is documented in "info gccint".  typeclass.h has

/* Values returned by __builtin_classify_type.  */

enum type_class
{
  no_type_class = -1,
  void_type_class, integer_type_class, char_type_class,
  enumeral_type_class, boolean_type_class,
  pointer_type_class, reference_type_class, offset_type_class,
  real_type_class, complex_type_class,
  function_type_class, method_type_class,
  record_type_class, union_type_class,
  array_type_class, string_type_class,
  lang_type_class
};

__builtin_classify_type ((type) 0) == 5 is compatible with all GCCs,
dating back as far as the first checkin when GCC was switched
to CVS from RCS in 1989, which predates the current Linux kernel:

commit 17cf341d65a3766f6451fe3bc20b9c010d38ba3b
Author: mycroft <mycroft@138bc75d-0d04-0410-961f-82ee72b054a4>
Date:   Sun Aug 13 19:24:27 1989 +0000

    entered into RCS


    git-svn-id: svn+ssh://gcc.gnu.org/svn/gcc/trunk@3
138bc75d-0d04-0410-961f-82ee72b054a4

diff --git a/gcc/typeclass.h b/gcc/typeclass.h
new file mode 100644
index 0000000..b166042
--- /dev/null
+++ b/gcc/typeclass.h
@@ -0,0 +1,14 @@
+/* Values returned by __builtin_classify_type.  */
+
+enum type_class
+{
+  no_type_class = -1,
+  void_type_class, integer_type_class, char_type_class,
+  enumeral_type_class, boolean_type_class,
+  pointer_type_class, reference_type_class, offset_type_class,
+  real_type_class, complex_type_class,
+  function_type_class, method_type_class,
+  record_type_class, union_type_class,
+  array_type_class, string_type_class, set_type_class, file_type_class,
+  lang_type_class
+};


-- 
H.J.

Re: [tip:x86/mm] x86, mm: Use a bitfield to mask nuisance get_user() warnings

[H. Peter Anvin]

[-- Attachment #1: Type: text/plain, Size: 310 bytes --]

I just thought up this variant, I'm about to test it, but H.J., do you
see any problems with it?

#define itype(x) \
__typeof__(__builtin_choose_expr(sizeof(*(x)) > sizeof(0UL), 0ULL, 0UL))

I tried it out with a small test program (attached), and it seems to
work.  Next for using it in the kernel...

	-hpa


[-- Attachment #2: testgcc.c --]
[-- Type: text/x-csrc, Size: 634 bytes --]

#include <stdio.h>
#include <stdlib.h>

#define itype(x) __typeof__(__builtin_choose_expr(sizeof(*(x)) > sizeof(0UL), 0ULL, 0UL))

int main(void)
{
  const char *a;
  const short *b;
  const int *c;
  const long *d;
  const long long *e;
  const void **p;

  itype(a) aa;
  itype(b) bb;
  itype(c) cc;
  itype(d) dd;
  itype(e) ee;
  itype(p) pp;

  aa = 1;
  bb = 2;
  cc = 3;
  dd = 4;
  ee = 5;
  pp = 6;

  printf("a = %zu\n", sizeof(aa));
  printf("b = %zu\n", sizeof(bb));
  printf("c = %zu\n", sizeof(cc));
  printf("d = %zu\n", sizeof(dd));
  printf("e = %zu\n", sizeof(ee));
  printf("p = %zu\n", sizeof(pp));

  return 0;
}

Re: [tip:x86/mm] x86, mm: Use a bitfield to mask nuisance get_user() warnings

[H.J. Lu]

On Tue, Feb 12, 2013 at 10:25 AM, H. Peter Anvin <hpa@zytor.com> wrote:
> I just thought up this variant, I'm about to test it, but H.J., do you
> see any problems with it?
>
> #define itype(x) \
> __typeof__(__builtin_choose_expr(sizeof(*(x)) > sizeof(0UL), 0ULL, 0UL))
>
> I tried it out with a small test program (attached), and it seems to
> work.  Next for using it in the kernel...
>
>         -hpa
>

It looks OK to me.

-- 
H.J.

Re: [tip:x86/mm] x86, mm: Use a bitfield to mask nuisance get_user() warnings

[Linus Torvalds]

On Tue, Feb 12, 2013 at 10:25 AM, H. Peter Anvin <hpa@zytor.com> wrote:
> I just thought up this variant, I'm about to test it, but H.J., do you
> see any problems with it?

Looks good to me. And we already use __builtin_choose_expr(), so it's
"portable". And it should avoid all the potential issues with
bitfields (rmk already pointed out how bitfields don't work well with
the ARM model, who knows what other pitfalls bitfield code generation
could have)

I wonder if we could/should eventually do some of the sizeof() in
generic code - not have these magic things duplicated in all the
architectures, just have the architectures specify the raw typed
details (__copy_to_user_4() etc). So cross-platform portability could
be a good thing. That's a separate discussion, though, and possibly
not worth it.

                      Linus

Re: [tip:x86/mm] x86, mm: Use a bitfield to mask nuisance get_user() warnings

[H. Peter Anvin]

On 02/12/2013 10:46 AM, Linus Torvalds wrote:
> On Tue, Feb 12, 2013 at 10:25 AM, H. Peter Anvin <hpa@zytor.com> wrote:
>> I just thought up this variant, I'm about to test it, but H.J., do you
>> see any problems with it?
>
> Looks good to me. And we already use __builtin_choose_expr(), so it's
> "portable". And it should avoid all the potential issues with
> bitfields (rmk already pointed out how bitfields don't work well with
> the ARM model, who knows what other pitfalls bitfield code generation
> could have)
>
> I wonder if we could/should eventually do some of the sizeof() in
> generic code - not have these magic things duplicated in all the
> architectures, just have the architectures specify the raw typed
> details (__copy_to_user_4() etc). So cross-platform portability could
> be a good thing. That's a separate discussion, though, and possibly
> not worth it.
>

I'm getting rid of the switch statement in the variant that I'm 
currently testing, so that would probably be undesirable.

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

[tip:x86/mm] x86, mm: Redesign get_user with a __builtin_choose_expr hack

[tip-bot for H. Peter Anvin]

Commit-ID:  3578baaed4613a9fc09bab9f79f6ce2ac682e8a3
Gitweb:     http://git.kernel.org/tip/3578baaed4613a9fc09bab9f79f6ce2ac682e8a3
Author:     H. Peter Anvin <hpa@linux.intel.com>
AuthorDate: Tue, 12 Feb 2013 11:47:31 -0800
Committer:  H. Peter Anvin <hpa@linux.intel.com>
CommitDate: Tue, 12 Feb 2013 12:46:40 -0800

x86, mm: Redesign get_user with a __builtin_choose_expr hack

Instead of using a bitfield, use an odd little trick using typeof,
__builtin_choose_expr, and sizeof.  __builtin_choose_expr is
explicitly defined to not convert its type (its argument is required
to be a constant expression) so this should be well-defined.

The code is still not 100% preturbation-free versus the baseline
before 64-bit get_user(), but the differences seem to be very small,
mostly related to padding and to gcc deciding when to spill registers.

Cc: Jamie Lokier <jamie@shareable.org>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Russell King <linux@arm.linux.org.uk>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: H. J. Lu <hjl.tools@gmail.com>
Link: http://lkml.kernel.org/r/511A8922.6050908@zytor.com
Signed-off-by: H. Peter Anvin <hpa@linux.intel.com>
---
 arch/x86/include/asm/uaccess.h | 57 +++++++++++-------------------------------
 1 file changed, 14 insertions(+), 43 deletions(-)

diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h
index a8d1265..d710a25 100644
--- a/arch/x86/include/asm/uaccess.h
+++ b/arch/x86/include/asm/uaccess.h
@@ -125,13 +125,12 @@ extern int __get_user_4(void);
 extern int __get_user_8(void);
 extern int __get_user_bad(void);
 
-#define __get_user_x(size, ret, x, ptr)		      \
-	asm volatile("call __get_user_" #size	      \
-		     : "=a" (ret), "=d" (x)	      \
-		     : "0" (ptr))		      \
-
-/* Careful: we have to cast the result to the type of the pointer
- * for sign reasons */
+/*
+ * This is a type: either unsigned long, if the argument fits into
+ * that type, or otherwise unsigned long long.
+ */
+#define __inttype(x) \
+__typeof__(__builtin_choose_expr(sizeof(x) > sizeof(0UL), 0ULL, 0UL))
 
 /**
  * get_user: - Get a simple variable from user space.
@@ -149,48 +148,20 @@ extern int __get_user_bad(void);
  *
  * Returns zero on success, or -EFAULT on error.
  * On error, the variable @x is set to zero.
+ *
+ * Careful: we have to cast the result to the type of the pointer
+ * for sign reasons.
  */
-#ifdef CONFIG_X86_32
-#define __get_user_8(ret, x, ptr)		      \
-do {						      \
-	register unsigned long long __xx asm("%edx"); \
-	asm volatile("call __get_user_8"	      \
-		     : "=a" (ret), "=r" (__xx)	      \
-		     : "0" (ptr));		      \
-	(x) = __xx;				      \
-} while (0)
-
-#else
-#define __get_user_8(__ret_gu, __val_gu, ptr)				\
-		__get_user_x(8, __ret_gu, __val_gu, ptr)
-#endif
-
 #define get_user(x, ptr)						\
 ({									\
 	int __ret_gu;							\
-	struct {							\
-		unsigned long long __val_n : 8*sizeof(*(ptr));		\
-	} __val_gu;							\
+	register __inttype(*(ptr)) __val_gu asm("%edx");		\
 	__chk_user_ptr(ptr);						\
 	might_fault();							\
-	switch (sizeof(*(ptr))) {					\
-	case 1:								\
-		__get_user_x(1, __ret_gu, __val_gu.__val_n, ptr);	\
-		break;							\
-	case 2:								\
-		__get_user_x(2, __ret_gu, __val_gu.__val_n, ptr);	\
-		break;							\
-	case 4:								\
-		__get_user_x(4, __ret_gu, __val_gu.__val_n, ptr);	\
-		break;							\
-	case 8:								\
-		__get_user_8(__ret_gu, __val_gu.__val_n, ptr);		\
-		break;							\
-	default:							\
-		__get_user_x(X, __ret_gu, __val_gu.__val_n, ptr);	\
-		break;							\
-	}								\
-	(x) = (__typeof__(*(ptr)))__val_gu.__val_n;			\
+	asm volatile("call __get_user_%P3"				\
+		     : "=a" (__ret_gu), "=r" (__val_gu)			\
+		     : "0" (ptr), "i" (sizeof(*(ptr))));		\
+	(x) = (__typeof__(*(ptr))) __val_gu;				\
 	__ret_gu;							\
 })
 

Re: [tip:x86/mm] x86, mm: Redesign get_user with a __builtin_choose_expr hack

[Linus Torvalds]

So this looks clean, but I noticed something (that was true even of
the old 64-bit accesses)

On Tue, Feb 12, 2013 at 12:55 PM, tip-bot for H. Peter Anvin
<hpa@linux.intel.com> wrote:
> +       register __inttype(*(ptr)) __val_gu asm("%edx");                \

How does gcc even alllow this?

On x86-32, you cannot put a 64-bit value in %edx.

Where do the upper bits go? It clearly cannot be %edx:%eax, since we
put the error value in %eax.

So is the rule for x86-32 that naming "long long" register values
names the first register, and the high bits go into the next one (I
forget the crazy register numbering, I assume it's %ecx). Or what?
This should have a comment.

Also, come to think of it, we have tried the "named register
variables" thing before, and it has resulted in problems with scope.
In particular, two variables within the same scope and the same
register have been problematic. And it *does* happen, when you have
things like

   /* copy_user */
   put_user(get_user(.., addr), addr2);

and then things go downhill.

Maybe we do not have these issues, but there are good reasons why
we've tried very hard on x86 to avoid named register variables.

(I realize that they happen, and some other architectures don't even
have good support for naming registers any other way so they are way
more common there, so I probably worry needlessly, but it does worry
me).

             Linus

Re: [tip:x86/mm] x86, mm: Redesign get_user with a __builtin_choose_expr hack

[H. Peter Anvin]

On 02/12/2013 03:06 PM, Linus Torvalds wrote:
> So this looks clean, but I noticed something (that was true even of
> the old 64-bit accesses)
> 
> On Tue, Feb 12, 2013 at 12:55 PM, tip-bot for H. Peter Anvin
> <hpa@linux.intel.com> wrote:
>> +       register __inttype(*(ptr)) __val_gu asm("%edx");                \
> 
> How does gcc even alllow this?
> 
> On x86-32, you cannot put a 64-bit value in %edx.
> 
> Where do the upper bits go? It clearly cannot be %edx:%eax, since we
> put the error value in %eax.
> 
> So is the rule for x86-32 that naming "long long" register values
> names the first register, and the high bits go into the next one (I
> forget the crazy register numbering, I assume it's %ecx). Or what?
> This should have a comment.
> 

Yes, it goes into the next register in gcc's register numbering, which
is %ecx.  This works with the register variable because the named
register is treated as a starting point, whereas using "=d" is treated
as a singleton register set.

I'll add a comment.

gcc's register numbering isn't all that crazy, incidentally: the only
difference from the standard x86 register numbering is that %ecx and
%edx is swapped, so that the standard %edx:%eax and %ebx:%ecx register
pairs end up consecutive.  It isn't really gcc's fault that the x86
register numbering doesn't match its (hard-coded!) register conventions...

> Also, come to think of it, we have tried the "named register
> variables" thing before, and it has resulted in problems with scope.
> In particular, two variables within the same scope and the same
> register have been problematic. And it *does* happen, when you have
> things like
> 
>    /* copy_user */
>    put_user(get_user(.., addr), addr2);
> 
> and then things go downhill.
> 
> Maybe we do not have these issues, but there are good reasons why
> we've tried very hard on x86 to avoid named register variables.

Yes, but there doesn't seem to be any other way to do this.  gcc won't
even allow "=cd" even if we know the variable is 64 bits, even though
"=A" is documented to be equivalent to "=da".

I don't think we have any additional problem here,though.  If we are
inside a scope with "%edx" as a named register variable *and* that
variable is live at the point get_user() happens, then yes, we can and
will have a problem, regardless if we use "=d" or a named register
variable.  The only solution to that is to keep the named register
variable live for as short time as possible.

If we do run into trouble, we could introduce a second copy, thus
reducing the lifespan of the named variable to the absolute minimum:

	register __inttype(*(ptr)) __val_gu asm("%edx");
	__inttype(*(ptr)) __val_gv;

	asm volatile(...);

	__val_gv = __val_gu;
	(x) = (__typeof__(*(ptr))) __val_gv;

That way if the evaluation of (x) as an lvalue somehow requires specific
registers they don't collide.

I would prefer if we could worry about that when we actually need to,
though.  It will trigger a compile error if relevant, so it shouldn't
cause any risk of silent corruption.

> (I realize that they happen, and some other architectures don't even
> have good support for naming registers any other way so they are way
> more common there, so I probably worry needlessly, but it does worry
> me).

Let me know what you think.

	-hpa

Re: [tip:x86/mm] x86, mm: Redesign get_user with a __builtin_choose_expr hack

[Linus Torvalds]

On Tue, Feb 12, 2013 at 3:19 PM, H. Peter Anvin <hpa@zytor.com> wrote:
>
> Yes, but there doesn't seem to be any other way to do this.  gcc won't
> even allow "=cd" even if we know the variable is 64 bits, even though
> "=A" is documented to be equivalent to "=da".

No, "=da" means value "in edx _or_ %eax". Not the same as "A".

But you're right, there's nothing similar for %ebx:%ecx. I thought
there was. I was really sure we did something special for 64-bit adc
etc.

> Let me know what you think.

I guess we don't have any choice. And the other cleanups certainly look good.

              Linus

Re: [tip:x86/mm] x86, mm: Redesign get_user with a __builtin_choose_expr hack

[H. Peter Anvin]

On 02/12/2013 03:49 PM, Linus Torvalds wrote:
> On Tue, Feb 12, 2013 at 3:19 PM, H. Peter Anvin <hpa@zytor.com> wrote:
>>
>> Yes, but there doesn't seem to be any other way to do this.  gcc won't
>> even allow "=cd" even if we know the variable is 64 bits, even though
>> "=A" is documented to be equivalent to "=da".
> 
> No, "=da" means value "in edx _or_ %eax". Not the same as "A".
> 

Actually, if you look at how gcc implements them, they are the same, and
if you are luckless enough to try to use a 32-bit value with an "A"
constraint you have it end up in either %eax or %edx.

However, they seem to have added some additional linting which prohibits
the compound form.  I'm not sure it would have worked anyway since we
need the two-register bit to be conditional.

> But you're right, there's nothing similar for %ebx:%ecx. I thought
> there was. I was really sure we did something special for 64-bit adc
> etc.
> 
>> Let me know what you think.
> 
> I guess we don't have any choice. And the other cleanups certainly look good.

OK, will commit the comment.  We can add the additional copy if we need it.

	-hpa

[tip:x86/mm] x86, doc: Clarify the use of asm("%edx") in uaccess.h

[tip-bot for H. Peter Anvin]

Commit-ID:  ff52c3b02b3f73178bfe0c219cd22abdcb0e46c3
Gitweb:     http://git.kernel.org/tip/ff52c3b02b3f73178bfe0c219cd22abdcb0e46c3
Author:     H. Peter Anvin <hpa@linux.intel.com>
AuthorDate: Tue, 12 Feb 2013 15:37:02 -0800
Committer:  H. Peter Anvin <hpa@linux.intel.com>
CommitDate: Tue, 12 Feb 2013 15:37:02 -0800

x86, doc: Clarify the use of asm("%edx") in uaccess.h

Put in a comment that explains that the use of asm("%edx") in
uaccess.h doesn't actually necessarily mean %edx alone.

Cc: Jamie Lokier <jamie@shareable.org>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Russell King <linux@arm.linux.org.uk>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: H. J. Lu <hjl.tools@gmail.com>
Link: http://lkml.kernel.org/r/511ACDFB.1050707@zytor.com
Signed-off-by: H. Peter Anvin <hpa@linux.intel.com>
---
 arch/x86/include/asm/uaccess.h | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h
index d710a25..5ee2687 100644
--- a/arch/x86/include/asm/uaccess.h
+++ b/arch/x86/include/asm/uaccess.h
@@ -148,9 +148,16 @@ __typeof__(__builtin_choose_expr(sizeof(x) > sizeof(0UL), 0ULL, 0UL))
  *
  * Returns zero on success, or -EFAULT on error.
  * On error, the variable @x is set to zero.
- *
+ */
+/*
  * Careful: we have to cast the result to the type of the pointer
  * for sign reasons.
+ *
+ * The use of %edx as the register specifier is a bit of a
+ * simplification, as gcc only cares about it as the starting point
+ * and not size: for a 64-bit value it will use %ecx:%edx on 32 bits
+ * (%ecx being the next register in gcc's x86 register sequence), and
+ * %rdx on 64 bits.
  */
 #define get_user(x, ptr)						\
 ({									\

Re: [tip:x86/mm] x86, mm: Redesign get_user with a __builtin_choose_expr hack

[Russell King - ARM Linux]

On Tue, Feb 12, 2013 at 03:06:51PM -0800, Linus Torvalds wrote:
> So this looks clean, but I noticed something (that was true even of
> the old 64-bit accesses)
> 
> On Tue, Feb 12, 2013 at 12:55 PM, tip-bot for H. Peter Anvin
> <hpa@linux.intel.com> wrote:
> > +       register __inttype(*(ptr)) __val_gu asm("%edx");                \
> 
> How does gcc even alllow this?
> 
> On x86-32, you cannot put a 64-bit value in %edx.
> 
> Where do the upper bits go? It clearly cannot be %edx:%eax, since we
> put the error value in %eax.

I can't talk for x86, but the ARM version is this:

                register __inttype(*p) __r2 asm("r2");

r2 is only 32-bit, so you can ask the same question there.  The answer
is, it ends up in r3, the next register, as required by the compiler
ABI.  (64-bit values are always contained in a consecutive even, odd
register pair.)

> Also, come to think of it, we have tried the "named register
> variables" thing before, and it has resulted in problems with scope.
> In particular, two variables within the same scope and the same
> register have been problematic. And it *does* happen, when you have
> things like
> 
>    /* copy_user */
>    put_user(get_user(.., addr), addr2);
> 
> and then things go downhill.

Umm.  Why would you want to put_user() the error code from get_user() ?
That's just weird, don't we normally want to return the error code?

But yes, I can see that kind of thing causing problems - I'll check
what the behaviour is on ARM.  What we _do_ have is a bit of assembly
level checking which verifies that the arguments are passed in the
correct registers, so at least we get build errors should things go
wrong.  We haven't seen that though.

> Maybe we do not have these issues, but there are good reasons why
> we've tried very hard on x86 to avoid named register variables.

Unfortunately on ARM there's no other way to get close to specifying
registers for asm code. :(

Re: [tip:x86/mm] x86, mm: Use a bitfield to mask nuisance get_user() warnings

[Linus Torvalds]

On Tue, Feb 12, 2013 at 9:14 AM, H. Peter Anvin <hpa@zytor.com> wrote:
>
> No, I think what he is talking about it this bit:

Ok, I agree that the bitfield code actually looks cleaner.

That said, maybe gcc has an easier time using a few odd builtins and
magic typeof's. But at least the bitfield trick looks half-way
portable..

                 Linus

Re: [tip:x86/mm] x86, mm: Use a bitfield to mask nuisance get_user() warnings

[H. Peter Anvin]

On 02/12/2013 09:32 AM, Linus Torvalds wrote:
> On Tue, Feb 12, 2013 at 9:14 AM, H. Peter Anvin <hpa@zytor.com> wrote:
>>
>> No, I think what he is talking about it this bit:
>
> Ok, I agree that the bitfield code actually looks cleaner.
>
> That said, maybe gcc has an easier time using a few odd builtins and
> magic typeof's. But at least the bitfield trick looks half-way
> portable..
>

On the other hand, it still uses two gcc extensions: long long bitfields 
and typeof.

I'll see what kind of code we get with the macro.

	-hpa


-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: [tip:x86/mm] x86, mm: Use a bitfield to mask nuisance get_user() warnings

[Linus Torvalds]

On Tue, Feb 12, 2013 at 9:35 AM, H. Peter Anvin <hpa@zytor.com> wrote:
>
> On the other hand, it still uses two gcc extensions: long long bitfields and
> typeof.
>
> I'll see what kind of code we get with the macro.

At least one thing to look out for is the poor LLVM people who are
trying to make the kernel compile with that compiler.. We shouldn't
make it arbitrarily harder for them, so *some* level of portability is
a good idea.

Then there is icc, but I don't know how relevant that would ever be.
At least LLVM has the potential to be widely available.

Of course, they may both already support even the odd gcc builtins -
we already use a lot of the more straightforward ones...

            Linus

Re: [tip:x86/mm] x86, mm: Use a bitfield to mask nuisance get_user() warnings

[Russell King - ARM Linux]

On Tue, Feb 12, 2013 at 09:32:54AM -0800, Linus Torvalds wrote:
> On Tue, Feb 12, 2013 at 9:14 AM, H. Peter Anvin <hpa@zytor.com> wrote:
> >
> > No, I think what he is talking about it this bit:
> 
> Ok, I agree that the bitfield code actually looks cleaner.
> 
> That said, maybe gcc has an easier time using a few odd builtins and
> magic typeof's. But at least the bitfield trick looks half-way
> portable..

I've just been trying hpa's solution on ARM, and I can't get it to work,
because the compiler refuses to put the struct { unsigned long long ... }
into the register(s) we need for the out of line assembly:

#define get_user(x,p)							\
	({								\
		register const typeof(*(p)) __user *__p asm("r0") = (p);\
		register int __e asm("r0");				\
		register struct {					\
			unsigned long long __r2:8 * sizeof(*(__p));	\
		} __v asm("r2");					\
		switch (sizeof(*(__p))) {				\
		case 1: 						\
			__get_user_x(__v.__r2, __p, __e, 1, "lr");	\
			break;						\
		case 2: 						\
			__get_user_x(__v.__r2, __p, __e, 2, "r3", "lr");\
			break;						\
		case 4: 						\
			__get_user_x(__v.__r2, __p, __e, 4, "lr");	\
			break;						\
		case 8: 						\
			__get_user_x(__v.__r2, __p, __e, 8, "lr");	\
			break;						\
		default: __e = __get_user_bad(); break; 		\
		}							\
		x = (typeof(*(__p))) __v.__r2;				\
		__e;							\
	})

This ends up with __v.__r2 ending up in r1/(r2) not r2/(r3).

However, I do have a working solution for 32-bit ARM which seems to work
fine with my test cases here, though as I mentioned to hpa, it may not
be portable to other 32-bit architectures:

#ifdef BIG_ENDIAN
#define __get_user_xb(__r2,__p,__e,__s,__i...)				\
	__get_user_x(__r2,(unsigned)__p+4,__e,__s,__i)
#else
#define __get_user_xb __get_user_x
#endif

#define get_user(x,p)							\
	({								\
		register const typeof(*(p)) __user *__p asm("r0") = (p);\
		register int __e asm("r0");				\
		register typeof(x) __r2 asm("r2");			\
		switch (sizeof(*(__p))) {				\
		case 1: 						\
			__get_user_x(__r2, __p, __e, 1, "lr");		\
			break;						\
		case 2: 						\
			__get_user_x(__r2, __p, __e, 2, "r3", "lr");	\
			break;						\
		case 4: 						\
			__get_user_x(__r2, __p, __e, 4, "lr");		\
			break;						\
		case 8: 						\
			{						\
			if (sizeof((x)) < 8)				\
				__get_user_xb(__r2, __p, __e, 4, "lr"); \
			else						\
				__get_user_x(__r2, __p, __e, 8, "lr");	\
			}						\
			break;						\
		default: __e = __get_user_bad(); break; 		\
		}							\
		x = (typeof(*(__p))) __r2;				\
		__e;							\
	})

It's risky because it relies upon a "register" being allocated as 32-bits
even if typeof(x) is 8-bit or 16-bit.