[Qemu-devel] [PATCH] linux-user: Fix do_store_exclusive for shared memory of interprocess.

[Qemu-devel] [PATCH] linux-user: Fix do_store_exclusive for shared memory of interprocess.

[Heiher]

From: Heiher <wangr@lemote.com>

test case: http://pastebin.com/raw/x2GW4xNW

Signed-off-by: Heiher <wangr@lemote.com>
---
 linux-user/main.c | 24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

diff --git a/linux-user/main.c b/linux-user/main.c
index 0e31dad..81b0a49 100644
--- a/linux-user/main.c
+++ b/linux-user/main.c
@@ -2312,6 +2312,23 @@ static const uint8_t mips_syscall_args[] = {
 #  undef MIPS_SYS
 # endif /* O32 */
 
+#define cmpxchg_user(old, new, gaddr, target_type)			\
+({									\
+    abi_ulong __gaddr = (gaddr);					\
+    target_type *__hptr;						\
+    abi_long __ret = 0;							\
+    if ((__hptr = lock_user(VERIFY_WRITE, __gaddr, sizeof(target_type), 0))) { \
+        if ((old) != atomic_cmpxchg(__hptr, (old), (new)))		\
+            __ret = -TARGET_EAGAIN;					\
+        unlock_user(__hptr, __gaddr, sizeof(target_type));		\
+    } else								\
+        __ret = -TARGET_EFAULT;						\
+    __ret;								\
+})
+
+#define cmpxchg_user_u32(old, new, gaddr) cmpxchg_user((old), (new), (gaddr), uint32_t)
+#define cmpxchg_user_u64(old, new, gaddr) cmpxchg_user((old), (new), (gaddr), uint64_t)
+
 static int do_store_exclusive(CPUMIPSState *env)
 {
     target_ulong addr;
@@ -2342,12 +2359,15 @@ static int do_store_exclusive(CPUMIPSState *env)
                 env->active_tc.gpr[reg] = 0;
             } else {
                 if (d) {
-                    segv = put_user_u64(env->llnewval, addr);
+                    segv = cmpxchg_user_u64(env->llval, env->llnewval, addr);
                 } else {
-                    segv = put_user_u32(env->llnewval, addr);
+                    segv = cmpxchg_user_u32(env->llval, env->llnewval, addr);
                 }
                 if (!segv) {
                     env->active_tc.gpr[reg] = 1;
+                } else if (-TARGET_EAGAIN == segv) {
+                    segv = 0;
+                    env->active_tc.gpr[reg] = 0;
                 }
             }
         }
-- 
2.10.0

Re: [Qemu-devel] [PATCH] linux-user: Fix do_store_exclusive for shared memory of interprocess.

[no-reply]

Hi,

Your series seems to have some coding style problems. See output below for
more information:

Subject: [Qemu-devel] [PATCH] linux-user: Fix do_store_exclusive for shared memory of interprocess.
Type: series
Message-id: 20161015155348.26834-1-r@hev.cc

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

# Useful git options
git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git show --no-patch --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
03ca3dc linux-user: Fix do_store_exclusive for shared memory of interprocess.

=== OUTPUT BEGIN ===
Checking PATCH 1/1: linux-user: Fix do_store_exclusive for shared memory of interprocess....
ERROR: code indent should never use tabs
#20: FILE: linux-user/main.c:2315:
+#define cmpxchg_user(old, new, gaddr, target_type)^I^I^I\$

