[PATCH] Introduce __fill_user() and kill __bzero()

[PATCH] Introduce __fill_user() and kill __bzero()

[Franck Bui-Huu]

Currently memset() is used to fill a user space area (clear_user) or
kernel one (memset). These two functions don't have the same
prototype, the former returning the number of bytes not copied and the
latter returning the start address of the area to clear. This forces
memset() to actually returns two values in an unconventional way ie
the number of bytes not copied is given by $a2. Therefore clear_user()
needs to call memset() using inline assembly.

Instead this patch creates __fill_user() which is the same as memset()
except it always returns the number of bytes not copied. This simplify
clear_user() and makes its definition saner.

Also an out of line version of memset is given because gcc generates
some calls to it since builtin functions have been disabled. It allows
assembly code to call it too.

Eventually __bzero() has been removed because it's not part of the
Linux uaccess API. And the nano-optimization it brings is not
worthing.

Signed-off-by: Franck Bui-Huu <fbuihuu@gmail.com>
---
 arch/mips/kernel/mips_ksyms.c |    3 +-
 arch/mips/lib/csum_partial.S  |    2 +-
 arch/mips/lib/memcpy.S        |    2 +-
 arch/mips/lib/memset.S        |   49 ++++++++++++++++++++++++++--------------
 include/asm-mips/string.h     |    7 +++++-
 include/asm-mips/uaccess.h    |   17 ++-----------
 6 files changed, 44 insertions(+), 36 deletions(-)

diff --git a/arch/mips/kernel/mips_ksyms.c b/arch/mips/kernel/mips_ksyms.c
index 225755d..a801e09 100644
--- a/arch/mips/kernel/mips_ksyms.c
+++ b/arch/mips/kernel/mips_ksyms.c
@@ -14,7 +14,6 @@
 #include <asm/pgtable.h>
 #include <asm/uaccess.h>
 
