[PATCH 0/3] linux-user/arm: Improvements for commpage atomics

[PATCH 0/3] linux-user/arm: Improvements for commpage atomics

[Richard Henderson]

Only the memory_barrer one is a bug; the rest improve system-wide
interaction, and would only affect pre-armv6.


r~


Richard Henderson (3):
  linux-user/arm: Implement __kernel_memory_barrier
  linux-user/arm: Implement __kernel_cmpxchg with host atomics
  linux-user/arm: Implement __kernel_cmpxchg64 with host atomics

 linux-user/arm/cpu_loop.c | 166 +++++++++++++++++++++++---------------
 1 file changed, 99 insertions(+), 67 deletions(-)

-- 
2.25.1

[PATCH 1/3] linux-user/arm: Implement __kernel_memory_barrier

[Richard Henderson]

This fallback syscall was stubbed out.
It would only matter for emulating pre-armv6.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 linux-user/arm/cpu_loop.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/linux-user/arm/cpu_loop.c b/linux-user/arm/cpu_loop.c
index 032e1ffddf..a0e43b261c 100644
--- a/linux-user/arm/cpu_loop.c
+++ b/linux-user/arm/cpu_loop.c
@@ -158,7 +158,7 @@ do_kernel_trap(CPUARMState *env)
 
     switch (env->regs[15]) {
     case 0xffff0fa0: /* __kernel_memory_barrier */
-        /* ??? No-op. Will need to do better for SMP.  */
+        smp_mb();
         break;
     case 0xffff0fc0: /* __kernel_cmpxchg */
          /* XXX: This only works between threads, not between processes.
-- 
2.25.1

Re: [PATCH 1/3] linux-user/arm: Implement __kernel_memory_barrier

[Peter Maydell]

On Mon, 14 Mar 2022 at 04:44, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> This fallback syscall was stubbed out.
> It would only matter for emulating pre-armv6.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH 1/3] linux-user/arm: Implement __kernel_memory_barrier

[Laurent Vivier]

Le 14/03/2022 à 05:43, Richard Henderson a écrit :
> This fallback syscall was stubbed out.
> It would only matter for emulating pre-armv6.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   linux-user/arm/cpu_loop.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/linux-user/arm/cpu_loop.c b/linux-user/arm/cpu_loop.c
> index 032e1ffddf..a0e43b261c 100644
> --- a/linux-user/arm/cpu_loop.c
> +++ b/linux-user/arm/cpu_loop.c
> @@ -158,7 +158,7 @@ do_kernel_trap(CPUARMState *env)
>   
>       switch (env->regs[15]) {
>       case 0xffff0fa0: /* __kernel_memory_barrier */
> -        /* ??? No-op. Will need to do better for SMP.  */
> +        smp_mb();
>           break;
>       case 0xffff0fc0: /* __kernel_cmpxchg */
>            /* XXX: This only works between threads, not between processes.


Applied to my linux-user-for-7.0 branch.

Thanks,
Laurent

[PATCH 2/3] linux-user/arm: Implement __kernel_cmpxchg with host atomics

[Richard Henderson]

The existing implementation using start/end_exclusive
does not provide atomicity across processes.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 linux-user/arm/cpu_loop.c | 85 +++++++++++++++++++++++++++------------
 1 file changed, 60 insertions(+), 25 deletions(-)

diff --git a/linux-user/arm/cpu_loop.c b/linux-user/arm/cpu_loop.c
index a0e43b261c..0122bb34f7 100644
--- a/linux-user/arm/cpu_loop.c
+++ b/linux-user/arm/cpu_loop.c
@@ -75,7 +75,65 @@
         put_user_u16(__x, (gaddr));                     \
     })
 
-/* Commpage handling -- there is no commpage for AArch64 */
+/*
+ * Similar to code in accel/tcg/user-exec.c, but outside the execution loop.
+ * Must be called with mmap_lock.
+ */
+static void *atomic_mmu_lookup(CPUArchState *env, uint32_t addr, int size)
+{
+    int need_flags = PAGE_READ | PAGE_WRITE_ORG | PAGE_VALID;
+    int page_flags;
+
+    /* Enforce guest required alignment.  */
+    if (unlikely(addr & (size - 1))) {
+        force_sig_fault(TARGET_SIGBUS, TARGET_BUS_ADRALN, addr);
+        return NULL;
+    }
+
+    page_flags = page_get_flags(addr);
+    if (unlikely((page_flags & need_flags) != need_flags)) {
+        force_sig_fault(TARGET_SIGSEGV,
+                        page_flags & PAGE_VALID ?
+                        TARGET_SEGV_ACCERR : TARGET_SEGV_MAPERR, addr);
+        return NULL;
+    }
+
+    return g2h(env_cpu(env), addr);
+}
+
+/*
+ * See the Linux kernel's Documentation/arm/kernel_user_helpers.rst
+ * Input:
+ * r0 = oldval
+ * r1 = newval
+ * r2 = pointer to target value
+ *
+ * Output:
+ * r0 = 0 if *ptr was changed, non-0 if no exchange happened
+ * C set if *ptr was changed, clear if no exchange happened
+ */
+static void arm_kernel_cmpxchg32_helper(CPUARMState *env)
+{
+    uint32_t oldval, newval, val, addr, cpsr, *host_addr;
+
+    oldval = env->regs[0];
+    newval = env->regs[1];
+    addr = env->regs[2];
+
+    mmap_lock();
+    host_addr = atomic_mmu_lookup(env, addr, 8);
+    if (!host_addr) {
+        mmap_unlock();
+        return;
+    }
+
+    val = qatomic_cmpxchg__nocheck(host_addr, oldval, newval);
+    mmap_unlock();
+
+    cpsr = (val == oldval) * CPSR_C;
+    cpsr_write(env, cpsr, CPSR_C, CPSRWriteByInstr);
+    env->regs[0] = cpsr ? 0 : -1;
+}
 
 /*
  * See the Linux kernel's Documentation/arm/kernel_user_helpers.txt
@@ -153,36 +211,13 @@ static int
 do_kernel_trap(CPUARMState *env)
 {
     uint32_t addr;
-    uint32_t cpsr;
-    uint32_t val;
 
     switch (env->regs[15]) {
     case 0xffff0fa0: /* __kernel_memory_barrier */
         smp_mb();
         break;
     case 0xffff0fc0: /* __kernel_cmpxchg */
-         /* XXX: This only works between threads, not between processes.
-            It's probably possible to implement this with native host
-            operations. However things like ldrex/strex are much harder so
-            there's not much point trying.  */
-        start_exclusive();
-        cpsr = cpsr_read(env);
-        addr = env->regs[2];
-        /* FIXME: This should SEGV if the access fails.  */
-        if (get_user_u32(val, addr))
-            val = ~env->regs[0];
-        if (val == env->regs[0]) {
-            val = env->regs[1];
-            /* FIXME: Check for segfaults.  */
-            put_user_u32(val, addr);
-            env->regs[0] = 0;
-            cpsr |= CPSR_C;
-        } else {
-            env->regs[0] = -1;
-            cpsr &= ~CPSR_C;
-        }
-        cpsr_write(env, cpsr, CPSR_C, CPSRWriteByInstr);
-        end_exclusive();
+        arm_kernel_cmpxchg32_helper(env);
         break;
     case 0xffff0fe0: /* __kernel_get_tls */
         env->regs[0] = cpu_get_tls(env);
-- 
2.25.1

Re: [PATCH 2/3] linux-user/arm: Implement __kernel_cmpxchg with host atomics

[Peter Maydell]

On Mon, 14 Mar 2022 at 04:44, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> The existing implementation using start/end_exclusive
> does not provide atomicity across processes.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  linux-user/arm/cpu_loop.c | 85 +++++++++++++++++++++++++++------------
>  1 file changed, 60 insertions(+), 25 deletions(-)

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH 2/3] linux-user/arm: Implement __kernel_cmpxchg with host atomics

