Message ID | 1377789697-12561-1-git-send-email-rth@twiddle.net |
---|---|
State | New |
Headers | show |
Il 29/08/2013 17:21, Richard Henderson ha scritto:
> I have pending but unposted patch sets for AArch64 and PowerPC.
I also have something working for PPC since I have old hardware... I'm
waiting for the API to settle before posting patches.
Paolo
On 08/29/2013 08:28 AM, Paolo Bonzini wrote: > I also have something working for PPC since I have old hardware... I'm > waiting for the API to settle before posting patches. I've gotten the admins to install enough of a 32-bit environment on the GCC compile farm's power7 box to test there, but it'll be nice to have a confirm on old hw too. r~
On Thu, Aug 29, 2013 at 08:21:37AM -0700, Richard Henderson wrote: > Indeed, remove it entirely and remove the is_tcg_gen_code check > from GETPC_EXT. > > Fixes https://bugs.launchpad.net/qemu/+bug/1218098 wherein a call > to a "normal" helper function performed a sequence of tail calls > all the way into the memory helper functions, leading to a stack > frame in which the memory helper function appeared to be called > directly from tcg. > > Signed-off-by: Richard Henderson <rth@twiddle.net> This fixes the bug I saw. Tested-by: Richard W.M. Jones <rjones@redhat.com> Rich.
On Thu, Aug 29, 2013 at 08:21:37AM -0700, Richard Henderson wrote: > Indeed, remove it entirely and remove the is_tcg_gen_code check > from GETPC_EXT. > > Fixes https://bugs.launchpad.net/qemu/+bug/1218098 wherein a call > to a "normal" helper function performed a sequence of tail calls > all the way into the memory helper functions, leading to a stack > frame in which the memory helper function appeared to be called > directly from tcg. > > Signed-off-by: Richard Henderson <rth@twiddle.net> > --- > include/exec/exec-all.h | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > This is actually conclusive proof that using is_tcg_gen_code, and > thus any scheme that requires GETPC_LDST, is fundamentally flawed. > > Thankfully, the follow-up patch sets that I've already posted give > a framework for properly fixing this. I've already posted a cleanup > for ARM to fix this. I have pending but unposted patch sets for > AArch64 and PowerPC. > > In the meantime, please apply this fix for x86_64 asap. > > > r~ > > > > diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h > index b70028a..ffb69a4 100644 > --- a/include/exec/exec-all.h > +++ b/include/exec/exec-all.h > @@ -326,9 +326,7 @@ extern uintptr_t tci_tb_ptr; > (6) jump to corresponding code of the next of fast path > */ > # if defined(__i386__) || defined(__x86_64__) > -# define GETRA() ((uintptr_t)__builtin_return_address(0)) > -/* The return address argument for ldst is passed directly. */ > -# define GETPC_LDST() (abort(), 0) > +# define GETPC_EXT() GETPC() > # elif defined (_ARCH_PPC) && !defined (_ARCH_PPC64) > # define GETRA() ((uintptr_t)__builtin_return_address(0)) > # define GETPC_LDST() ((uintptr_t) ((*(int32_t *)(GETRA() - 4)) - 1)) > @@ -349,7 +347,7 @@ static inline uintptr_t tcg_getpc_ldst(uintptr_t ra) > not the start of the next opcode */ > return ra; > } > -#elif defined(__aarch64__) > +# elif defined(__aarch64__) > # define GETRA() ((uintptr_t)__builtin_return_address(0)) > # define GETPC_LDST() tcg_getpc_ldst(GETRA()) > static inline uintptr_t tcg_getpc_ldst(uintptr_t ra) > @@ -367,7 +365,9 @@ static inline uintptr_t tcg_getpc_ldst(uintptr_t ra) > # error "CONFIG_QEMU_LDST_OPTIMIZATION needs GETPC_LDST() implementation!" > # endif > bool is_tcg_gen_code(uintptr_t pc_ptr); > -# define GETPC_EXT() (is_tcg_gen_code(GETRA()) ? GETPC_LDST() : GETPC()) > +# ifndef GETPC_EXT > +# define GETPC_EXT() (is_tcg_gen_code(GETRA()) ? GETPC_LDST() : GETPC()) > +# endif > #else > # define GETPC_EXT() GETPC() > #endif Thanks, applied.
diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h index b70028a..ffb69a4 100644 --- a/include/exec/exec-all.h +++ b/include/exec/exec-all.h @@ -326,9 +326,7 @@ extern uintptr_t tci_tb_ptr; (6) jump to corresponding code of the next of fast path */ # if defined(__i386__) || defined(__x86_64__) -# define GETRA() ((uintptr_t)__builtin_return_address(0)) -/* The return address argument for ldst is passed directly. */ -# define GETPC_LDST() (abort(), 0) +# define GETPC_EXT() GETPC() # elif defined (_ARCH_PPC) && !defined (_ARCH_PPC64) # define GETRA() ((uintptr_t)__builtin_return_address(0)) # define GETPC_LDST() ((uintptr_t) ((*(int32_t *)(GETRA() - 4)) - 1)) @@ -349,7 +347,7 @@ static inline uintptr_t tcg_getpc_ldst(uintptr_t ra) not the start of the next opcode */ return ra; } -#elif defined(__aarch64__) +# elif defined(__aarch64__) # define GETRA() ((uintptr_t)__builtin_return_address(0)) # define GETPC_LDST() tcg_getpc_ldst(GETRA()) static inline uintptr_t tcg_getpc_ldst(uintptr_t ra) @@ -367,7 +365,9 @@ static inline uintptr_t tcg_getpc_ldst(uintptr_t ra) # error "CONFIG_QEMU_LDST_OPTIMIZATION needs GETPC_LDST() implementation!" # endif bool is_tcg_gen_code(uintptr_t pc_ptr); -# define GETPC_EXT() (is_tcg_gen_code(GETRA()) ? GETPC_LDST() : GETPC()) +# ifndef GETPC_EXT +# define GETPC_EXT() (is_tcg_gen_code(GETRA()) ? GETPC_LDST() : GETPC()) +# endif #else # define GETPC_EXT() GETPC() #endif
Indeed, remove it entirely and remove the is_tcg_gen_code check from GETPC_EXT. Fixes https://bugs.launchpad.net/qemu/+bug/1218098 wherein a call to a "normal" helper function performed a sequence of tail calls all the way into the memory helper functions, leading to a stack frame in which the memory helper function appeared to be called directly from tcg. Signed-off-by: Richard Henderson <rth@twiddle.net> --- include/exec/exec-all.h | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) This is actually conclusive proof that using is_tcg_gen_code, and thus any scheme that requires GETPC_LDST, is fundamentally flawed. Thankfully, the follow-up patch sets that I've already posted give a framework for properly fixing this. I've already posted a cleanup for ARM to fix this. I have pending but unposted patch sets for AArch64 and PowerPC. In the meantime, please apply this fix for x86_64 asap. r~