Patchwork tcg-i386: Remove abort from GETPC_LDST

login
register
mail settings
Submitter Richard Henderson
Date Aug. 29, 2013, 3:21 p.m.
Message ID <1377789697-12561-1-git-send-email-rth@twiddle.net>
Download mbox | patch
Permalink /patch/270872/
State New
Headers show

Comments

Richard Henderson - Aug. 29, 2013, 3:21 p.m.
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~
Paolo Bonzini - Aug. 29, 2013, 3:28 p.m.
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
Richard Henderson - Aug. 29, 2013, 3:32 p.m.
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~
Richard W.M. Jones - Aug. 29, 2013, 3:50 p.m.
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.
Aurelien Jarno - Aug. 29, 2013, 7:13 p.m.
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.

Patch

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