kernel - Optimize bcopy, bzero, memset
authorMatthew Dillon <dillon@apollo.backplane.com>
Sun, 23 Sep 2018 22:42:26 +0000 (15:42 -0700)
committerMatthew Dillon <dillon@apollo.backplane.com>
Tue, 25 Sep 2018 16:29:09 +0000 (09:29 -0700)
* Use __builtin_memset() for bzero() and __builtin_memmove()
  for bcopy().

  - Must use _bcopy in a few places where GCC complains about
    structural punning.  Even casting doesn't help.

  - GCC's __builtin_memset() and __builtin_memmove() has a side
    effect where it assumes that the pointer arguments cannot be
    NULL.  In fact, they can be NULL when the byte count is 0.
    This assumption by GCC causes later unrelated conditionals
    on the pointers against NULL to be improperly optimized-out.

    We had to fix one place where this blew the system up.

* Implement memset() in assembly (remove from libkern).

* Implement memmove() in assembly (remove from libkern).

sys/dev/crypto/safe/safe.c
sys/dev/crypto/ubsec/ubsec.c
sys/kern/subr_bus.c
sys/libkern/memmove.c
sys/libkern/memset.c
sys/net/bpf.c
sys/platform/pc64/x86_64/machdep.c
sys/platform/pc64/x86_64/support.s
sys/platform/pc64/x86_64/trap.c
sys/sys/systm.h

index ad5c72f..c616559 100644 (file)
@@ -661,7 +661,8 @@ safe_setup_mackey(struct safe_session *ses, int algo, caddr_t key, int klen)
                MD5Init(&md5ctx);
                MD5Update(&md5ctx, key, klen);
                MD5Update(&md5ctx, hmac_ipad_buffer, MD5_HMAC_BLOCK_LEN - klen);
-               bcopy(&md5ctx.A, ses->ses_hminner, sizeof(md5ctx.A) * 4);
+               /* gcc8 craps out on -Warray-bounds w/ optimized bcopy */
+               _bcopy(&md5ctx.A, ses->ses_hminner, sizeof(md5ctx.A) * 4);
        } else {
                SHA1Init(&sha1ctx);
                SHA1Update(&sha1ctx, key, klen);
@@ -677,7 +678,8 @@ safe_setup_mackey(struct safe_session *ses, int algo, caddr_t key, int klen)
                MD5Init(&md5ctx);
                MD5Update(&md5ctx, key, klen);
                MD5Update(&md5ctx, hmac_opad_buffer, MD5_HMAC_BLOCK_LEN - klen);
-               bcopy(&md5ctx.A, ses->ses_hmouter, sizeof(md5ctx.A) * 4);
+               /* gcc8 craps out on -Warray-bounds w/ optimized bcopy */
+               _bcopy(&md5ctx.A, ses->ses_hmouter, sizeof(md5ctx.A) * 4);
        } else {
                SHA1Init(&sha1ctx);
                SHA1Update(&sha1ctx, key, klen);
index 2335270..06bcd9c 100644 (file)
@@ -851,7 +851,8 @@ ubsec_setup_mackey(struct ubsec_session *ses, int algo, caddr_t key, int klen)
                MD5Init(&md5ctx);
                MD5Update(&md5ctx, key, klen);
                MD5Update(&md5ctx, hmac_ipad_buffer, MD5_HMAC_BLOCK_LEN - klen);
-               bcopy(&md5ctx.A, ses->ses_hminner, sizeof(md5ctx.A) * 4);
+               /* gcc8 craps out on -Warray-bounds w/ optimized bcopy */
+               _bcopy(&md5ctx.A, ses->ses_hminner, sizeof(md5ctx.A) * 4);
        } else {
                SHA1Init(&sha1ctx);
                SHA1Update(&sha1ctx, key, klen);
@@ -867,7 +868,8 @@ ubsec_setup_mackey(struct ubsec_session *ses, int algo, caddr_t key, int klen)
                MD5Init(&md5ctx);
                MD5Update(&md5ctx, key, klen);
                MD5Update(&md5ctx, hmac_opad_buffer, MD5_HMAC_BLOCK_LEN - klen);
-               bcopy(&md5ctx.A, ses->ses_hmouter, sizeof(md5ctx.A) * 4);
+               /* gcc8 craps out on -Warray-bounds w/ optimized bcopy */
+               _bcopy(&md5ctx.A, ses->ses_hmouter, sizeof(md5ctx.A) * 4);
        } else {
                SHA1Init(&sha1ctx);
                SHA1Update(&sha1ctx, key, klen);
index 9767402..e12453d 100644 (file)
@@ -1094,9 +1094,21 @@ devclass_alloc_unit(devclass_t dc, int *unitp)
                                 M_INTWAIT | M_ZERO);
                if (newlist == NULL)
                        return(ENOMEM);
