Code Review - Fix gcc-fuggetaboud-ecx issue

2 views
Skip to first unread message

Davide Libenzi

unread,
Dec 9, 2015, 10:33:44 PM12/9/15
to Akaros
This moves the user memcpy inline asm to an uaccess.c file, to avoid GCC messing up with ECX loading.


https://github.com/brho/akaros/compare/master...dlibenzi:gcc_fuggetaboud_ecx


The following changes since commit 2fa42319139e4cc5ca853546363f84443d0ead00:

  Rename 'reallocarray' to 'kreallocarray'. (2015-11-25 18:02:04 -0500)

are available in the git repository at:

  g...@github.com:dlibenzi/akaros gcc_fuggetaboud_ecx

for you to fetch changes up to 93d0ee92f6a3fee03e149a7462f17277c386cbda:

  Fix nasty GCC bug where ECX was forgotten (2015-12-09 19:29:11 -0800)

----------------------------------------------------------------
Davide Libenzi (1):
      Fix nasty GCC bug where ECX was forgotten

 kern/arch/x86/Kbuild    |  1 +
 kern/arch/x86/uaccess.c | 26 ++++++++++++++++++++++++++
 kern/arch/x86/uaccess.h | 11 +++++++----
 3 files changed, 34 insertions(+), 4 deletions(-)
 create mode 100644 kern/arch/x86/uaccess.c

diff --git a/kern/arch/x86/Kbuild b/kern/arch/x86/Kbuild
index ae25726..cbc4e65 100644
--- a/kern/arch/x86/Kbuild
+++ b/kern/arch/x86/Kbuild
@@ -31,4 +31,5 @@ obj-y += trap.o trap64.o
 obj-y += trapentry64.o
 obj-y += usb.o
 obj-y += topology.o
+obj-y += uaccess.o
 obj-y += vmm/
diff --git a/kern/arch/x86/uaccess.c b/kern/arch/x86/uaccess.c
new file mode 100644
index 0000000..6d13924
--- /dev/null
+++ b/kern/arch/x86/uaccess.c
@@ -0,0 +1,26 @@
+/* Copyright (c) 2015 Google Inc
+ * Davide Libenzi <dlib...@google.com>
+ * See LICENSE for details.
+ *
+ * Part of this code coming from a Linux kernel file:
+ *
+ * linux/arch/x86/include/asm/uaccess.h
+ *
+ * Which, even though missing specific copyright, it is supposed to be
+ * ruled by the overall Linux copyright.
+ */
+
+#include <ros/errno.h>
+#include <compiler.h>
+#include <stdint.h>
+#include <umem.h>
+#include <arch/uaccess.h>
+
+int user_memcpy(void *dst, const void *src, unsigned int count)
+{
+ int err = 0;
+
+ __user_memcpy(dst, src, count, err, -EFAULT);
+
+ return err;
+}
diff --git a/kern/arch/x86/uaccess.h b/kern/arch/x86/uaccess.h
index cba0ef5..d5f1ea3 100644
--- a/kern/arch/x86/uaccess.h
+++ b/kern/arch/x86/uaccess.h
@@ -94,6 +94,7 @@ struct extable_ip_fixup {
 
 #define __user_memcpy(dst, src, count, err, errret) \
  asm volatile(ASM_STAC "\n" \
+ " cld\n" \
  "1: rep movsb\n" \
  "2: " ASM_CLAC "\n" \
  ".section .fixup,\"ax\"\n" \
@@ -105,6 +106,8 @@ struct extable_ip_fixup {
  : "D" (dst), "S" (src), "c" (count), "i" (errret), "0" (err) \
  : "memory")
 
+int user_memcpy(void *dst, const void *src, unsigned int count);
+
 static inline int __put_user(void *dst, const void *src, unsigned int count)
 {
  int err = 0;
@@ -127,7 +130,7 @@ static inline int __put_user(void *dst, const void *src, unsigned int count)
    "", "er", -EFAULT);
  break;
  default:
- __user_memcpy(dst, src, count, err, -EFAULT);
+ err = user_memcpy(dst, src, count);
  }
 
  return err;
@@ -140,7 +143,7 @@ static inline int copy_to_user(void *dst, const void *src, unsigned int count)
  if (unlikely(!is_user_rwaddr(dst, count))) {
  err = -EFAULT;
  } else if (!__builtin_constant_p(count)) {
- __user_memcpy(dst, src, count, err, -EFAULT);
+ err = user_memcpy(dst, src, count);
  } else {
  err = __put_user(dst, src, count);
  }
@@ -170,7 +173,7 @@ static inline int __get_user(void *dst, const void *src, unsigned int count)
    "", "=r", -EFAULT);
  break;
  default:
- __user_memcpy(dst, src, count, err, -EFAULT);
+ err = user_memcpy(dst, src, count);
  }
 
  return err;
@@ -184,7 +187,7 @@ static inline int copy_from_user(void *dst, const void *src,
  if (unlikely(!is_user_raddr((void *) src, count))) {
  err = -EFAULT;
  } else if (!__builtin_constant_p(count)) {
- __user_memcpy(dst, src, count, err, -EFAULT);
+ err = user_memcpy(dst, src, count);
  } else {
  err = __get_user(dst, src, count);
  }

Davide Libenzi

unread,
Dec 9, 2015, 10:54:03 PM12/9/15
to Akaros
Hold on.
This is one solution.
Adding ecx, esi, edi in the clobber will not work, as they are inputs, so alternative proposal follows ...

Reply all
Reply to author
Forward
0 new messages