-extern void *__bzero(void *__s, size_t __count);
 extern long __strncpy_from_user_nocheck_asm(char *__to,
                                             const char *__from, long __len);
 extern long __strncpy_from_user_asm(char *__to, const char *__from,
@@ -36,9 +35,9 @@ EXPORT_SYMBOL(kernel_thread);
 /*
  * Userspace access stuff.
  */
+EXPORT_SYMBOL(__fill_user);
 EXPORT_SYMBOL(__copy_user);
 EXPORT_SYMBOL(__copy_user_inatomic);
-EXPORT_SYMBOL(__bzero);
 EXPORT_SYMBOL(__strncpy_from_user_nocheck_asm);
 EXPORT_SYMBOL(__strncpy_from_user_asm);
 EXPORT_SYMBOL(__strlen_user_nocheck_asm);
diff --git a/arch/mips/lib/csum_partial.S b/arch/mips/lib/csum_partial.S
index c0a77fe..8d3fa1e 100644
--- a/arch/mips/lib/csum_partial.S
+++ b/arch/mips/lib/csum_partial.S
@@ -694,7 +694,7 @@ l_exc:
 	ADD	dst, t0			# compute start address in a1
 	SUB	dst, src
 	/*
-	 * Clear len bytes starting at dst.  Can't call __bzero because it
+	 * Clear len bytes starting at dst.  Can't call memset because it
 	 * might modify len.  An inefficient loop for these rare times...
 	 */
 	beqz	len, done
diff --git a/arch/mips/lib/memcpy.S b/arch/mips/lib/memcpy.S
index a526c62..425f2c3 100644
--- a/arch/mips/lib/memcpy.S
+++ b/arch/mips/lib/memcpy.S
@@ -443,7 +443,7 @@ l_exc:
 	ADD	dst, t0			# compute start address in a1
 	SUB	dst, src
 	/*
-	 * Clear len bytes starting at dst.  Can't call __bzero because it
+	 * Clear len bytes starting at dst.  Can't call memset because it
 	 * might modify len.  An inefficient loop for these rare times...
 	 */
 	beqz	len, done
diff --git a/arch/mips/lib/memset.S b/arch/mips/lib/memset.S
index 3f8b8b3..cb6b83d 100644
--- a/arch/mips/lib/memset.S
+++ b/arch/mips/lib/memset.S
@@ -46,17 +46,34 @@
 	.endm
 
 /*
- * memset(void *s, int c, size_t n)
+ * An outline version of memset, which should be used either by gcc or
+ * by assembly code.
+ */
+NESTED(memset, 24, ra)
+	PTR_ADDU	sp, sp, -24
+	LONG_S		a0, 16(sp)
+	LONG_S		ra, 20(sp)
+	jal		__fill_user
+	LONG_L		v0, 16(sp)
+	LONG_L		ra, 20(sp)
+	PTR_ADDU	sp, sp, 24
+	jr		ra
+END(memset)
+
+/*
+ * __kernel_size_t __fill_user(void __user *s, long c, __kernel_size_t n)
  *
  * a0: start of area to clear
  * a1: char to fill with
  * a2: size of area to clear
+ *
+ * Returns the number of bytes NOT set or 0 on success.
  */
 	.set	noreorder
 	.align	5
-LEAF(memset)
+LEAF(__fill_user)
 	beqz		a1, 1f
-	 move		v0, a0			/* result */
+	 move		v0, zero		/* result */
 
 	andi		a1, 0xff		/* spread fillword */
 	LONG_SLL		t1, a1, 8
@@ -68,8 +85,6 @@ LEAF(memset)
 #endif
 	or		a1, t1
 1:
-
-FEXPORT(__bzero)
 	sltiu		t0, a2, LONGSIZE	/* very small region? */
 	bnez		t0, small_memset
 	 andi		t0, a0, LONGMASK	/* aligned? */
@@ -127,7 +142,7 @@ memset_partial:
 	EX(LONG_S_L, a1, -1(a0), last_fixup)
 #endif
 1:	jr		ra
-	 move		a2, zero
+	 nop
 
 small_memset:
 	beqz		a2, 2f
@@ -138,29 +153,29 @@ small_memset:
 	 sb		a1, -1(a0)
 
 2:	jr		ra			/* done */
-	 move		a2, zero
-	END(memset)
+	 nop
+END(__fill_user)
 
 first_fixup:
-	jr	ra
-	 nop
+	jr		ra
+	 move		v0, a2
 
 fwd_fixup:
 	PTR_L		t0, TI_TASK($28)
 	LONG_L		t0, THREAD_BUADDR(t0)
-	andi		a2, 0x3f
-	LONG_ADDU	a2, t1
+	andi		v0, a2, 0x3f
+	LONG_ADDU	v0, t1
 	jr		ra
-	 LONG_SUBU	a2, t0
+	 LONG_SUBU	v0, t0
 
 partial_fixup:
 	PTR_L		t0, TI_TASK($28)
 	LONG_L		t0, THREAD_BUADDR(t0)
-	andi		a2, LONGMASK
-	LONG_ADDU	a2, t1
+	andi		v0, a2, LONGMASK
+	LONG_ADDU	v0, t1
 	jr		ra
-	 LONG_SUBU	a2, t0
+	 LONG_SUBU	v0, t0
 
 last_fixup:
 	jr		ra
-	 andi		v1, a2, LONGMASK
+	 andi		v0, a2, LONGMASK
diff --git a/include/asm-mips/string.h b/include/asm-mips/string.h
index 436e3ad..2bba927 100644
--- a/include/asm-mips/string.h
+++ b/include/asm-mips/string.h
@@ -10,6 +10,7 @@
 #ifndef _ASM_STRING_H
 #define _ASM_STRING_H
 
+#include <asm/uaccess.h>	/* __fill_user() */
 
 /*
  * Most of the inline functions are rather naive implementations so I just
@@ -132,7 +133,11 @@ strncmp(__const__ char *__cs, __const__ char *__ct, size_t __count)
 #endif /* CONFIG_32BIT */
 
 #define __HAVE_ARCH_MEMSET
-extern void *memset(void *__s, int __c, size_t __count);
+extern inline void *memset(void *s, int c, size_t count)
+{
+	__fill_user(s, c, count);
+	return s;
+}
 
 #define __HAVE_ARCH_MEMCPY
 extern void *memcpy(void *__to, __const__ void *__from, size_t __n);
diff --git a/include/asm-mips/uaccess.h b/include/asm-mips/uaccess.h
index c30c718..8c0d226 100644
--- a/include/asm-mips/uaccess.h
+++ b/include/asm-mips/uaccess.h
@@ -11,7 +11,6 @@
 
 #include <linux/kernel.h>
 #include <linux/errno.h>
-#include <linux/thread_info.h>
 #include <asm-generic/uaccess.h>
 
 /*
@@ -633,23 +632,13 @@ extern size_t __copy_user_inatomic(void *__to, const void *__from, size_t __n);
  * Returns number of bytes that could not be cleared.
  * On success, this will be zero.
  */
+extern __kernel_size_t __fill_user(void __user *s, long c, __kernel_size_t n);
+
 static inline __kernel_size_t
 __clear_user(void __user *addr, __kernel_size_t size)
 {
-	__kernel_size_t res;
-
 	might_sleep();
-	__asm__ __volatile__(
-		"move\t$4, %1\n\t"
-		"move\t$5, $0\n\t"
-		"move\t$6, %2\n\t"
-		__MODULE_JAL(__bzero)
-		"move\t%0, $6"
-		: "=r" (res)
-		: "r" (addr), "r" (size)
-		: "$4", "$5", "$6", __UA_t0, __UA_t1, "$31");
-
-	return res;
+	return __fill_user(addr, 0, size);
 }
 
 #define clear_user(addr,n)						\
-- 
1.5.3.4

Re: [PATCH] Introduce __fill_user() and kill __bzero()

[Thiemo Seufer]

Franck Bui-Huu wrote:
> Currently memset() is used to fill a user space area (clear_user) or
> kernel one (memset). These two functions don't have the same
> prototype, the former returning the number of bytes not copied and the
> latter returning the start address of the area to clear. This forces
> memset() to actually returns two values in an unconventional way ie
> the number of bytes not copied is given by $a2. Therefore clear_user()
> needs to call memset() using inline assembly.
> 
> Instead this patch creates __fill_user() which is the same as memset()
> except it always returns the number of bytes not copied. This simplify
> clear_user() and makes its definition saner.
> 
> Also an out of line version of memset is given because gcc generates
> some calls to it since builtin functions have been disabled. It allows
> assembly code to call it too.
> 
> Eventually __bzero() has been removed because it's not part of the
> Linux uaccess API. And the nano-optimization it brings is not
> worthing.
> 
> Signed-off-by: Franck Bui-Huu <fbuihuu@gmail.com>
> ---
>  arch/mips/kernel/mips_ksyms.c |    3 +-
>  arch/mips/lib/csum_partial.S  |    2 +-
>  arch/mips/lib/memcpy.S        |    2 +-
>  arch/mips/lib/memset.S        |   49 ++++++++++++++++++++++++++--------------
>  include/asm-mips/string.h     |    7 +++++-
>  include/asm-mips/uaccess.h    |   17 ++-----------
>  6 files changed, 44 insertions(+), 36 deletions(-)
> 
> diff --git a/arch/mips/kernel/mips_ksyms.c b/arch/mips/kernel/mips_ksyms.c
> index 225755d..a801e09 100644
> --- a/arch/mips/kernel/mips_ksyms.c
> +++ b/arch/mips/kernel/mips_ksyms.c
> @@ -14,7 +14,6 @@
>  #include <asm/pgtable.h>
>  #include <asm/uaccess.h>
>  
> -extern void *__bzero(void *__s, size_t __count);
>  extern long __strncpy_from_user_nocheck_asm(char *__to,
>                                              const char *__from, long __len);
>  extern long __strncpy_from_user_asm(char *__to, const char *__from,
> @@ -36,9 +35,9 @@ EXPORT_SYMBOL(kernel_thread);
>  /*
>   * Userspace access stuff.
>   */
> +EXPORT_SYMBOL(__fill_user);
>  EXPORT_SYMBOL(__copy_user);
>  EXPORT_SYMBOL(__copy_user_inatomic);
> -EXPORT_SYMBOL(__bzero);
>  EXPORT_SYMBOL(__strncpy_from_user_nocheck_asm);
>  EXPORT_SYMBOL(__strncpy_from_user_asm);
>  EXPORT_SYMBOL(__strlen_user_nocheck_asm);
> diff --git a/arch/mips/lib/csum_partial.S b/arch/mips/lib/csum_partial.S
> index c0a77fe..8d3fa1e 100644
> --- a/arch/mips/lib/csum_partial.S
> +++ b/arch/mips/lib/csum_partial.S
> @@ -694,7 +694,7 @@ l_exc:
>  	ADD	dst, t0			# compute start address in a1
>  	SUB	dst, src
>  	/*
> -	 * Clear len bytes starting at dst.  Can't call __bzero because it
> +	 * Clear len bytes starting at dst.  Can't call memset because it
>  	 * might modify len.  An inefficient loop for these rare times...
>  	 */
>  	beqz	len, done
> diff --git a/arch/mips/lib/memcpy.S b/arch/mips/lib/memcpy.S
> index a526c62..425f2c3 100644
> --- a/arch/mips/lib/memcpy.S
> +++ b/arch/mips/lib/memcpy.S
> @@ -443,7 +443,7 @@ l_exc:
>  	ADD	dst, t0			# compute start address in a1
>  	SUB	dst, src
>  	/*
> -	 * Clear len bytes starting at dst.  Can't call __bzero because it
> +	 * Clear len bytes starting at dst.  Can't call memset because it
>  	 * might modify len.  An inefficient loop for these rare times...
>  	 */
>  	beqz	len, done
> diff --git a/arch/mips/lib/memset.S b/arch/mips/lib/memset.S
> index 3f8b8b3..cb6b83d 100644
> --- a/arch/mips/lib/memset.S
> +++ b/arch/mips/lib/memset.S
> @@ -46,17 +46,34 @@
>  	.endm
>  
>  /*
> - * memset(void *s, int c, size_t n)
> + * An outline version of memset, which should be used either by gcc or
> + * by assembly code.
> + */
> +NESTED(memset, 24, ra)
> +	PTR_ADDU	sp, sp, -24
> +	LONG_S		a0, 16(sp)
> +	LONG_S		ra, 20(sp)
> +	jal		__fill_user
> +	LONG_L		v0, 16(sp)
> +	LONG_L		ra, 20(sp)
> +	PTR_ADDU	sp, sp, 24
> +	jr		ra
> +END(memset)

This will break on 64bit kernels.


Thiemo

Re: [PATCH] Introduce __fill_user() and kill __bzero()

[Franck Bui-Huu]

Thiemo Seufer wrote:
>>  /*
>> - * memset(void *s, int c, size_t n)
>> + * An outline version of memset, which should be used either by gcc or
>> + * by assembly code.
>> + */
>> +NESTED(memset, 24, ra)
>> +	PTR_ADDU	sp, sp, -24
>> +	LONG_S		a0, 16(sp)
>> +	LONG_S		ra, 20(sp)
>> +	jal		__fill_user
>> +	LONG_L		v0, 16(sp)
>> +	LONG_L		ra, 20(sp)
>> +	PTR_ADDU	sp, sp, 24
>> +	jr		ra
>> +END(memset)
> 
> This will break on 64bit kernels.
> 

Obviously...

Looks like I should find a good place to implement it in C... or do
you know a sane way (without too many #ifdef) to do that in assembly
code ?

thanks,
		Franck

Re: [PATCH] Introduce __fill_user() and kill __bzero()

[Franck Bui-Huu]

Thiemo Seufer wrote:
> Franck Bui-Huu wrote:
>>  /*
>> - * memset(void *s, int c, size_t n)
>> + * An outline version of memset, which should be used either by gcc or
>> + * by assembly code.
>> + */
>> +NESTED(memset, 24, ra)
>> +	PTR_ADDU	sp, sp, -24
>> +	LONG_S		a0, 16(sp)
>> +	LONG_S		ra, 20(sp)
>> +	jal		__fill_user
>> +	LONG_L		v0, 16(sp)
>> +	LONG_L		ra, 20(sp)
>> +	PTR_ADDU	sp, sp, 24
>> +	jr		ra
>> +END(memset)
> 
> This will break on 64bit kernels.
> 

Is the following correct ?

NESTED(memset, 16, ra)
        PTR_ADDU        sp, sp, -16
        LONG_S          a0,  8(sp)
        LONG_S          ra, 16(sp)
        jal             __fill_user
        LONG_L          v0,  8(sp)
        LONG_L          ra, 16(sp)
        PTR_ADDU        sp, sp, 16
        jr              ra
END(memset)

I know it doesn't respect any mips ABI but in this case do
we really care ?

thanks.

		Franck

Re: [PATCH] Introduce __fill_user() and kill __bzero()

[Thiemo Seufer]

Franck Bui-Huu wrote:
> Thiemo Seufer wrote:
> > Franck Bui-Huu wrote:
> >>  /*
> >> - * memset(void *s, int c, size_t n)
> >> + * An outline version of memset, which should be used either by gcc or
> >> + * by assembly code.
> >> + */
> >> +NESTED(memset, 24, ra)
> >> +	PTR_ADDU	sp, sp, -24
> >> +	LONG_S		a0, 16(sp)
> >> +	LONG_S		ra, 20(sp)
> >> +	jal		__fill_user
> >> +	LONG_L		v0, 16(sp)
> >> +	LONG_L		ra, 20(sp)
> >> +	PTR_ADDU	sp, sp, 24
> >> +	jr		ra
> >> +END(memset)
> > 
> > This will break on 64bit kernels.
> > 
> 
> Is the following correct ?
> 
> NESTED(memset, 16, ra)
>         PTR_ADDU        sp, sp, -16
>         LONG_S          a0,  8(sp)
>         LONG_S          ra, 16(sp)
>         jal             __fill_user
>         LONG_L          v0,  8(sp)
>         LONG_L          ra, 16(sp)
>         PTR_ADDU        sp, sp, 16
>         jr              ra
> END(memset)
> 
> I know it doesn't respect any mips ABI but in this case do
> we really care ?

In general we do (think of stack unwinding etc.).  I believe this
implementation should move to C, as it doesn't need an assembler
implementation:

void *memset (void *s, int c, kernel_size_t n)
{
	__fill_user(s, c, n);
	return s;
}

It looks much nicer that way. :-)


Thiemo

Re: [PATCH] Introduce __fill_user() and kill __bzero()

[Franck Bui-Huu]

Thiemo Seufer wrote:
> In general we do (think of stack unwinding etc.).  I believe this
> implementation should move to C, as it doesn't need an assembler
> implementation:
> 
> void *memset (void *s, int c, kernel_size_t n)
> {
> 	__fill_user(s, c, n);
> 	return s;
> }
> 
> It looks much nicer that way. :-)
> 

Sure but memset.S was a really good place to implement memset(), wasn't
it ?

And since the implementation should have been trivial, I thought it was
ok to implement in assembly.

Ok, I'll look for another place.

Thanks,
		Franck

Re: [PATCH] Introduce __fill_user() and kill __bzero()

[Thiemo Seufer]

Franck Bui-Huu wrote:
> Thiemo Seufer wrote:
> > In general we do (think of stack unwinding etc.).  I believe this
> > implementation should move to C, as it doesn't need an assembler
> > implementation:
> > 
> > void *memset (void *s, int c, kernel_size_t n)
> > {
> > 	__fill_user(s, c, n);
> > 	return s;
> > }
> > 
> > It looks much nicer that way. :-)
> > 
> 
> Sure but memset.S was a really good place to implement memset(), wasn't
> it ?

What about using memset.c and fill_user.S ?

> And since the implementation should have been trivial,

As you found out now, nothing is trivial in assembler. :-)

> I thought it was ok to implement in assembly.

As a general rule, assembly should only be used when C doesn't cut it.


Thiemo

Re: [PATCH] Introduce __fill_user() and kill __bzero()

[Franck Bui-Huu]

On Nov 14, 2007 2:48 PM, Thiemo Seufer <ths@networkno.de> wrote:
> What about using memset.c and fill_user.S ?
>

Quite frankly, I don't know if we could create memset.c and put inside
a function of 2 lines. And I don't think we're going to add some stuff in
it later.

What about implementing fill_user() in C ?

-- 
               Franck