-               bcopy(dc->devices, newlist, sizeof(device_t) * dc->maxunit);
-               if (dc->devices)
+               /*
+                * WARNING: Due to gcc builtin optimization,
+                *          calling bcopy causes gcc to assume
+                *          that the source and destination args
+                *          cannot be NULL and optimize-away later
+                *          conditional tests to determine if dc->devices
+                *          is NULL.  In this situation, in fact,
+                *          dc->devices CAN be NULL w/ maxunit == 0.
+                */
+               if (dc->devices) {
+                       bcopy(dc->devices,
+                             newlist,
+                             sizeof(device_t) * dc->maxunit);
                        kfree(dc->devices, M_BUS);
+               }
                dc->devices = newlist;
                dc->maxunit = newsize;
        }
index e4b8d51..9db11fd 100644 (file)
@@ -28,6 +28,8 @@
 
 #include <sys/libkern.h>
 
+#if 0
+/* moved to support.s.  interferes with bcopy macro */
 void *
 memmove(void *dest, const void *src, size_t n)
 {
@@ -35,3 +37,4 @@ memmove(void *dest, const void *src, size_t n)
        bcopy(src, dest, n);
        return (dest);
 }
+#endif
index 77d6b28..0a33606 100644 (file)
@@ -28,6 +28,8 @@
 
 #define        LIBKERN_INLINE
 
+#if 0
+
 #include <sys/types.h>
 #include <sys/libkern.h>
 
@@ -37,9 +39,11 @@ memset(void *b, int c, size_t len)
        char *bb;
 
        if (c == 0)
-               bzero(b, len);
+               _bzero(b, len);
        else
                for (bb = (char *)b; len--; )
                        *bb++ = c;
        return (b);
 }
+
+#endif
index ddc0aaa..75a4225 100644 (file)
@@ -1260,7 +1260,7 @@ bpf_tap(struct bpf_if *bp, u_char *pkt, u_int pktlen)
                                microtime(&tv);
                                gottime = 1;
                        }
-                       catchpacket(d, pkt, pktlen, slen, bcopy, &tv);
+                       catchpacket(d, pkt, pktlen, slen, _bcopy, &tv);
                }
        }
        lwkt_reltoken(&bpf_token);
index 5100f24..e83ecca 100644 (file)
@@ -744,7 +744,8 @@ sendsig(sig_t catcher, int sig, sigset_t *mask, u_long code)
        sf.sf_uc.uc_stack = lp->lwp_sigstk;
        sf.sf_uc.uc_mcontext.mc_onstack = oonstack;
        KKASSERT(__offsetof(struct trapframe, tf_rdi) == 0);
-       bcopy(regs, &sf.sf_uc.uc_mcontext.mc_rdi, sizeof(struct trapframe));
+       /* gcc errors out on optimized bcopy */
+       _bcopy(regs, &sf.sf_uc.uc_mcontext.mc_rdi, sizeof(struct trapframe));
 
        /* Make the size of the saved context visible to userland */
        sf.sf_uc.uc_mcontext.mc_len = sizeof(sf.sf_uc.uc_mcontext);
@@ -1017,7 +1018,9 @@ sys_sigreturn(struct sigreturn_args *uap)
                        trapsignal(lp, SIGBUS, T_PROTFLT);
                        return(EINVAL);
                }