[Laurent Vivier]

Le 14/03/2022 à 05:43, Richard Henderson a écrit :
> The existing implementation using start/end_exclusive
> does not provide atomicity across processes.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   linux-user/arm/cpu_loop.c | 85 +++++++++++++++++++++++++++------------
>   1 file changed, 60 insertions(+), 25 deletions(-)
> 
> diff --git a/linux-user/arm/cpu_loop.c b/linux-user/arm/cpu_loop.c
> index a0e43b261c..0122bb34f7 100644
> --- a/linux-user/arm/cpu_loop.c
> +++ b/linux-user/arm/cpu_loop.c
> @@ -75,7 +75,65 @@
>           put_user_u16(__x, (gaddr));                     \
>       })
>   
> -/* Commpage handling -- there is no commpage for AArch64 */
> +/*
> + * Similar to code in accel/tcg/user-exec.c, but outside the execution loop.
> + * Must be called with mmap_lock.
> + */
> +static void *atomic_mmu_lookup(CPUArchState *env, uint32_t addr, int size)
> +{
> +    int need_flags = PAGE_READ | PAGE_WRITE_ORG | PAGE_VALID;
> +    int page_flags;
> +
> +    /* Enforce guest required alignment.  */
> +    if (unlikely(addr & (size - 1))) {
> +        force_sig_fault(TARGET_SIGBUS, TARGET_BUS_ADRALN, addr);
> +        return NULL;
> +    }
> +
> +    page_flags = page_get_flags(addr);
> +    if (unlikely((page_flags & need_flags) != need_flags)) {
> +        force_sig_fault(TARGET_SIGSEGV,
> +                        page_flags & PAGE_VALID ?
> +                        TARGET_SEGV_ACCERR : TARGET_SEGV_MAPERR, addr);
> +        return NULL;
> +    }
> +
> +    return g2h(env_cpu(env), addr);
> +}
> +
> +/*
> + * See the Linux kernel's Documentation/arm/kernel_user_helpers.rst
> + * Input:
> + * r0 = oldval
> + * r1 = newval
> + * r2 = pointer to target value
> + *
> + * Output:
> + * r0 = 0 if *ptr was changed, non-0 if no exchange happened
> + * C set if *ptr was changed, clear if no exchange happened
> + */
> +static void arm_kernel_cmpxchg32_helper(CPUARMState *env)
> +{
> +    uint32_t oldval, newval, val, addr, cpsr, *host_addr;
> +
> +    oldval = env->regs[0];
> +    newval = env->regs[1];
> +    addr = env->regs[2];
> +
> +    mmap_lock();
> +    host_addr = atomic_mmu_lookup(env, addr, 8);
> +    if (!host_addr) {
> +        mmap_unlock();
> +        return;
> +    }
> +
> +    val = qatomic_cmpxchg__nocheck(host_addr, oldval, newval);
> +    mmap_unlock();
> +
> +    cpsr = (val == oldval) * CPSR_C;
> +    cpsr_write(env, cpsr, CPSR_C, CPSRWriteByInstr);
> +    env->regs[0] = cpsr ? 0 : -1;
> +}
>   
>   /*
>    * See the Linux kernel's Documentation/arm/kernel_user_helpers.txt
> @@ -153,36 +211,13 @@ static int
>   do_kernel_trap(CPUARMState *env)
>   {
>       uint32_t addr;
> -    uint32_t cpsr;
> -    uint32_t val;
>   
>       switch (env->regs[15]) {
>       case 0xffff0fa0: /* __kernel_memory_barrier */
>           smp_mb();
>           break;
>       case 0xffff0fc0: /* __kernel_cmpxchg */
> -         /* XXX: This only works between threads, not between processes.
> -            It's probably possible to implement this with native host
> -            operations. However things like ldrex/strex are much harder so
> -            there's not much point trying.  */
> -        start_exclusive();
> -        cpsr = cpsr_read(env);
> -        addr = env->regs[2];
> -        /* FIXME: This should SEGV if the access fails.  */
> -        if (get_user_u32(val, addr))
> -            val = ~env->regs[0];
> -        if (val == env->regs[0]) {
> -            val = env->regs[1];
> -            /* FIXME: Check for segfaults.  */
> -            put_user_u32(val, addr);
> -            env->regs[0] = 0;
> -            cpsr |= CPSR_C;
> -        } else {
> -            env->regs[0] = -1;
> -            cpsr &= ~CPSR_C;
> -        }
> -        cpsr_write(env, cpsr, CPSR_C, CPSRWriteByInstr);
> -        end_exclusive();
> +        arm_kernel_cmpxchg32_helper(env);
>           break;
>       case 0xffff0fe0: /* __kernel_get_tls */
>           env->regs[0] = cpu_get_tls(env);