ERROR: code indent should never use tabs
#21: FILE: linux-user/main.c:2316:
+({^I^I^I^I^I^I^I^I^I\$

ERROR: code indent should never use tabs
#22: FILE: linux-user/main.c:2317:
+    abi_ulong __gaddr = (gaddr);^I^I^I^I^I\$

ERROR: code indent should never use tabs
#23: FILE: linux-user/main.c:2318:
+    target_type *__hptr;^I^I^I^I^I^I\$

ERROR: code indent should never use tabs
#24: FILE: linux-user/main.c:2319:
+    abi_long __ret = 0;^I^I^I^I^I^I^I\$

ERROR: do not use assignment in if condition
#25: FILE: linux-user/main.c:2320:
+    if ((__hptr = lock_user(VERIFY_WRITE, __gaddr, sizeof(target_type), 0))) { \

ERROR: braces {} are necessary for all arms of this statement
#25: FILE: linux-user/main.c:2320:
+    if ((__hptr = lock_user(VERIFY_WRITE, __gaddr, sizeof(target_type), 0))) { \
[...]
+    } else								\
[...]

ERROR: code indent should never use tabs
#26: FILE: linux-user/main.c:2321:
+        if ((old) != atomic_cmpxchg(__hptr, (old), (new)))^I^I\$

ERROR: braces {} are necessary for all arms of this statement
#26: FILE: linux-user/main.c:2321:
+        if ((old) != atomic_cmpxchg(__hptr, (old), (new)))		\
[...]

ERROR: code indent should never use tabs
#27: FILE: linux-user/main.c:2322:
+            __ret = -TARGET_EAGAIN;^I^I^I^I^I\$

ERROR: code indent should never use tabs
#28: FILE: linux-user/main.c:2323:
+        unlock_user(__hptr, __gaddr, sizeof(target_type));^I^I\$

ERROR: code indent should never use tabs
#29: FILE: linux-user/main.c:2324:
+    } else^I^I^I^I^I^I^I^I\$

ERROR: code indent should never use tabs
#30: FILE: linux-user/main.c:2325:
+        __ret = -TARGET_EFAULT;^I^I^I^I^I^I\$

ERROR: code indent should never use tabs
#31: FILE: linux-user/main.c:2326:
+    __ret;^I^I^I^I^I^I^I^I\$

WARNING: line over 80 characters
#34: FILE: linux-user/main.c:2329:
+#define cmpxchg_user_u32(old, new, gaddr) cmpxchg_user((old), (new), (gaddr), uint32_t)

WARNING: line over 80 characters
#35: FILE: linux-user/main.c:2330:
+#define cmpxchg_user_u64(old, new, gaddr) cmpxchg_user((old), (new), (gaddr), uint64_t)

total: 14 errors, 2 warnings, 40 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org

Re: [Qemu-devel] [PATCH] linux-user: Fix do_store_exclusive for shared memory of interprocess.

[Emilio G. Cota]

(Adding Richard to Cc)

On Sat, Oct 15, 2016 at 23:53:48 +0800, Heiher wrote:
> From: Heiher <wangr@lemote.com>
> 
> test case: http://pastebin.com/raw/x2GW4xNW

You should check out this patchset and use it as a base for working on this
topic:

  http://lists.nongnu.org/archive/html/qemu-devel/2016-10/msg02341.html

In particular, the added tests/atomic_add-bench does a very similar thing
to what you're doing with your test case -- although with pthreads instead of
mmap(MAP_SHARED) + fork.

(more comments below)

> Signed-off-by: Heiher <wangr@lemote.com>
> ---
>  linux-user/main.c | 24 ++++++++++++++++++++++--
>  1 file changed, 22 insertions(+), 2 deletions(-)
> 
> diff --git a/linux-user/main.c b/linux-user/main.c
> index 0e31dad..81b0a49 100644
> --- a/linux-user/main.c
> +++ b/linux-user/main.c
> @@ -2312,6 +2312,23 @@ static const uint8_t mips_syscall_args[] = {
>  #  undef MIPS_SYS
>  # endif /* O32 */
>  
> +#define cmpxchg_user(old, new, gaddr, target_type)			\
> +({									\
> +    abi_ulong __gaddr = (gaddr);					\
> +    target_type *__hptr;						\
> +    abi_long __ret = 0;							\
> +    if ((__hptr = lock_user(VERIFY_WRITE, __gaddr, sizeof(target_type), 0))) { \
> +        if ((old) != atomic_cmpxchg(__hptr, (old), (new)))		\
> +            __ret = -TARGET_EAGAIN;					\
> +        unlock_user(__hptr, __gaddr, sizeof(target_type));		\
> +    } else								\
> +        __ret = -TARGET_EFAULT;						\
> +    __ret;								\
> +})
> +
> +#define cmpxchg_user_u32(old, new, gaddr) cmpxchg_user((old), (new), (gaddr), uint32_t)
> +#define cmpxchg_user_u64(old, new, gaddr) cmpxchg_user((old), (new), (gaddr), uint64_t)
> +
>  static int do_store_exclusive(CPUMIPSState *env)
>  {
>      target_ulong addr;
> @@ -2342,12 +2359,15 @@ static int do_store_exclusive(CPUMIPSState *env)
>                  env->active_tc.gpr[reg] = 0;
>              } else {
>                  if (d) {
> -                    segv = put_user_u64(env->llnewval, addr);
> +                    segv = cmpxchg_user_u64(env->llval, env->llnewval, addr);
>                  } else {
> -                    segv = put_user_u32(env->llnewval, addr);
> +                    segv = cmpxchg_user_u32(env->llval, env->llnewval, addr);
>                  }
>                  if (!segv) {
>                      env->active_tc.gpr[reg] = 1;
> +                } else if (-TARGET_EAGAIN == segv) {
> +                    segv = 0;
> +                    env->active_tc.gpr[reg] = 0;
>                  }
>              }
>          }

With the atomic cmpxchg patch series referenced above, we should directly
translate to cmpxchg, thereby removing the exception--just like this
patch does for the Alpha architecture:
  http://lists.nongnu.org/archive/html/qemu-devel/2016-10/msg02373.html

Thanks

                Emilio

Re: [Qemu-devel] [PATCH] linux-user: Fix do_store_exclusive for shared memory of interprocess.

[Richard Henderson]

On 10/15/2016 08:53 AM, Heiher wrote:
> +    if ((__hptr = lock_user(VERIFY_WRITE, __gaddr, sizeof(target_type), 0))) { \
> +        if ((old) != atomic_cmpxchg(__hptr, (old), (new)))		\
> +            __ret = -TARGET_EAGAIN;					\
> +        unlock_user(__hptr, __gaddr, sizeof(target_type));		\

This doesn't perform an atomic operation, because lock_user and unlock_user 
copy data from and to the guest.  The atomic operation you're doing is on 
memory private to the host.

You also have to handle host byte order != target byte order.

That said, we are some way toward addressing this.  The patch set that Emilio 
pointed you at is a good start.


r~