-               bcopy(&ucp->uc_mcontext.mc_rdi, regs, sizeof(struct trapframe));
+               /* gcc errors out on optimized bcopy */
+               _bcopy(&ucp->uc_mcontext.mc_rdi, regs,
+                      sizeof(struct trapframe));
        }
 
        /*
index ef60345..11eebd7 100644 (file)
@@ -64,6 +64,39 @@ ENTRY(bzero)
        ret
 END(bzero)
 
+       .weak   _bzero
+       .equ    _bzero, bzero
+
+/*
+ * void *memset(ptr:%rdi, char:%rsi, bytes:%rdx)
+ *
+ * Same as bzero except we load the char into all byte
+ * positions of %rax.  Returns original (ptr).
+ */
+ENTRY(memset)
+       movzbq  %sil,%r8
+       movabs  $0x0101010101010101,%rax
+       imulq   %r8,%rax
+
+       movq    %rdi,%r9
+       movq    %rdx,%rcx
+       shrq    $3,%rcx
+       rep
+       stosq
+       movq    %rdx,%rcx
+       andq    $7,%rcx
+       jnz     1f
+       movq    %r9,%rax
+       ret
+1:     rep
+       stosb
+       movq    %r9,%rax
+       ret
+END(memset)
+
+       .weak   _memset
+       .equ    _memset, memset
+
 /*
  * pagezero(ptr:%rdi)
  *
@@ -163,6 +196,60 @@ ENTRY(bcopy)
        ret
 END(bcopy)
 
+       /*
+        * Use in situations where a bcopy function pointer is needed.
+        */
+       .weak   _bcopy
+       .equ    _bcopy, bcopy
+
+       /*
+        * memmove(dst:%rdi, src:%rsi, cnt:%rdx)
+        * (same as bcopy but without the xchgq, and must return (dst)).
+        *
+        * NOTE: gcc builtin backs-off to memmove() call
+        *
+        * NOTE: We leave %rdi in %rax for the return value.
+        */
+ENTRY(memmove)
+       movq    %rdx,%rcx
+       movq    %rdi,%rax                       /* return value */
+       movq    %rdi,%r8
+       subq    %rsi,%r8
+       cmpq    %rcx,%r8                        /* overlapping && src < dst? */
+       jb      2f
+
+       shrq    $3,%rcx                         /* copy by 64-bit words */
+       rep
+       movsq
+       movq    %rdx,%rcx
+       andq    $7,%rcx                         /* any bytes left? */
+       jnz     1f
+       ret
+1:     rep
+       movsb
+       ret
+
+       ALIGN_TEXT
+2:
+       addq    %rcx,%rdi                       /* copy backwards */
+       addq    %rcx,%rsi
+       std
+       decq    %rdi
+       decq    %rsi
+       andq    $7,%rcx                         /* any fractional bytes? */
+       jz      3f
+       rep
+       movsb
+3:     movq    %rdx,%rcx                       /* copy by 32-bit words */
+       shrq    $3,%rcx
+       subq    $7,%rsi
+       subq    $7,%rdi
+       rep
+       movsq
+       cld
+       ret
+END(memmove)
+
 ENTRY(reset_dbregs)
        movq    $0x200,%rax     /* the manual says that bit 10 must be set to 1 */
        movq    %rax,%dr7       /* disable all breapoints first */
@@ -178,7 +265,8 @@ END(reset_dbregs)
 /*
  * memcpy(dst:%rdi, src:%rsi, bytes:%rdx)
  *
- * Note: memcpy does not support overlapping copies
+ * NOTE: memcpy does not support overlapping copies
+ * NOTE: returns dst
  */
 ENTRY(memcpy)
        movq    %rdi,%r8
index d9603bd..db1cded 100644 (file)
@@ -1203,7 +1203,7 @@ syscall2(struct trapframe *frame)
         * Its easier to copy up to the highest number of syscall arguments
         * passed in registers, which is 6, than to conditionalize it.
         */
-       __builtin_memcpy(argsdst, argp, sizeof(register_t) * optimized_regcnt);
+       bcopy(argp, argsdst, sizeof(register_t) * optimized_regcnt);
 
        /*
         * Any arguments beyond available argument-passing registers must
index 6166198..c62067f 100644 (file)
@@ -232,12 +232,33 @@ quad_t    strtoq(const char *, char **, int) __nonnull(1);
 u_quad_t strtouq(const char *, char **, int) __nonnull(1);
 
 /*
- * note: some functions commonly used by device drivers may be passed
- * pointers to volatile storage, volatile set to avoid warnings.
+ * NOTE: some functions commonly used by device drivers may be passed
+ *       pointers to volatile storage, volatile set to avoid warnings.
+ *
+ * NOTE: When using builtin's, GCC does enormous optimizations and makes
+ *       a number of assumptions that can cause later unrelated conditionals
+ *       to be optimized out.  In the case of memset and memmove, GCC
+ *       assumes that (to) and (from) cannot be NULL (even when (len) might
+ *       be 0), and will optimize-out later NULL tests on (to) or (from).
  */
+#if 1
+#define bcopy(from, to, len)                           \
+       __builtin_memmove(__DEQUALIFY(void *, (to)),    \
+                         __DEQUALIFY(void *, (from)),  \
+                         (len))
+#else
 void   bcopy(volatile const void *from, volatile void *to, size_t len)
            __nonnull(1, 2);
+#endif
+#if 1
+#define bzero(buf, len)                                        \
+       __builtin_memset(__DEQUALIFY(void *, (buf)), 0, (len))
+#else
 void   bzero(volatile void *buf, size_t len) __nonnull(1);
+#endif
+void   _bcopy(volatile const void *from, volatile void *to, size_t len)
+           __nonnull(1, 2);
+void   _bzero(volatile void *buf, size_t len) __nonnull(1);
 void   bzeront(volatile void *buf, size_t len) __nonnull(1);
 void   *memcpy(void *to, const void *from, size_t len)
            __nonnull(1, 2);