Applied to my linux-user-for-7.0 branch.

Thanks,
Laurent

Re: [PATCH 2/3] linux-user/arm: Implement __kernel_cmpxchg with host atomics

[Laurent Vivier]

Le 22/03/2022 à 12:46, Laurent Vivier a écrit :
> Le 14/03/2022 à 05:43, Richard Henderson a écrit :
>> The existing implementation using start/end_exclusive
>> does not provide atomicity across processes.
>>
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>> ---
>>   linux-user/arm/cpu_loop.c | 85 +++++++++++++++++++++++++++------------
>>   1 file changed, 60 insertions(+), 25 deletions(-)
>>
>> diff --git a/linux-user/arm/cpu_loop.c b/linux-user/arm/cpu_loop.c
>> index a0e43b261c..0122bb34f7 100644
>> --- a/linux-user/arm/cpu_loop.c
>> +++ b/linux-user/arm/cpu_loop.c
>> @@ -75,7 +75,65 @@
>>           put_user_u16(__x, (gaddr));                     \
>>       })
>> -/* Commpage handling -- there is no commpage for AArch64 */
>> +/*
>> + * Similar to code in accel/tcg/user-exec.c, but outside the execution loop.
>> + * Must be called with mmap_lock.
>> + */
>> +static void *atomic_mmu_lookup(CPUArchState *env, uint32_t addr, int size)
>> +{
>> +    int need_flags = PAGE_READ | PAGE_WRITE_ORG | PAGE_VALID;
>> +    int page_flags;
>> +
>> +    /* Enforce guest required alignment.  */
>> +    if (unlikely(addr & (size - 1))) {
>> +        force_sig_fault(TARGET_SIGBUS, TARGET_BUS_ADRALN, addr);
>> +        return NULL;
>> +    }
>> +
>> +    page_flags = page_get_flags(addr);
>> +    if (unlikely((page_flags & need_flags) != need_flags)) {
>> +        force_sig_fault(TARGET_SIGSEGV,
>> +                        page_flags & PAGE_VALID ?
>> +                        TARGET_SEGV_ACCERR : TARGET_SEGV_MAPERR, addr);
>> +        return NULL;
>> +    }
>> +
>> +    return g2h(env_cpu(env), addr);
>> +}
>> +
>> +/*
>> + * See the Linux kernel's Documentation/arm/kernel_user_helpers.rst
>> + * Input:
>> + * r0 = oldval
>> + * r1 = newval
>> + * r2 = pointer to target value
>> + *
>> + * Output:
>> + * r0 = 0 if *ptr was changed, non-0 if no exchange happened
>> + * C set if *ptr was changed, clear if no exchange happened
>> + */
>> +static void arm_kernel_cmpxchg32_helper(CPUARMState *env)
>> +{
>> +    uint32_t oldval, newval, val, addr, cpsr, *host_addr;
>> +
>> +    oldval = env->regs[0];
>> +    newval = env->regs[1];
>> +    addr = env->regs[2];
>> +
>> +    mmap_lock();
>> +    host_addr = atomic_mmu_lookup(env, addr, 8);
>> +    if (!host_addr) {
>> +        mmap_unlock();
>> +        return;
>> +    }
>> +
>> +    val = qatomic_cmpxchg__nocheck(host_addr, oldval, newval);
>> +    mmap_unlock();
>> +
>> +    cpsr = (val == oldval) * CPSR_C;
>> +    cpsr_write(env, cpsr, CPSR_C, CPSRWriteByInstr);
>> +    env->regs[0] = cpsr ? 0 : -1;
>> +}
>>   /*
>>    * See the Linux kernel's Documentation/arm/kernel_user_helpers.txt
>> @@ -153,36 +211,13 @@ static int
>>   do_kernel_trap(CPUARMState *env)
>>   {
>>       uint32_t addr;
>> -    uint32_t cpsr;
>> -    uint32_t val;
>>       switch (env->regs[15]) {
>>       case 0xffff0fa0: /* __kernel_memory_barrier */
>>           smp_mb();
>>           break;
>>       case 0xffff0fc0: /* __kernel_cmpxchg */
>> -         /* XXX: This only works between threads, not between processes.
>> -            It's probably possible to implement this with native host
>> -            operations. However things like ldrex/strex are much harder so
>> -            there's not much point trying.  */
>> -        start_exclusive();
>> -        cpsr = cpsr_read(env);
>> -        addr = env->regs[2];
>> -        /* FIXME: This should SEGV if the access fails.  */
>> -        if (get_user_u32(val, addr))
>> -            val = ~env->regs[0];
>> -        if (val == env->regs[0]) {
>> -            val = env->regs[1];
>> -            /* FIXME: Check for segfaults.  */
>> -            put_user_u32(val, addr);
>> -            env->regs[0] = 0;
>> -            cpsr |= CPSR_C;
>> -        } else {
>> -            env->regs[0] = -1;
>> -            cpsr &= ~CPSR_C;
>> -        }
>> -        cpsr_write(env, cpsr, CPSR_C, CPSRWriteByInstr);
>> -        end_exclusive();
>> +        arm_kernel_cmpxchg32_helper(env);
>>           break;
>>       case 0xffff0fe0: /* __kernel_get_tls */
>>           env->regs[0] = cpu_get_tls(env);
> 
> Applied to my linux-user-for-7.0 branch.
> 
>

