[Qemu-devel] [PATCH 0/4] linux-user improvements

[Qemu-devel] [PATCH 0/4] linux-user improvements

[Richard Henderson]

The remaining alpha-linux-user patch, with updated dependencies.
In addition, a new fix for guest system paths containing links.


r~


Richard Henderson (4):
  cpu-all: Add unaligned load/store helper functions
  linux-user: Rewrite __get_user/__put_user with __builtin_choose_expr
  alpha-linux-user: Fix sigaction
  user: Consider symbolic links as possible directories

 cpu-all.h                 | 38 +++++++++++++++++++++++++++
 linux-user/qemu.h         | 67 ++++++++++++++++++++++++++---------------------
 linux-user/signal.c       | 22 ++++++----------
 linux-user/syscall_defs.h |  2 +-
 path.c                    |  5 ++--
 5 files changed, 87 insertions(+), 47 deletions(-)

-- 
1.7.11.7

[Qemu-devel] [PATCH 1/4] cpu-all: Add unaligned load/store helper functions

[Richard Henderson]

Signed-off-by: Richard Henderson <rth@twiddle.net>
---
 cpu-all.h | 38 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/cpu-all.h b/cpu-all.h
index 2b99682..2db4414 100644
--- a/cpu-all.h
+++ b/cpu-all.h
@@ -113,6 +113,44 @@ static inline void tswap64s(uint64_t *s)
 #define bswaptls(s) bswap64s(s)
 #endif
 
