accel/tcg/atomic_template.h | 32 +++++++++++++---- include/exec/cpu_ldst.h | 2 ++ include/exec/cpu_ldst_useronly_template.h | 14 ++++++-- accel/tcg/cputlb.c | 1 + accel/tcg/user-exec.c | 58 +++++++++++++++++++++++++------ 5 files changed, 87 insertions(+), 20 deletions(-)
When we handle a signal from a fault within a user-only memory helper,
we cannot cpu_restore_state with the PC found within the signal frame.
Use a TLS variable, helper_retaddr, to record the unwind start point
to find the faulting guest insn.
Reported-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
Tested only with a silly test case --
int main()
{
int new = 1, old = 0;
__atomic_compare_exchange((int *)0, &old, &new, 0, 0, 0);
return old;
}
which even before the patch does not fail in the way Peter describes.
As I post this, I remember in theory we should use __atomic_signal_fence
after setting helper_retaddr, but as far as I know this is a no-op on all
supported hosts. It might still generate a compiler barrier though, so it's
worth considering.
r~
---
accel/tcg/atomic_template.h | 32 +++++++++++++----
include/exec/cpu_ldst.h | 2 ++
include/exec/cpu_ldst_useronly_template.h | 14 ++++++--
accel/tcg/cputlb.c | 1 +
accel/tcg/user-exec.c | 58 +++++++++++++++++++++++++------
5 files changed, 87 insertions(+), 20 deletions(-)
diff --git a/accel/tcg/atomic_template.h b/accel/tcg/atomic_template.h
index b400b2a3d3..1c7c17526c 100644
--- a/accel/tcg/atomic_template.h
+++ b/accel/tcg/atomic_template.h
@@ -62,7 +62,9 @@ ABI_TYPE ATOMIC_NAME(cmpxchg)(CPUArchState *env, target_ulong addr,
ABI_TYPE cmpv, ABI_TYPE newv EXTRA_ARGS)
{
DATA_TYPE *haddr = ATOMIC_MMU_LOOKUP;
- return atomic_cmpxchg__nocheck(haddr, cmpv, newv);
+ DATA_TYPE ret = atomic_cmpxchg__nocheck(haddr, cmpv, newv);
+ ATOMIC_MMU_CLEANUP;
+ return ret;
}
#if DATA_SIZE >= 16
@@ -70,6 +72,7 @@ ABI_TYPE ATOMIC_NAME(ld)(CPUArchState *env, target_ulong addr EXTRA_ARGS)
{
DATA_TYPE val, *haddr = ATOMIC_MMU_LOOKUP;
__atomic_load(haddr, &val, __ATOMIC_RELAXED);
+ ATOMIC_MMU_CLEANUP;
return val;
}
@@ -78,13 +81,16 @@ void ATOMIC_NAME(st)(CPUArchState *env, target_ulong addr,
{
DATA_TYPE *haddr = ATOMIC_MMU_LOOKUP;
__atomic_store(haddr, &val, __ATOMIC_RELAXED);
+ ATOMIC_MMU_CLEANUP;
}
#else
ABI_TYPE ATOMIC_NAME(xchg)(CPUArchState *env, target_ulong addr,
ABI_TYPE val EXTRA_ARGS)
{
DATA_TYPE *haddr = ATOMIC_MMU_LOOKUP;
- return atomic_xchg__nocheck(haddr, val);
+ DATA_TYPE ret = atomic_xchg__nocheck(haddr, val);
+ ATOMIC_MMU_CLEANUP;
+ return ret;
}
#define GEN_ATOMIC_HELPER(X) \
@@ -92,8 +98,10 @@ ABI_TYPE ATOMIC_NAME(X)(CPUArchState *env, target_ulong addr, \
ABI_TYPE val EXTRA_ARGS) \
{ \
DATA_TYPE *haddr = ATOMIC_MMU_LOOKUP; \
- return atomic_##X(haddr, val); \
-} \
+ DATA_TYPE ret = atomic_##X(haddr, val); \
+ ATOMIC_MMU_CLEANUP; \
+ return ret; \
+}
GEN_ATOMIC_HELPER(fetch_add)
GEN_ATOMIC_HELPER(fetch_and)
@@ -123,7 +131,9 @@ ABI_TYPE ATOMIC_NAME(cmpxchg)(CPUArchState *env, target_ulong addr,
ABI_TYPE cmpv, ABI_TYPE newv EXTRA_ARGS)
{
DATA_TYPE *haddr = ATOMIC_MMU_LOOKUP;
- return BSWAP(atomic_cmpxchg__nocheck(haddr, BSWAP(cmpv), BSWAP(newv)));
+ DATA_TYPE ret = atomic_cmpxchg__nocheck(haddr, BSWAP(cmpv), BSWAP(newv));
+ ATOMIC_MMU_CLEANUP;
+ return BSWAP(ret);
}
#if DATA_SIZE >= 16
@@ -131,6 +141,7 @@ ABI_TYPE ATOMIC_NAME(ld)(CPUArchState *env, target_ulong addr EXTRA_ARGS)
{
DATA_TYPE val, *haddr = ATOMIC_MMU_LOOKUP;
__atomic_load(haddr, &val, __ATOMIC_RELAXED);
+ ATOMIC_MMU_CLEANUP;
return BSWAP(val);
}
@@ -140,13 +151,16 @@ void ATOMIC_NAME(st)(CPUArchState *env, target_ulong addr,
DATA_TYPE *haddr = ATOMIC_MMU_LOOKUP;
val = BSWAP(val);
__atomic_store(haddr, &val, __ATOMIC_RELAXED);
+ ATOMIC_MMU_CLEANUP;
}
#else
ABI_TYPE ATOMIC_NAME(xchg)(CPUArchState *env, target_ulong addr,
ABI_TYPE val EXTRA_ARGS)
{
DATA_TYPE *haddr = ATOMIC_MMU_LOOKUP;
- return BSWAP(atomic_xchg__nocheck(haddr, BSWAP(val)));
+ ABI_TYPE ret = atomic_xchg__nocheck(haddr, BSWAP(val));
+ ATOMIC_MMU_CLEANUP;
+ return BSWAP(ret);
}
#define GEN_ATOMIC_HELPER(X) \
@@ -154,7 +168,9 @@ ABI_TYPE ATOMIC_NAME(X)(CPUArchState *env, target_ulong addr, \
ABI_TYPE val EXTRA_ARGS) \
{ \
DATA_TYPE *haddr = ATOMIC_MMU_LOOKUP; \
- return BSWAP(atomic_##X(haddr, BSWAP(val))); \
+ DATA_TYPE ret = atomic_##X(haddr, BSWAP(val)); \
+ ATOMIC_MMU_CLEANUP; \
+ return BSWAP(ret); \
}
GEN_ATOMIC_HELPER(fetch_and)
@@ -180,6 +196,7 @@ ABI_TYPE ATOMIC_NAME(fetch_add)(CPUArchState *env, target_ulong addr,
sto = BSWAP(ret + val);
ldn = atomic_cmpxchg__nocheck(haddr, ldo, sto);
if (ldn == ldo) {
+ ATOMIC_MMU_CLEANUP;
return ret;
}
ldo = ldn;
@@ -198,6 +215,7 @@ ABI_TYPE ATOMIC_NAME(add_fetch)(CPUArchState *env, target_ulong addr,
sto = BSWAP(ret);
ldn = atomic_cmpxchg__nocheck(haddr, ldo, sto);
if (ldn == ldo) {
+ ATOMIC_MMU_CLEANUP;
return ret;
}
ldo = ldn;
diff --git a/include/exec/cpu_ldst.h b/include/exec/cpu_ldst.h
index 6eb5fe80dc..191f2e962a 100644
--- a/include/exec/cpu_ldst.h
+++ b/include/exec/cpu_ldst.h
@@ -76,6 +76,8 @@
#if defined(CONFIG_USER_ONLY)
+extern __thread uintptr_t helper_retaddr;
+
/* In user-only mode we provide only the _code and _data accessors. */
#define MEMSUFFIX _data
diff --git a/include/exec/cpu_ldst_useronly_template.h b/include/exec/cpu_ldst_useronly_template.h
index 7b8c7c506e..c168f31bba 100644
--- a/include/exec/cpu_ldst_useronly_template.h
+++ b/include/exec/cpu_ldst_useronly_template.h
@@ -73,7 +73,11 @@ glue(glue(glue(cpu_ld, USUFFIX), MEMSUFFIX), _ra)(CPUArchState *env,
target_ulong ptr,
uintptr_t retaddr)
{
- return glue(glue(cpu_ld, USUFFIX), MEMSUFFIX)(env, ptr);
+ RES_TYPE ret;
+ helper_retaddr = retaddr;
+ ret = glue(glue(cpu_ld, USUFFIX), MEMSUFFIX)(env, ptr);
+ helper_retaddr = 0;
+ return ret;
}
#if DATA_SIZE <= 2
@@ -93,7 +97,11 @@ glue(glue(glue(cpu_lds, SUFFIX), MEMSUFFIX), _ra)(CPUArchState *env,
target_ulong ptr,
uintptr_t retaddr)
{
- return glue(glue(cpu_lds, SUFFIX), MEMSUFFIX)(env, ptr);
+ int ret;
+ helper_retaddr = retaddr;
+ ret = glue(glue(cpu_lds, SUFFIX), MEMSUFFIX)(env, ptr);
+ helper_retaddr = 0;
+ return ret;
}
#endif
@@ -116,7 +124,9 @@ glue(glue(glue(cpu_st, SUFFIX), MEMSUFFIX), _ra)(CPUArchState *env,
RES_TYPE v,
uintptr_t retaddr)
{
+ helper_retaddr = retaddr;
glue(glue(cpu_st, SUFFIX), MEMSUFFIX)(env, ptr, v);
+ helper_retaddr = 0;
}
#endif
diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
index a23919c3a8..d071ca4d14 100644
--- a/accel/tcg/cputlb.c
+++ b/accel/tcg/cputlb.c
@@ -1041,6 +1041,7 @@ static void *atomic_mmu_lookup(CPUArchState *env, target_ulong addr,
#define ATOMIC_NAME(X) \
HELPER(glue(glue(glue(atomic_ ## X, SUFFIX), END), _mmu))
#define ATOMIC_MMU_LOOKUP atomic_mmu_lookup(env, addr, oi, retaddr)
+#define ATOMIC_MMU_CLEANUP do { } while (0)
#define DATA_SIZE 1
#include "atomic_template.h"
diff --git a/accel/tcg/user-exec.c b/accel/tcg/user-exec.c
index 492ea0826c..0324ba8ad1 100644
--- a/accel/tcg/user-exec.c
+++ b/accel/tcg/user-exec.c
@@ -39,6 +39,8 @@
#include <sys/ucontext.h>
#endif
+__thread uintptr_t helper_retaddr;
+
//#define DEBUG_SIGNAL
/* exit the current TB from a signal handler. The host registers are
@@ -62,6 +64,27 @@ static inline int handle_cpu_signal(uintptr_t pc, unsigned long address,
CPUClass *cc;
int ret;
+ /* We must handle PC addresses from two different sources:
+ * a call return address and a signal frame address.
+ *
+ * Within cpu_restore_state_from_tb we assume the former and adjust
+ * the address by -GETPC_ADJ so that the address is within the call
+ * insn so that addr does not accidentally match the beginning of the
+ * next guest insn.
+ *
+ * However, when the PC comes from the signal frame, it points to
+ * the actual faulting host insn and not a call insn. Subtracting
+ * GETPC_ADJ in that case may accidentally match the previous guest insn.
+ *
+ * So for the later case, adjust forward to compensate for what
+ * will be done later by cpu_restore_state_from_tb.
+ */
+ if (helper_retaddr) {
+ pc = helper_retaddr;
+ } else {
+ pc += GETPC_ADJ;
+ }
+
/* For synchronous signals we expect to be coming from the vCPU
* thread (so current_cpu should be valid) and either from running
* code or during translation which can fault as we cross pages.
@@ -84,21 +107,24 @@ static inline int handle_cpu_signal(uintptr_t pc, unsigned long address,
switch (page_unprotect(h2g(address), pc)) {
case 0:
/* Fault not caused by a page marked unwritable to protect
- * cached translations, must be the guest binary's problem
+ * cached translations, must be the guest binary's problem.
*/
break;
case 1:
/* Fault caused by protection of cached translation; TBs
- * invalidated, so resume execution
+ * invalidated, so resume execution. Retain helper_retaddr
+ * for a possible second fault.
*/
return 1;
case 2:
/* Fault caused by protection of cached translation, and the
* currently executing TB was modified and must be exited
- * immediately.
+ * immediately. Clear helper_retaddr for next execution.
*/
+ helper_retaddr = 0;
cpu_exit_tb_from_sighandler(cpu, old_set);
- g_assert_not_reached();
+ /* NORETURN */
+
default:
g_assert_not_reached();
}
@@ -112,17 +138,25 @@ static inline int handle_cpu_signal(uintptr_t pc, unsigned long address,
/* see if it is an MMU fault */
g_assert(cc->handle_mmu_fault);
ret = cc->handle_mmu_fault(cpu, address, is_write, MMU_USER_IDX);
+
+ if (ret == 0) {
+ /* The MMU fault was handled without causing real CPU fault.
+ * Retain helper_retaddr for a possible second fault.
+ */
+ return 1;
+ }
+
+ /* All other paths lead to cpu_exit; clear helper_retaddr
+ * for next execution.
+ */
+ helper_retaddr = 0;
+
if (ret < 0) {
return 0; /* not an MMU fault */
}
- if (ret == 0) {
- return 1; /* the MMU fault was handled without causing real CPU fault */
- }
- /* Now we have a real cpu fault. Since this is the exact location of
- * the exception, we must undo the adjustment done by cpu_restore_state
- * for handling call return addresses. */
- cpu_restore_state(cpu, pc + GETPC_ADJ);
+ /* Now we have a real cpu fault. */
+ cpu_restore_state(cpu, pc);
sigprocmask(SIG_SETMASK, old_set, NULL);
cpu_loop_exit(cpu);
@@ -585,11 +619,13 @@ static void *atomic_mmu_lookup(CPUArchState *env, target_ulong addr,
if (unlikely(addr & (size - 1))) {
cpu_loop_exit_atomic(ENV_GET_CPU(env), retaddr);
}
+ helper_retaddr = retaddr;
return g2h(addr);
}
/* Macro to call the above, with local variables from the use context. */
#define ATOMIC_MMU_LOOKUP atomic_mmu_lookup(env, addr, DATA_SIZE, GETPC())
+#define ATOMIC_MMU_CLEANUP do { helper_retaddr = 0; } while (0)
#define ATOMIC_NAME(X) HELPER(glue(glue(atomic_ ## X, SUFFIX), END))
#define EXTRA_ARGS
--
2.13.6
On 14 November 2017 at 09:42, Richard Henderson
<richard.henderson@linaro.org> wrote:
> When we handle a signal from a fault within a user-only memory helper,
> we cannot cpu_restore_state with the PC found within the signal frame.
> Use a TLS variable, helper_retaddr, to record the unwind start point
> to find the faulting guest insn.
>
> Reported-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>
> Tested only with a silly test case --
>
> int main()
> {
> int new = 1, old = 0;
> __atomic_compare_exchange((int *)0, &old, &new, 0, 0, 0);
> return old;
> }
>
> which even before the patch does not fail in the way Peter describes.
Probably because the 32-bit atomic compare gets done inline, rather than
via a helper. Here's a test case which does fail:
===begin===
/* segv on atomic access: check we report correct PC
*
* Build with:
* aarch64-linux-gnu-gcc -g -Wall -static -o segv-atomic segv-atomic.c
*
* Copyright (c) 2017 Linaro Ltd
* License: 2-clause BSD.
*/
#include <stdlib.h>
#include <signal.h>
#include <sys/mman.h>
#include <stdio.h>
#include <string.h>
#include <sys/ucontext.h>
#include <stdint.h>
#include <inttypes.h>
static int sigcount = 0;
static unsigned char *mem = 0;
void sighandler(int s, siginfo_t *info, void *puc)
{
struct ucontext *uc = puc;
uint64_t pc = uc->uc_mcontext.pc;
uint64_t faultaddr = uc->uc_mcontext.fault_address;
printf("in sighandler, pc = %" PRIx64 " faultaddr %" PRIx64 "\n",
pc, faultaddr);
sigcount++;
if (sigcount == 2) {
printf("too many signals\n");
exit(1);
}
/* Advance past the insn */
uc->uc_mcontext.pc += 4;
}
int main(void)
{
struct sigaction sa;
int x;
mem = mmap(0, 4096, PROT_READ | PROT_WRITE,
MAP_PRIVATE|MAP_ANONYMOUS, -1, 0);
printf("memory at %p\n", mem);
memset(&sa, 0, sizeof(sa));
sa.sa_sigaction = sighandler;
sa.sa_flags = SA_SIGINFO;
sigaction(SIGSEGV, &sa, NULL);
memset(mem, 42, 4096);
mprotect(mem, 4096, PROT_READ);
/* This should fault the first time around, and then
* the segv handler will advance the PC by 4 to skip the insn.
* We surround it with some movs so we know the insn
* is not at the start of a TB and so we can tell if we
* correctly skipped it.
*/
printf("Trying atomic stxp...\n");
__asm__ volatile("mov %[x], #1\n"
"ldxp x4, x3, [%[addr]]\n"
"mov %[x], #2\n"
"stxp w5, x4, x3, [%[addr]]\n"
"mov %[x], #3\n"
: [x] "=&r" (x)
: [addr] "r" (mem)
: "w5", "x4", "x3", "memory");
printf("...done, with x %d\n", x);
return 0;
}
===endit===
On real hardware:
pm215@gcc115:~$ ./segv-atomic
memory at 0x7f99810000
Trying atomic stlxr...
in sighandler, pc = 4007f0 faultaddr 7f99810000
...done, with x 3
On QEMU linux-user:
$ ~/linaro/qemu-from-laptop/qemu/build/all-linux-static/aarch64-linux-user/qemu-aarch64
-d in_asm,out_asm,cpu,exec,int -D /tmp/atomic.log ./segv-atomic
memory at 0x4000801000
Trying atomic stlxr...
in sighandler, pc = 4007d8 faultaddr 4000801000
in sighandler, pc = 4007e0 faultaddr a32
too many signals
and the log file shows that the relevant TB is:
0x004007d8: d0000460 adrp x0, #0x48e000
0x004007dc: 9128c000 add x0, x0, #0xa30
0x004007e0: f9400001 ldr x1, [x0]
0x004007e4: d2800020 movz x0, #0x1
0x004007e8: c87f0c24 ldxp x4, x3, [x1]
0x004007ec: d2800040 movz x0, #0x2
0x004007f0: c8250c24 stxp w5, x4, x3, [x1]
0x004007f4: d2800060 movz x0, #0x3
0x004007f8: b9001fa0 str w0, [x29, #0x1c]
0x004007fc: f00002e0 adrp x0, #0x45f000
0x00400800: 91044000 add x0, x0, #0x110
0x00400804: b9401fa1 ldr w1, [x29, #0x1c]
0x00400808: 940018b6 bl #0x406ae0
so the stxp faults but the PC reported to the handler is the start
of the TB, at which point it goes off into the weeds because we
restart in the wrong place.
Unfortunately it still fails even with your patch...haven't
investigated why yet.
thanks
-- PMM
Richard Henderson <richard.henderson@linaro.org> writes:
> When we handle a signal from a fault within a user-only memory helper,
> we cannot cpu_restore_state with the PC found within the signal frame.
> Use a TLS variable, helper_retaddr, to record the unwind start point
> to find the faulting guest insn.
>
> Reported-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>
> Tested only with a silly test case --
>
> int main()
> {
> int new = 1, old = 0;
> __atomic_compare_exchange((int *)0, &old, &new, 0, 0, 0);
> return old;
> }
>
> which even before the patch does not fail in the way Peter describes.
>
> As I post this, I remember in theory we should use __atomic_signal_fence
> after setting helper_retaddr, but as far as I know this is a no-op on all
> supported hosts. It might still generate a compiler barrier though, so it's
> worth considering.
>
>
> r~
> ---
>
> accel/tcg/atomic_template.h | 32 +++++++++++++----
> include/exec/cpu_ldst.h | 2 ++
> include/exec/cpu_ldst_useronly_template.h | 14 ++++++--
> accel/tcg/cputlb.c | 1 +
> accel/tcg/user-exec.c | 58 +++++++++++++++++++++++++------
> 5 files changed, 87 insertions(+), 20 deletions(-)
>
> diff --git a/accel/tcg/atomic_template.h b/accel/tcg/atomic_template.h
> index b400b2a3d3..1c7c17526c 100644
> --- a/accel/tcg/atomic_template.h
> +++ b/accel/tcg/atomic_template.h
> @@ -62,7 +62,9 @@ ABI_TYPE ATOMIC_NAME(cmpxchg)(CPUArchState *env, target_ulong addr,
> ABI_TYPE cmpv, ABI_TYPE newv EXTRA_ARGS)
> {
> DATA_TYPE *haddr = ATOMIC_MMU_LOOKUP;
> - return atomic_cmpxchg__nocheck(haddr, cmpv, newv);
> + DATA_TYPE ret = atomic_cmpxchg__nocheck(haddr, cmpv, newv);
> + ATOMIC_MMU_CLEANUP;
> + return ret;
> }
>
> #if DATA_SIZE >= 16
> @@ -70,6 +72,7 @@ ABI_TYPE ATOMIC_NAME(ld)(CPUArchState *env, target_ulong addr EXTRA_ARGS)
> {
> DATA_TYPE val, *haddr = ATOMIC_MMU_LOOKUP;
> __atomic_load(haddr, &val, __ATOMIC_RELAXED);
> + ATOMIC_MMU_CLEANUP;
> return val;
> }
>
> @@ -78,13 +81,16 @@ void ATOMIC_NAME(st)(CPUArchState *env, target_ulong addr,
> {
> DATA_TYPE *haddr = ATOMIC_MMU_LOOKUP;
> __atomic_store(haddr, &val, __ATOMIC_RELAXED);
> + ATOMIC_MMU_CLEANUP;
> }
> #else
> ABI_TYPE ATOMIC_NAME(xchg)(CPUArchState *env, target_ulong addr,
> ABI_TYPE val EXTRA_ARGS)
> {
> DATA_TYPE *haddr = ATOMIC_MMU_LOOKUP;
> - return atomic_xchg__nocheck(haddr, val);
> + DATA_TYPE ret = atomic_xchg__nocheck(haddr, val);
> + ATOMIC_MMU_CLEANUP;
> + return ret;
> }
>
> #define GEN_ATOMIC_HELPER(X) \
> @@ -92,8 +98,10 @@ ABI_TYPE ATOMIC_NAME(X)(CPUArchState *env, target_ulong addr, \
> ABI_TYPE val EXTRA_ARGS) \
> { \
> DATA_TYPE *haddr = ATOMIC_MMU_LOOKUP; \
> - return atomic_##X(haddr, val); \
> -} \
> + DATA_TYPE ret = atomic_##X(haddr, val); \
> + ATOMIC_MMU_CLEANUP; \
> + return ret; \
> +}
>
> GEN_ATOMIC_HELPER(fetch_add)
> GEN_ATOMIC_HELPER(fetch_and)
> @@ -123,7 +131,9 @@ ABI_TYPE ATOMIC_NAME(cmpxchg)(CPUArchState *env, target_ulong addr,
> ABI_TYPE cmpv, ABI_TYPE newv EXTRA_ARGS)
> {
> DATA_TYPE *haddr = ATOMIC_MMU_LOOKUP;
> - return BSWAP(atomic_cmpxchg__nocheck(haddr, BSWAP(cmpv), BSWAP(newv)));
> + DATA_TYPE ret = atomic_cmpxchg__nocheck(haddr, BSWAP(cmpv), BSWAP(newv));
> + ATOMIC_MMU_CLEANUP;
> + return BSWAP(ret);
> }
>
> #if DATA_SIZE >= 16
> @@ -131,6 +141,7 @@ ABI_TYPE ATOMIC_NAME(ld)(CPUArchState *env, target_ulong addr EXTRA_ARGS)
> {
> DATA_TYPE val, *haddr = ATOMIC_MMU_LOOKUP;
> __atomic_load(haddr, &val, __ATOMIC_RELAXED);
> + ATOMIC_MMU_CLEANUP;
> return BSWAP(val);
> }
>
> @@ -140,13 +151,16 @@ void ATOMIC_NAME(st)(CPUArchState *env, target_ulong addr,
> DATA_TYPE *haddr = ATOMIC_MMU_LOOKUP;
> val = BSWAP(val);
> __atomic_store(haddr, &val, __ATOMIC_RELAXED);
> + ATOMIC_MMU_CLEANUP;
> }
> #else
> ABI_TYPE ATOMIC_NAME(xchg)(CPUArchState *env, target_ulong addr,
> ABI_TYPE val EXTRA_ARGS)
> {
> DATA_TYPE *haddr = ATOMIC_MMU_LOOKUP;
> - return BSWAP(atomic_xchg__nocheck(haddr, BSWAP(val)));
> + ABI_TYPE ret = atomic_xchg__nocheck(haddr, BSWAP(val));
> + ATOMIC_MMU_CLEANUP;
> + return BSWAP(ret);
> }
>
> #define GEN_ATOMIC_HELPER(X) \
> @@ -154,7 +168,9 @@ ABI_TYPE ATOMIC_NAME(X)(CPUArchState *env, target_ulong addr, \
> ABI_TYPE val EXTRA_ARGS) \
> { \
> DATA_TYPE *haddr = ATOMIC_MMU_LOOKUP; \
> - return BSWAP(atomic_##X(haddr, BSWAP(val))); \
> + DATA_TYPE ret = atomic_##X(haddr, BSWAP(val)); \
> + ATOMIC_MMU_CLEANUP; \
> + return BSWAP(ret); \
> }
>
> GEN_ATOMIC_HELPER(fetch_and)
> @@ -180,6 +196,7 @@ ABI_TYPE ATOMIC_NAME(fetch_add)(CPUArchState *env, target_ulong addr,
> sto = BSWAP(ret + val);
> ldn = atomic_cmpxchg__nocheck(haddr, ldo, sto);
> if (ldn == ldo) {
> + ATOMIC_MMU_CLEANUP;
> return ret;
> }
> ldo = ldn;
> @@ -198,6 +215,7 @@ ABI_TYPE ATOMIC_NAME(add_fetch)(CPUArchState *env, target_ulong addr,
> sto = BSWAP(ret);
> ldn = atomic_cmpxchg__nocheck(haddr, ldo, sto);
> if (ldn == ldo) {
> + ATOMIC_MMU_CLEANUP;
> return ret;
> }
> ldo = ldn;
> diff --git a/include/exec/cpu_ldst.h b/include/exec/cpu_ldst.h
> index 6eb5fe80dc..191f2e962a 100644
> --- a/include/exec/cpu_ldst.h
> +++ b/include/exec/cpu_ldst.h
> @@ -76,6 +76,8 @@
>
> #if defined(CONFIG_USER_ONLY)
>
> +extern __thread uintptr_t helper_retaddr;
> +
> /* In user-only mode we provide only the _code and _data accessors. */
>
> #define MEMSUFFIX _data
> diff --git a/include/exec/cpu_ldst_useronly_template.h b/include/exec/cpu_ldst_useronly_template.h
> index 7b8c7c506e..c168f31bba 100644
> --- a/include/exec/cpu_ldst_useronly_template.h
> +++ b/include/exec/cpu_ldst_useronly_template.h
> @@ -73,7 +73,11 @@ glue(glue(glue(cpu_ld, USUFFIX), MEMSUFFIX), _ra)(CPUArchState *env,
> target_ulong ptr,
> uintptr_t retaddr)
> {
> - return glue(glue(cpu_ld, USUFFIX), MEMSUFFIX)(env, ptr);
> + RES_TYPE ret;
> + helper_retaddr = retaddr;
> + ret = glue(glue(cpu_ld, USUFFIX), MEMSUFFIX)(env, ptr);
> + helper_retaddr = 0;
> + return ret;
> }
>
> #if DATA_SIZE <= 2
> @@ -93,7 +97,11 @@ glue(glue(glue(cpu_lds, SUFFIX), MEMSUFFIX), _ra)(CPUArchState *env,
> target_ulong ptr,
> uintptr_t retaddr)
> {
> - return glue(glue(cpu_lds, SUFFIX), MEMSUFFIX)(env, ptr);
> + int ret;
> + helper_retaddr = retaddr;
> + ret = glue(glue(cpu_lds, SUFFIX), MEMSUFFIX)(env, ptr);
> + helper_retaddr = 0;
> + return ret;
> }
> #endif
>
> @@ -116,7 +124,9 @@ glue(glue(glue(cpu_st, SUFFIX), MEMSUFFIX), _ra)(CPUArchState *env,
> RES_TYPE v,
> uintptr_t retaddr)
> {
> + helper_retaddr = retaddr;
> glue(glue(cpu_st, SUFFIX), MEMSUFFIX)(env, ptr, v);
> + helper_retaddr = 0;
> }
> #endif
>
> diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
> index a23919c3a8..d071ca4d14 100644
> --- a/accel/tcg/cputlb.c
> +++ b/accel/tcg/cputlb.c
> @@ -1041,6 +1041,7 @@ static void *atomic_mmu_lookup(CPUArchState *env, target_ulong addr,
> #define ATOMIC_NAME(X) \
> HELPER(glue(glue(glue(atomic_ ## X, SUFFIX), END), _mmu))
> #define ATOMIC_MMU_LOOKUP atomic_mmu_lookup(env, addr, oi, retaddr)
> +#define ATOMIC_MMU_CLEANUP do { } while (0)
>
> #define DATA_SIZE 1
> #include "atomic_template.h"
> diff --git a/accel/tcg/user-exec.c b/accel/tcg/user-exec.c
> index 492ea0826c..0324ba8ad1 100644
> --- a/accel/tcg/user-exec.c
> +++ b/accel/tcg/user-exec.c
> @@ -39,6 +39,8 @@
> #include <sys/ucontext.h>
> #endif
>
> +__thread uintptr_t helper_retaddr;
> +
> //#define DEBUG_SIGNAL
>
> /* exit the current TB from a signal handler. The host registers are
> @@ -62,6 +64,27 @@ static inline int handle_cpu_signal(uintptr_t pc, unsigned long address,
> CPUClass *cc;
> int ret;
>
> + /* We must handle PC addresses from two different sources:
> + * a call return address and a signal frame address.
> + *
> + * Within cpu_restore_state_from_tb we assume the former and adjust
> + * the address by -GETPC_ADJ so that the address is within the call
> + * insn so that addr does not accidentally match the beginning of the
> + * next guest insn.
> + *
> + * However, when the PC comes from the signal frame, it points to
> + * the actual faulting host insn and not a call insn. Subtracting
> + * GETPC_ADJ in that case may accidentally match the previous guest insn.
> + *
> + * So for the later case, adjust forward to compensate for what
> + * will be done later by cpu_restore_state_from_tb.
> + */
> + if (helper_retaddr) {
> + pc = helper_retaddr;
> + } else {
> + pc += GETPC_ADJ;
> + }
> +
> /* For synchronous signals we expect to be coming from the vCPU
> * thread (so current_cpu should be valid) and either from running
> * code or during translation which can fault as we cross pages.
> @@ -84,21 +107,24 @@ static inline int handle_cpu_signal(uintptr_t pc, unsigned long address,
> switch (page_unprotect(h2g(address), pc)) {
> case 0:
> /* Fault not caused by a page marked unwritable to protect
> - * cached translations, must be the guest binary's problem
> + * cached translations, must be the guest binary's problem.
> */
> break;
> case 1:
> /* Fault caused by protection of cached translation; TBs
> - * invalidated, so resume execution
> + * invalidated, so resume execution. Retain helper_retaddr
> + * for a possible second fault.
> */
> return 1;
> case 2:
> /* Fault caused by protection of cached translation, and the
> * currently executing TB was modified and must be exited
> - * immediately.
> + * immediately. Clear helper_retaddr for next execution.
> */
> + helper_retaddr = 0;
> cpu_exit_tb_from_sighandler(cpu, old_set);
> - g_assert_not_reached();
> + /* NORETURN */
> +
> default:
> g_assert_not_reached();
> }
> @@ -112,17 +138,25 @@ static inline int handle_cpu_signal(uintptr_t pc, unsigned long address,
> /* see if it is an MMU fault */
> g_assert(cc->handle_mmu_fault);
> ret = cc->handle_mmu_fault(cpu, address, is_write, MMU_USER_IDX);
> +
> + if (ret == 0) {
> + /* The MMU fault was handled without causing real CPU fault.
> + * Retain helper_retaddr for a possible second fault.
> + */
> + return 1;
> + }
> +
> + /* All other paths lead to cpu_exit; clear helper_retaddr
> + * for next execution.
> + */
> + helper_retaddr = 0;
> +
> if (ret < 0) {
> return 0; /* not an MMU fault */
> }
> - if (ret == 0) {
> - return 1; /* the MMU fault was handled without causing real CPU fault */
> - }
>
> - /* Now we have a real cpu fault. Since this is the exact location of
> - * the exception, we must undo the adjustment done by cpu_restore_state
> - * for handling call return addresses. */
> - cpu_restore_state(cpu, pc + GETPC_ADJ);
> + /* Now we have a real cpu fault. */
> + cpu_restore_state(cpu, pc);
I can't help thinking when we get it wrong we should be doing something
here, maybe a LOG_UNIMP? Otherwise we silently fail or at least the
user-space falls off a cliff later.
Anyway, other than that minor nit:
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Tested-by: Alex Bennée <alex.bennee@linaro.org>
>
> sigprocmask(SIG_SETMASK, old_set, NULL);
> cpu_loop_exit(cpu);
> @@ -585,11 +619,13 @@ static void *atomic_mmu_lookup(CPUArchState *env, target_ulong addr,
> if (unlikely(addr & (size - 1))) {
> cpu_loop_exit_atomic(ENV_GET_CPU(env), retaddr);
> }
> + helper_retaddr = retaddr;
> return g2h(addr);
> }
>
> /* Macro to call the above, with local variables from the use context. */
> #define ATOMIC_MMU_LOOKUP atomic_mmu_lookup(env, addr, DATA_SIZE, GETPC())
> +#define ATOMIC_MMU_CLEANUP do { helper_retaddr = 0; } while (0)
>
> #define ATOMIC_NAME(X) HELPER(glue(glue(atomic_ ## X, SUFFIX), END))
> #define EXTRA_ARGS
--
Alex Bennée
On 11/14/2017 05:09 PM, Alex Bennée wrote: >> - /* Now we have a real cpu fault. Since this is the exact location of >> - * the exception, we must undo the adjustment done by cpu_restore_state >> - * for handling call return addresses. */ >> - cpu_restore_state(cpu, pc + GETPC_ADJ); >> + /* Now we have a real cpu fault. */ >> + cpu_restore_state(cpu, pc); > > I can't help thinking when we get it wrong we should be doing something > here, maybe a LOG_UNIMP? Otherwise we silently fail or at least the > user-space falls off a cliff later. Oh we silently get it wrong in so many ways. E.g. zero callers of cpu_restore_state_from_tb check its return status. Anyway, I think this sort of cleanup has to wait til next cycle. r~
We use raw memory primitives along the !parallel_cpus paths in order to
simplify the endianness handling. Because of that, we did not benefit
from the generic changes to cpu_ldst_user_only_template.h.
The simplest fix is to manipulate helper_retaddr here.
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
target/arm/helper-a64.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/target/arm/helper-a64.c b/target/arm/helper-a64.c
index d0e435ca4b..96a3ecf707 100644
--- a/target/arm/helper-a64.c
+++ b/target/arm/helper-a64.c
@@ -456,6 +456,8 @@ static uint64_t do_paired_cmpxchg64_le(CPUARMState *env, uint64_t addr,
#ifdef CONFIG_USER_ONLY
/* ??? Enforce alignment. */
uint64_t *haddr = g2h(addr);
+
+ helper_retaddr = ra;
o0 = ldq_le_p(haddr + 0);
o1 = ldq_le_p(haddr + 1);
oldv = int128_make128(o0, o1);
@@ -465,6 +467,7 @@ static uint64_t do_paired_cmpxchg64_le(CPUARMState *env, uint64_t addr,
stq_le_p(haddr + 0, int128_getlo(newv));
stq_le_p(haddr + 1, int128_gethi(newv));
}
+ helper_retaddr = 0;
#else
int mem_idx = cpu_mmu_index(env, false);
TCGMemOpIdx oi0 = make_memop_idx(MO_LEQ | MO_ALIGN_16, mem_idx);
@@ -523,6 +526,8 @@ static uint64_t do_paired_cmpxchg64_be(CPUARMState *env, uint64_t addr,
#ifdef CONFIG_USER_ONLY
/* ??? Enforce alignment. */
uint64_t *haddr = g2h(addr);
+
+ helper_retaddr = ra;
o1 = ldq_be_p(haddr + 0);
o0 = ldq_be_p(haddr + 1);
oldv = int128_make128(o0, o1);
@@ -532,6 +537,7 @@ static uint64_t do_paired_cmpxchg64_be(CPUARMState *env, uint64_t addr,
stq_be_p(haddr + 0, int128_gethi(newv));
stq_be_p(haddr + 1, int128_getlo(newv));
}
+ helper_retaddr = 0;
#else
int mem_idx = cpu_mmu_index(env, false);
TCGMemOpIdx oi0 = make_memop_idx(MO_BEQ | MO_ALIGN_16, mem_idx);
--
2.12.0
On 14 November 2017 at 12:53, Richard Henderson
<richard.henderson@linaro.org> wrote:
> We use raw memory primitives along the !parallel_cpus paths in order to
> simplify the endianness handling. Because of that, we did not benefit
> from the generic changes to cpu_ldst_user_only_template.h.
>
> The simplest fix is to manipulate helper_retaddr here.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
> target/arm/helper-a64.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/target/arm/helper-a64.c b/target/arm/helper-a64.c
> index d0e435ca4b..96a3ecf707 100644
> --- a/target/arm/helper-a64.c
> +++ b/target/arm/helper-a64.c
> @@ -456,6 +456,8 @@ static uint64_t do_paired_cmpxchg64_le(CPUARMState *env, uint64_t addr,
> #ifdef CONFIG_USER_ONLY
> /* ??? Enforce alignment. */
> uint64_t *haddr = g2h(addr);
> +
> + helper_retaddr = ra;
> o0 = ldq_le_p(haddr + 0);
> o1 = ldq_le_p(haddr + 1);
> oldv = int128_make128(o0, o1);
> @@ -465,6 +467,7 @@ static uint64_t do_paired_cmpxchg64_le(CPUARMState *env, uint64_t addr,
> stq_le_p(haddr + 0, int128_getlo(newv));
> stq_le_p(haddr + 1, int128_gethi(newv));
> }
> + helper_retaddr = 0;
> #else
> int mem_idx = cpu_mmu_index(env, false);
> TCGMemOpIdx oi0 = make_memop_idx(MO_LEQ | MO_ALIGN_16, mem_idx);
> @@ -523,6 +526,8 @@ static uint64_t do_paired_cmpxchg64_be(CPUARMState *env, uint64_t addr,
> #ifdef CONFIG_USER_ONLY
> /* ??? Enforce alignment. */
> uint64_t *haddr = g2h(addr);
> +
> + helper_retaddr = ra;
> o1 = ldq_be_p(haddr + 0);
> o0 = ldq_be_p(haddr + 1);
> oldv = int128_make128(o0, o1);
> @@ -532,6 +537,7 @@ static uint64_t do_paired_cmpxchg64_be(CPUARMState *env, uint64_t addr,
> stq_be_p(haddr + 0, int128_gethi(newv));
> stq_be_p(haddr + 1, int128_getlo(newv));
> }
> + helper_retaddr = 0;
> #else
> int mem_idx = cpu_mmu_index(env, false);
> TCGMemOpIdx oi0 = make_memop_idx(MO_BEQ | MO_ALIGN_16, mem_idx);
> --
> 2.12.0
These functions aren't directly called from guest code, so needs
this fixup to work with an unoptimised build:
diff --git a/target/arm/helper-a64.c b/target/arm/helper-a64.c
index 96a3ecf..b84ebca 100644
--- a/target/arm/helper-a64.c
+++ b/target/arm/helper-a64.c
@@ -432,9 +432,8 @@ uint64_t HELPER(crc32c_64)(uint64_t acc, uint64_t
val, uint32_t bytes)
/* Returns 0 on success; 1 otherwise. */
static uint64_t do_paired_cmpxchg64_le(CPUARMState *env, uint64_t addr,
uint64_t new_lo, uint64_t new_hi,
- bool parallel)
+ bool parallel, uintptr_t ra)
{
- uintptr_t ra = GETPC();
Int128 oldv, cmpv, newv;
bool success;
@@ -491,20 +490,19 @@ static uint64_t
do_paired_cmpxchg64_le(CPUARMState *env, uint64_t addr,
uint64_t HELPER(paired_cmpxchg64_le)(CPUARMState *env, uint64_t addr,
uint64_t new_lo, uint64_t new_hi)
{
- return do_paired_cmpxchg64_le(env, addr, new_lo, new_hi, false);
+ return do_paired_cmpxchg64_le(env, addr, new_lo, new_hi, false, GETPC());
}
uint64_t HELPER(paired_cmpxchg64_le_parallel)(CPUARMState *env, uint64_t addr,
uint64_t new_lo, uint64_t new_hi)
{
- return do_paired_cmpxchg64_le(env, addr, new_lo, new_hi, true);
+ return do_paired_cmpxchg64_le(env, addr, new_lo, new_hi, true, GETPC());
}
static uint64_t do_paired_cmpxchg64_be(CPUARMState *env, uint64_t addr,
uint64_t new_lo, uint64_t new_hi,
- bool parallel)
+ bool parallel, uintptr_t ra)
{
- uintptr_t ra = GETPC();
Int128 oldv, cmpv, newv;
bool success;
@@ -561,11 +559,11 @@ static uint64_t
do_paired_cmpxchg64_be(CPUARMState *env, uint64_t addr,
uint64_t HELPER(paired_cmpxchg64_be)(CPUARMState *env, uint64_t addr,
uint64_t new_lo, uint64_t new_hi)
{
- return do_paired_cmpxchg64_be(env, addr, new_lo, new_hi, false);
+ return do_paired_cmpxchg64_be(env, addr, new_lo, new_hi, false, GETPC());
}
uint64_t HELPER(paired_cmpxchg64_be_parallel)(CPUARMState *env, uint64_t addr,
uint64_t new_lo, uint64_t new_hi)
{
- return do_paired_cmpxchg64_be(env, addr, new_lo, new_hi, true);
+ return do_paired_cmpxchg64_be(env, addr, new_lo, new_hi, true, GETPC());
}
thanks
-- PMM
Richard Henderson <richard.henderson@linaro.org> writes: > We use raw memory primitives along the !parallel_cpus paths in order to > simplify the endianness handling. Because of that, we did not benefit > from the generic changes to cpu_ldst_user_only_template.h. > > The simplest fix is to manipulate helper_retaddr here. > > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > target/arm/helper-a64.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/target/arm/helper-a64.c b/target/arm/helper-a64.c > index d0e435ca4b..96a3ecf707 100644 > --- a/target/arm/helper-a64.c > +++ b/target/arm/helper-a64.c > @@ -456,6 +456,8 @@ static uint64_t do_paired_cmpxchg64_le(CPUARMState *env, uint64_t addr, > #ifdef CONFIG_USER_ONLY > /* ??? Enforce alignment. */ > uint64_t *haddr = g2h(addr); > + > + helper_retaddr = ra; > o0 = ldq_le_p(haddr + 0); > o1 = ldq_le_p(haddr + 1); > oldv = int128_make128(o0, o1); > @@ -465,6 +467,7 @@ static uint64_t do_paired_cmpxchg64_le(CPUARMState *env, uint64_t addr, > stq_le_p(haddr + 0, int128_getlo(newv)); > stq_le_p(haddr + 1, int128_gethi(newv)); > } > + helper_retaddr = 0; > #else > int mem_idx = cpu_mmu_index(env, false); > TCGMemOpIdx oi0 = make_memop_idx(MO_LEQ | MO_ALIGN_16, mem_idx); > @@ -523,6 +526,8 @@ static uint64_t do_paired_cmpxchg64_be(CPUARMState *env, uint64_t addr, > #ifdef CONFIG_USER_ONLY > /* ??? Enforce alignment. */ > uint64_t *haddr = g2h(addr); > + > + helper_retaddr = ra; > o1 = ldq_be_p(haddr + 0); > o0 = ldq_be_p(haddr + 1); > oldv = int128_make128(o0, o1); > @@ -532,6 +537,7 @@ static uint64_t do_paired_cmpxchg64_be(CPUARMState *env, uint64_t addr, > stq_be_p(haddr + 0, int128_gethi(newv)); > stq_be_p(haddr + 1, int128_getlo(newv)); > } > + helper_retaddr = 0; > #else > int mem_idx = cpu_mmu_index(env, false); > TCGMemOpIdx oi0 = make_memop_idx(MO_BEQ | MO_ALIGN_16, mem_idx); Reviewed-by: Alex Bennée <alex.bennee@linaro.org> -- Alex Bennée
Use of GETPC must be restricted to those functions that are
directly called from TCG generated code.
Fixes: 2399d4e7cec22ecf1c51062d2ebfd45220dbaace
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
target/arm/helper-a64.c | 14 ++++++--------
1 file changed, 6 insertions(+), 8 deletions(-)
diff --git a/target/arm/helper-a64.c b/target/arm/helper-a64.c
index 96a3ecf707..b84ebcae6e 100644
--- a/target/arm/helper-a64.c
+++ b/target/arm/helper-a64.c
@@ -432,9 +432,8 @@ uint64_t HELPER(crc32c_64)(uint64_t acc, uint64_t val, uint32_t bytes)
/* Returns 0 on success; 1 otherwise. */
static uint64_t do_paired_cmpxchg64_le(CPUARMState *env, uint64_t addr,
uint64_t new_lo, uint64_t new_hi,
- bool parallel)
+ bool parallel, uintptr_t ra)
{
- uintptr_t ra = GETPC();
Int128 oldv, cmpv, newv;
bool success;
@@ -491,20 +490,19 @@ static uint64_t do_paired_cmpxchg64_le(CPUARMState *env, uint64_t addr,
uint64_t HELPER(paired_cmpxchg64_le)(CPUARMState *env, uint64_t addr,
uint64_t new_lo, uint64_t new_hi)
{
- return do_paired_cmpxchg64_le(env, addr, new_lo, new_hi, false);
+ return do_paired_cmpxchg64_le(env, addr, new_lo, new_hi, false, GETPC());
}
uint64_t HELPER(paired_cmpxchg64_le_parallel)(CPUARMState *env, uint64_t addr,
uint64_t new_lo, uint64_t new_hi)
{
- return do_paired_cmpxchg64_le(env, addr, new_lo, new_hi, true);
+ return do_paired_cmpxchg64_le(env, addr, new_lo, new_hi, true, GETPC());
}
static uint64_t do_paired_cmpxchg64_be(CPUARMState *env, uint64_t addr,
uint64_t new_lo, uint64_t new_hi,
- bool parallel)
+ bool parallel, uintptr_t ra)
{
- uintptr_t ra = GETPC();
Int128 oldv, cmpv, newv;
bool success;
@@ -561,11 +559,11 @@ static uint64_t do_paired_cmpxchg64_be(CPUARMState *env, uint64_t addr,
uint64_t HELPER(paired_cmpxchg64_be)(CPUARMState *env, uint64_t addr,
uint64_t new_lo, uint64_t new_hi)
{
- return do_paired_cmpxchg64_be(env, addr, new_lo, new_hi, false);
+ return do_paired_cmpxchg64_be(env, addr, new_lo, new_hi, false, GETPC());
}
uint64_t HELPER(paired_cmpxchg64_be_parallel)(CPUARMState *env, uint64_t addr,
uint64_t new_lo, uint64_t new_hi)
{
- return do_paired_cmpxchg64_be(env, addr, new_lo, new_hi, true);
+ return do_paired_cmpxchg64_be(env, addr, new_lo, new_hi, true, GETPC());
}
--
2.12.0
Richard Henderson <richard.henderson@linaro.org> writes:
> Use of GETPC must be restricted to those functions that are
> directly called from TCG generated code.
>
> Fixes: 2399d4e7cec22ecf1c51062d2ebfd45220dbaace
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
> ---
> target/arm/helper-a64.c | 14 ++++++--------
> 1 file changed, 6 insertions(+), 8 deletions(-)
>
> diff --git a/target/arm/helper-a64.c b/target/arm/helper-a64.c
> index 96a3ecf707..b84ebcae6e 100644
> --- a/target/arm/helper-a64.c
> +++ b/target/arm/helper-a64.c
> @@ -432,9 +432,8 @@ uint64_t HELPER(crc32c_64)(uint64_t acc, uint64_t val, uint32_t bytes)
> /* Returns 0 on success; 1 otherwise. */
> static uint64_t do_paired_cmpxchg64_le(CPUARMState *env, uint64_t addr,
> uint64_t new_lo, uint64_t new_hi,
> - bool parallel)
> + bool parallel, uintptr_t ra)
> {
> - uintptr_t ra = GETPC();
> Int128 oldv, cmpv, newv;
> bool success;
>
> @@ -491,20 +490,19 @@ static uint64_t do_paired_cmpxchg64_le(CPUARMState *env, uint64_t addr,
> uint64_t HELPER(paired_cmpxchg64_le)(CPUARMState *env, uint64_t addr,
> uint64_t new_lo, uint64_t new_hi)
> {
> - return do_paired_cmpxchg64_le(env, addr, new_lo, new_hi, false);
> + return do_paired_cmpxchg64_le(env, addr, new_lo, new_hi, false, GETPC());
> }
>
> uint64_t HELPER(paired_cmpxchg64_le_parallel)(CPUARMState *env, uint64_t addr,
> uint64_t new_lo, uint64_t new_hi)
> {
> - return do_paired_cmpxchg64_le(env, addr, new_lo, new_hi, true);
> + return do_paired_cmpxchg64_le(env, addr, new_lo, new_hi, true, GETPC());
> }
>
> static uint64_t do_paired_cmpxchg64_be(CPUARMState *env, uint64_t addr,
> uint64_t new_lo, uint64_t new_hi,
> - bool parallel)
> + bool parallel, uintptr_t ra)
> {
> - uintptr_t ra = GETPC();
> Int128 oldv, cmpv, newv;
> bool success;
>
> @@ -561,11 +559,11 @@ static uint64_t do_paired_cmpxchg64_be(CPUARMState *env, uint64_t addr,
> uint64_t HELPER(paired_cmpxchg64_be)(CPUARMState *env, uint64_t addr,
> uint64_t new_lo, uint64_t new_hi)
> {
> - return do_paired_cmpxchg64_be(env, addr, new_lo, new_hi, false);
> + return do_paired_cmpxchg64_be(env, addr, new_lo, new_hi, false, GETPC());
> }
>
> uint64_t HELPER(paired_cmpxchg64_be_parallel)(CPUARMState *env, uint64_t addr,
> uint64_t new_lo, uint64_t new_hi)
> {
> - return do_paired_cmpxchg64_be(env, addr, new_lo, new_hi, true);
> + return do_paired_cmpxchg64_be(env, addr, new_lo, new_hi, true, GETPC());
> }
--
Alex Bennée
© 2016 - 2024 Red Hat, Inc.