I have removed this patch and the following one from the branch because it hangs when executed in an 
armhf/bionic chroot the following example:

cat > /tmp/hello.go <<EOF
package main

import "fmt"

func main() {
         fmt.Println("Hello Google!")
}
EOF

go run /tmp/hello.go

Thanks,
Laurent

Re: [PATCH 2/3] linux-user/arm: Implement __kernel_cmpxchg with host atomics

[Richard Henderson]

On 3/22/22 13:08, Laurent Vivier wrote:
> I have removed this patch and the following one from the branch because it hangs when 
> executed in an armhf/bionic chroot the following example:
> 
> cat > /tmp/hello.go <<EOF
> package main
> 
> import "fmt"
> 
> func main() {
>          fmt.Println("Hello Google!")
> }
> EOF
> 
> go run /tmp/hello.go

I don't see a hang.  I see a SIGBUS, due to a silly typo here:


> +static void arm_kernel_cmpxchg32_helper(CPUARMState *env)
> +{
> +    uint32_t oldval, newval, val, addr, cpsr, *host_addr;
> +
> +    oldval = env->regs[0];
> +    newval = env->regs[1];
> +    addr = env->regs[2];
> +
> +    mmap_lock();
> +    host_addr = atomic_mmu_lookup(env, addr, 8); 

s/8/4/.


r~

[PATCH 3/3] linux-user/arm: Implement __kernel_cmpxchg64 with host atomics

[Richard Henderson]

If CONFIG_ATOMIC64, we can use a host cmpxchg and provide
atomicity across processes; otherwise we have no choice but
to continue using start/end_exclusive.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 linux-user/arm/cpu_loop.c | 79 +++++++++++++++++++--------------------
 1 file changed, 38 insertions(+), 41 deletions(-)

diff --git a/linux-user/arm/cpu_loop.c b/linux-user/arm/cpu_loop.c
index 0122bb34f7..d9651f199f 100644
--- a/linux-user/arm/cpu_loop.c
+++ b/linux-user/arm/cpu_loop.c
@@ -136,7 +136,7 @@ static void arm_kernel_cmpxchg32_helper(CPUARMState *env)
 }
 
 /*
- * See the Linux kernel's Documentation/arm/kernel_user_helpers.txt
+ * See the Linux kernel's Documentation/arm/kernel_user_helpers.rst
  * Input:
  * r0 = pointer to oldval
  * r1 = pointer to newval
@@ -153,57 +153,54 @@ static void arm_kernel_cmpxchg64_helper(CPUARMState *env)
 {
     uint64_t oldval, newval, val;
     uint32_t addr, cpsr;
+    uint64_t *host_addr;
 
-    /* Based on the 32 bit code in do_kernel_trap */
+    addr = env->regs[0];
+    if (get_user_u64(oldval, addr)) {
+        goto segv;
+    }
 