+/* Unaligned loads and stores.  */
+
+static inline uint16_t unaligned_r16(const void *ptr)
+{
+    uint16_t ret;
+    memcpy(&ret, ptr, sizeof(ret));
+    return ret;
+}
+
+static inline uint32_t unaligned_r32(const void *ptr)
+{
+    uint32_t ret;
+    memcpy(&ret, ptr, sizeof(ret));
+    return ret;
+}
+
+static inline uint64_t unaligned_r64(const void *ptr)
+{
+    uint64_t ret;
+    memcpy(&ret, ptr, sizeof(ret));
+    return ret;
+}
+
+static inline void unaligned_w16(void *ptr, uint16_t v)
+{
+    memcpy(ptr, &v, sizeof(v));
+}
+
+static inline void unaligned_w32(void *ptr, uint32_t v)
+{
+    memcpy(ptr, &v, sizeof(v));
+}
+
+static inline void unaligned_w64(void *ptr, uint64_t v)
+{
+    memcpy(ptr, &v, sizeof(v));
+}
+
 /* CPU memory access without any memory or io remapping */
 
 /*
-- 
1.7.11.7

[Qemu-devel] [PATCH 2/4] linux-user: Rewrite __get_user/__put_user with __builtin_choose_expr

[Richard Henderson]

The previous formuation with multiple assignments to __typeof(*hptr) falls
down when hptr is qualified const.  E.g. with const struct S *p, p->f is
also qualified const.

With this formulation, there's no assignment to any local variable.

Signed-off-by: Richard Henderson <rth@twiddle.net>
---
 linux-user/qemu.h | 67 ++++++++++++++++++++++++++++++-------------------------
 1 file changed, 37 insertions(+), 30 deletions(-)

diff --git a/linux-user/qemu.h b/linux-user/qemu.h
index 5e53dca..bf0c911 100644
--- a/linux-user/qemu.h
+++ b/linux-user/qemu.h
@@ -287,36 +287,43 @@ static inline int access_ok(int type, abi_ulong addr, abi_ulong size)
                             (type == VERIFY_READ) ? PAGE_READ : (PAGE_READ | PAGE_WRITE)) == 0;
 }
 
-/* NOTE __get_user and __put_user use host pointers and don't check access. */
-/* These are usually used to access struct data members once the
- * struct has been locked - usually with lock_user_struct().
- */
-#define __put_user(x, hptr)\
-({ __typeof(*hptr) pu_ = (x);\
-    switch(sizeof(*hptr)) {\
-    case 1: break;\
-    case 2: pu_ = tswap16(pu_); break; \
-    case 4: pu_ = tswap32(pu_); break; \
-    case 8: pu_ = tswap64(pu_); break; \
-    default: abort();\
-    }\
-    memcpy(hptr, &pu_, sizeof(pu_)); \
-    0;\
-})
-
-#define __get_user(x, hptr) \
-({ __typeof(*hptr) gu_; \
-    memcpy(&gu_, hptr, sizeof(gu_)); \
-    switch(sizeof(*hptr)) {\
-    case 1: break; \
-    case 2: gu_ = tswap16(gu_); break; \
-    case 4: gu_ = tswap32(gu_); break; \
-    case 8: gu_ = tswap64(gu_); break; \
-    default: abort();\
-    }\
-    (x) = gu_; \
-    0;\
-})
+/* NOTE __get_user and __put_user use host pointers and don't check access.
+   These are usually used to access struct data members once the struct has
+   been locked - usually with lock_user_struct.  */
+
+/* Tricky points:
+   - Use __builtin_choose_expr to avoid type promotion from ?:,
+   - Invalid sizes result in a "invalid use of void expression" error,
+     stemming from the fact that the return type of abort is void.
+   - While we eliminate byte stores with the "true" part of the first
+     choose, the "false" part of the first choose must remain valid
+     to avoid tripping over the "invalid use" error above.  Thus the
+     use of <= 2 to keep byte stores syntactically ok in the discarded
+     expression.  */
+
+#define __put_user(x, hptr)                                               \
+(__builtin_choose_expr(sizeof(*(hptr)) == 1, *(hptr) = (uint8_t)(x),      \
+   __builtin_choose_expr(sizeof(*(hptr)) <= 2, unaligned_w16,             \
+   __builtin_choose_expr(sizeof(*(hptr)) == 4, unaligned_w32,             \
+   __builtin_choose_expr(sizeof(*(hptr)) == 8, unaligned_w64, abort)))    \
+     ((hptr),                                                             \
+      __builtin_choose_expr(sizeof(*(hptr)) <= 2, tswap16((uint16_t)(x)), \
+      __builtin_choose_expr(sizeof(*(hptr)) == 4, tswap32((uint32_t)(x)), \
+      __builtin_choose_expr(sizeof(*(hptr)) == 8, tswap64((uint64_t)(x)), \
+                            abort()))))),                                 \
+ 0)
+
+#define __get_user(x, hptr)                                           \
+((x) =                                                                \
+ __builtin_choose_expr(sizeof(*(hptr)) == 1, *(hptr),                 \
+ __builtin_choose_expr(sizeof(*(hptr)) == 2,                          \
+                       (typeof(*(hptr)))tswap16(unaligned_r16(hptr)), \
+ __builtin_choose_expr(sizeof(*(hptr)) == 4,                          \
+                       (typeof(*(hptr)))tswap32(unaligned_r32(hptr)), \
+ __builtin_choose_expr(sizeof(*(hptr)) == 8,                          \
+                       (typeof(*(hptr)))tswap64(unaligned_r64(hptr)), \
+                       abort())))),                                   \
+ 0)
 
 /* put_user()/get_user() take a guest address and check access */
 /* These are usually used to access an atomic data type, such as an int,
-- 
1.7.11.7

[Qemu-devel] [PATCH 3/4] alpha-linux-user: Fix sigaction

[Richard Henderson]

Unconditional bswap replaced by __get_user/__put_user.

Signed-off-by: Richard Henderson <rth@twiddle.net>
---
 linux-user/signal.c       | 22 ++++++++--------------
 linux-user/syscall_defs.h |  2 +-
 2 files changed, 9 insertions(+), 15 deletions(-)

diff --git a/linux-user/signal.c b/linux-user/signal.c
index 95e2ffa..407619a 100644
--- a/linux-user/signal.c
+++ b/linux-user/signal.c
@@ -607,28 +607,22 @@ int do_sigaction(int sig, const struct target_sigaction *act,
             sig, act, oact);
 #endif
     if (oact) {
-        oact->_sa_handler = tswapal(k->_sa_handler);
-#if defined(TARGET_MIPS) || defined (TARGET_ALPHA)
-        oact->sa_flags = bswap32(k->sa_flags);
-#else
-        oact->sa_flags = tswapal(k->sa_flags);
-#endif
+        __put_user(k->_sa_handler, &oact->_sa_handler);
+        __put_user(k->sa_flags, &oact->sa_flags);
 #if !defined(TARGET_MIPS)
-        oact->sa_restorer = tswapal(k->sa_restorer);
+        __put_user(k->sa_restorer, &oact->sa_restorer);
 #endif
+        /* Not swapped.  */
         oact->sa_mask = k->sa_mask;
     }
     if (act) {
         /* FIXME: This is not threadsafe.  */
-        k->_sa_handler = tswapal(act->_sa_handler);
-#if defined(TARGET_MIPS) || defined (TARGET_ALPHA)
-        k->sa_flags = bswap32(act->sa_flags);
-#else
-        k->sa_flags = tswapal(act->sa_flags);
-#endif
+        __get_user(k->_sa_handler, &act->_sa_handler);
+        __get_user(k->sa_flags, &act->sa_flags);
 #if !defined(TARGET_MIPS)
-        k->sa_restorer = tswapal(act->sa_restorer);
+        __get_user(k->sa_restorer, &act->sa_restorer);
 #endif
+        /* To be swapped in target_to_host_sigset.  */
         k->sa_mask = act->sa_mask;
 
         /* we update the host linux signal state */
diff --git a/linux-user/syscall_defs.h b/linux-user/syscall_defs.h
index a98cbf7..8ca70b9 100644
--- a/linux-user/syscall_defs.h
+++ b/linux-user/syscall_defs.h
@@ -540,7 +540,7 @@ int do_sigaction(int sig, const struct target_sigaction *act,
 struct target_old_sigaction {
     abi_ulong _sa_handler;
     abi_ulong sa_mask;
-    abi_ulong sa_flags;
+    int32_t sa_flags;
 };
 
 struct target_rt_sigaction {
-- 
1.7.11.7

[Qemu-devel] [PATCH 4/4] user: Consider symbolic links as possible directories

[Richard Henderson]

Commit 2296f194dfde4c0a54f249d3fdb8c8ca21dc611b reduced the number
of syscalls performed during user emulation startup, but failed to
consider the use of symbolic links in creating directory structures.

Signed-off-by: Richard Henderson <rth@twiddle.net>
---
 path.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/path.c b/path.c
index ef3f277..4c5b0f6 100644
--- a/path.c
+++ b/path.c
@@ -58,9 +58,10 @@ static struct pathelem *new_entry(const char *root,
 #define streq(a,b) (strcmp((a), (b)) == 0)
 
 /* Not all systems provide this feature */
-#if defined(DT_DIR) && defined(DT_UNKNOWN)
+#if defined(DT_DIR) && defined(DT_UNKNOWN) && defined(DT_LNK)
 # define dirent_type(dirent) ((dirent)->d_type)
-# define is_dir_maybe(type)  ((type) == DT_DIR || (type) == DT_UNKNOWN)
+# define is_dir_maybe(type) \
+    ((type) == DT_DIR || (type) == DT_UNKNOWN || (type) == DT_LNK)
 #else
 # define dirent_type(dirent) (1)
 # define is_dir_maybe(type)  (type)
-- 
1.7.11.7

Re: [Qemu-devel] [PATCH 1/4] cpu-all: Add unaligned load/store helper functions

[Blue Swirl]

On Wed, Oct 17, 2012 at 4:17 AM, Richard Henderson <rth@twiddle.net> wrote:
> Signed-off-by: Richard Henderson <rth@twiddle.net>
> ---
>  cpu-all.h | 38 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 38 insertions(+)
>
> diff --git a/cpu-all.h b/cpu-all.h
> index 2b99682..2db4414 100644
> --- a/cpu-all.h
> +++ b/cpu-all.h
> @@ -113,6 +113,44 @@ static inline void tswap64s(uint64_t *s)
>  #define bswaptls(s) bswap64s(s)
>  #endif
>
> +/* Unaligned loads and stores.  */
> +
> +static inline uint16_t unaligned_r16(const void *ptr)

I think the names should match other loads and stores, so this should
be something like lduw_unaligned_raw().

> +{
> +    uint16_t ret;
> +    memcpy(&ret, ptr, sizeof(ret));

This also assumes native host byte order which may not be so useful.
Perhaps actually there should be three versions, _be, _le and without
infix for target byte order?

> +    return ret;
> +}
> +
> +static inline uint32_t unaligned_r32(const void *ptr)
> +{
> +    uint32_t ret;
> +    memcpy(&ret, ptr, sizeof(ret));
> +    return ret;
> +}
> +
> +static inline uint64_t unaligned_r64(const void *ptr)
> +{
> +    uint64_t ret;
> +    memcpy(&ret, ptr, sizeof(ret));
> +    return ret;
> +}
> +
> +static inline void unaligned_w16(void *ptr, uint16_t v)
> +{
> +    memcpy(ptr, &v, sizeof(v));
> +}
> +
> +static inline void unaligned_w32(void *ptr, uint32_t v)
> +{
> +    memcpy(ptr, &v, sizeof(v));
> +}
> +
> +static inline void unaligned_w64(void *ptr, uint64_t v)
> +{
> +    memcpy(ptr, &v, sizeof(v));
> +}
> +
>  /* CPU memory access without any memory or io remapping */
>
>  /*
> --
> 1.7.11.7
>
>