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~

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 
> 
> 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 
> ---
>  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.

[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-...@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-de...@freelists.org

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

[Heiher]

From: Heiher 

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

Signed-off-by: Heiher 
---
 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