-    /* XXX: This only works between threads, not between processes.
-       It's probably possible to implement this with native host
-       operations. However things like ldrex/strex are much harder so
-       there's not much point trying.  */
-    start_exclusive();
-    cpsr = cpsr_read(env);
+    addr = env->regs[1];
+    if (get_user_u64(newval, addr)) {
+        goto segv;
+    }
+
+    mmap_lock();
     addr = env->regs[2];
-
-    if (get_user_u64(oldval, env->regs[0])) {
-        env->exception.vaddress = env->regs[0];
-        goto segv;
-    };
-
-    if (get_user_u64(newval, env->regs[1])) {
-        env->exception.vaddress = env->regs[1];
-        goto segv;
-    };
-
-    if (get_user_u64(val, addr)) {
-        env->exception.vaddress = addr;
-        goto segv;
+    host_addr = atomic_mmu_lookup(env, addr, 8);
+    if (!host_addr) {
+        mmap_unlock();
+        return;
     }
 
+#ifdef CONFIG_ATOMIC64
+    val = qatomic_cmpxchg__nocheck(host_addr, oldval, newval);
+    cpsr = (val == oldval) * CPSR_C;
+#else
+    /*
+     * This only works between threads, not between processes, but since
+     * the host has no 64-bit cmpxchg, it is the best that we can do.
+     */
+    start_exclusive();
+    val = *host_addr;
     if (val == oldval) {
-        val = newval;
-
-        if (put_user_u64(val, addr)) {
-            env->exception.vaddress = addr;
-            goto segv;
-        };
-
-        env->regs[0] = 0;
-        cpsr |= CPSR_C;
+        *host_addr = newval;
+        cpsr = CPSR_C;
     } else {
-        env->regs[0] = -1;
-        cpsr &= ~CPSR_C;
+        cpsr = 0;
     }
-    cpsr_write(env, cpsr, CPSR_C, CPSRWriteByInstr);
     end_exclusive();
+#endif
+    mmap_unlock();
+
+    cpsr_write(env, cpsr, CPSR_C, CPSRWriteByInstr);
+    env->regs[0] = cpsr ? 0 : -1;
     return;
 
-segv:
-    end_exclusive();
-    /* We get the PC of the entry address - which is as good as anything,
-       on a real kernel what you get depends on which mode it uses. */
-    /* XXX: check env->error_code */
-    force_sig_fault(TARGET_SIGSEGV, TARGET_SEGV_MAPERR,
-                    env->exception.vaddress);
+ segv:
+    force_sig_fault(TARGET_SIGSEGV,
+                    page_get_flags(addr) & PAGE_VALID ?
+                    TARGET_SEGV_ACCERR : TARGET_SEGV_MAPERR, addr);
 }
 
 /* Handle a jump to the kernel code page.  */
-- 
2.25.1

Re: [PATCH 3/3] linux-user/arm: Implement __kernel_cmpxchg64 with host atomics

[Peter Maydell]

On Mon, 14 Mar 2022 at 04:46, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> If CONFIG_ATOMIC64, we can use a host cmpxchg and provide
> atomicity across processes; otherwise we have no choice but
> to continue using start/end_exclusive.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

> -segv:
> -    end_exclusive();
> -    /* We get the PC of the entry address - which is as good as anything,
> -       on a real kernel what you get depends on which mode it uses. */

This comment about the PC the guest signal handler is going
to see when we take the SEGV is still valid, I think ?

> -    /* XXX: check env->error_code */
> -    force_sig_fault(TARGET_SIGSEGV, TARGET_SEGV_MAPERR,
> -                    env->exception.vaddress);
> + segv:
> +    force_sig_fault(TARGET_SIGSEGV,
> +                    page_get_flags(addr) & PAGE_VALID ?
> +                    TARGET_SEGV_ACCERR : TARGET_SEGV_MAPERR, addr);
>  }

Otherwise
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH 3/3] linux-user/arm: Implement __kernel_cmpxchg64 with host atomics

[Richard Henderson]

On 3/15/22 11:18, Peter Maydell wrote:
>> -segv:
>> -    end_exclusive();
>> -    /* We get the PC of the entry address - which is as good as anything,
>> -       on a real kernel what you get depends on which mode it uses. */
> 
> This comment about the PC the guest signal handler is going
> to see when we take the SEGV is still valid, I think ?

Yes.  I guess I could move it to the block comment in front of atomic_mmu_lookup, because 
it would apply to both the SEGV and the BUS raised there.


r~

Re: [PATCH 3/3] linux-user/arm: Implement __kernel_cmpxchg64 with host atomics

[Laurent Vivier]

Le 15/03/2022 à 19:31, Richard Henderson a écrit :
> On 3/15/22 11:18, Peter Maydell wrote:
>>> -segv:
>>> -    end_exclusive();
>>> -    /* We get the PC of the entry address - which is as good as anything,
>>> -       on a real kernel what you get depends on which mode it uses. */
>>
>> This comment about the PC the guest signal handler is going
>> to see when we take the SEGV is still valid, I think ?
> 
> Yes.  I guess I could move it to the block comment in front of atomic_mmu_lookup, because it would 
> apply to both the SEGV and the BUS raised there.


Applied to my linux-user-for-7.0 branch with the following change:

diff --git a/linux-user/arm/cpu_loop.c b/linux-user/arm/cpu_loop.c
index d9651f199f97..0172a7aae7e8 100644
--- a/linux-user/arm/cpu_loop.c
+++ b/linux-user/arm/cpu_loop.c
@@ -78,6 +78,8 @@
  /*
   * Similar to code in accel/tcg/user-exec.c, but outside the execution loop.
   * Must be called with mmap_lock.
+ * We get the PC of the entry address - which is as good as anything,
+ * on a real kernel what you get depends on which mode it uses.
   */
  static void *atomic_mmu_lookup(CPUArchState *env, uint32_t addr, int size)
  {

Let me know if you prefer something else.

Thanks,
Laurent