diff mbox

tci: Optimize saving of TCG code address

Message ID 1398751044-22136-1-git-send-email-sw@weilnetz.de
State Under Review
Headers show

Commit Message

Stefan Weil April 29, 2014, 5:57 a.m. UTC 
It is needed by the GETRA() macro which is called in helper functions,
so it is sufficient to set it before calling any of these helper functions.

In current QEMU, all targets use GETRA(). Therefore tci_tb_ptr is now
needed unconditionally. Setting its value is time critical because it
happens in the inner interpreter loop.

Signed-off-by: Stefan Weil <sw@weilnetz.de>
---
 include/exec/exec-all.h |    6 +-----
 tci.c                   |   32 +++++++++++++++++++++++---------
 2 files changed, 24 insertions(+), 14 deletions(-)

Comments

Richard Henderson April 29, 2014, 3:20 p.m. UTC | #1 
On 04/28/2014 10:57 PM, Stefan Weil wrote:
> -static tcg_target_ulong tci_reg[TCG_TARGET_NB_REGS];
> +static inline void save_tb_ptr(void *tb_ptr)
> +{
> +#ifdef CONFIG_SOFTMMU
> +    tci_tb_ptr = (uintptr_t)tb_ptr;
> +#endif
> +}
>  

Wouldn't it be better to save this always?

I'm a bit confused about how the SIGSEGV path works with TCI (not at all?), but
I have trouble believing that it ever could work without having this value
available.


r~
Stefan Weil April 29, 2014, 5:33 p.m. UTC | #2 
Am 29.04.2014 17:20, schrieb Richard Henderson:
> On 04/28/2014 10:57 PM, Stefan Weil wrote:
>> -static tcg_target_ulong tci_reg[TCG_TARGET_NB_REGS];
>> +static inline void save_tb_ptr(void *tb_ptr)
>> +{
>> +#ifdef CONFIG_SOFTMMU
>> +    tci_tb_ptr = (uintptr_t)tb_ptr;
>> +#endif
>> +}
>>  
> 
> Wouldn't it be better to save this always?
> 
> I'm a bit confused about how the SIGSEGV path works with TCI (not at all?), but
> I have trouble believing that it ever could work without having this value
> available.
> 
> 
> r~
> 

Hi,

I'm still investigating whether it's necessary to set tci_tb_ptr to 0
(as you suggested). Up to now, the TCI code did never invalidate
tci_tb_ptr, but there was no obvious indication of problems caused by
this behaviour.

The new function save_tb_ptr() is only used for the opcodes which call
helper_{ld,st}*_mmu. Those calls are only compiled for CONFIG_SOFTMMU,
so there is no need to save tci_tb_ptr if that macro is undefined. The
user mode emulation (which does not set CONFIG_SOFTMMU) is faster like that.

For the op_call opcode, tci_tb_ptr is set unconditionally.

Regards
Stefan
Richard Henderson April 29, 2014, 6:15 p.m. UTC | #3 
On 04/29/2014 10:33 AM, Stefan Weil wrote:
> I'm still investigating whether it's necessary to set tci_tb_ptr to 0
> (as you suggested). Up to now, the TCI code did never invalidate
> tci_tb_ptr, but there was no obvious indication of problems caused by
> this behaviour.

Oh, I mis-read this bit before.

You could be right that it's not needed, now that we've fixed
the code reading functions to hard-code NULL as the return address.


r~
diff mbox

Patch

diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
index f9ac332..44b24bd 100644
--- a/include/exec/exec-all.h
+++ b/include/exec/exec-all.h
@@ -314,11 +314,7 @@  extern uintptr_t tci_tb_ptr;
    to indicate the compressed mode; subtracting two works around that.  It
    is also the case that there are no host isas that contain a call insn
    smaller than 4 bytes, so we don't worry about special-casing this.  */
-#if defined(CONFIG_TCG_INTERPRETER)
-# define GETPC_ADJ   0
-#else
-# define GETPC_ADJ   2
-#endif
+#define GETPC_ADJ   2
 
 #define GETPC()  (GETRA() - GETPC_ADJ)
 
diff --git a/tci.c b/tci.c
index 6523ab8..407dd3a 100644
--- a/tci.c
+++ b/tci.c
@@ -51,13 +51,18 @@  typedef uint64_t (*helper_function)(tcg_target_ulong, tcg_target_ulong,
                                     tcg_target_ulong);
 #endif
 
-/* Targets which don't use GETPC also don't need tci_tb_ptr
-   which makes them a little faster. */
-#if defined(GETPC)
+static tcg_target_ulong tci_reg[TCG_TARGET_NB_REGS];
+
+/* The macro GETRA() which is called by many helper functions
+ * uses tci_tb_ptr to get the return address. */
 uintptr_t tci_tb_ptr;
-#endif
 
-static tcg_target_ulong tci_reg[TCG_TARGET_NB_REGS];
+static inline void save_tb_ptr(void *tb_ptr)
+{
+#ifdef CONFIG_SOFTMMU
+    tci_tb_ptr = (uintptr_t)tb_ptr;
+#endif
+}
 
 static tcg_target_ulong tci_read_reg(TCGReg index)
 {
@@ -467,10 +472,6 @@  uintptr_t tcg_qemu_tb_exec(CPUArchState *env, uint8_t *tb_ptr)
         uint64_t v64;
 #endif
 
-#if defined(GETPC)
-        tci_tb_ptr = (uintptr_t)tb_ptr;
-#endif
-
         /* Skip opcode and size entry. */
         tb_ptr += 2;
 
@@ -489,6 +490,7 @@  uintptr_t tcg_qemu_tb_exec(CPUArchState *env, uint8_t *tb_ptr)
             TODO();
             break;
         case INDEX_op_call:
+            tci_tb_ptr = (uintptr_t)tb_ptr;
             t0 = tci_read_ri(&tb_ptr);
 #if TCG_TARGET_REG_BITS == 32
             tmp64 = ((helper_function)t0)(tci_read_reg(TCG_REG_R0),
@@ -1087,6 +1089,7 @@  uintptr_t tcg_qemu_tb_exec(CPUArchState *env, uint8_t *tb_ptr)
             tb_ptr += (int32_t)t0;
             continue;
         case INDEX_op_qemu_ld8u:
+            save_tb_ptr(tb_ptr);
             t0 = *tb_ptr++;
             taddr = tci_read_ulong(&tb_ptr);
 #ifdef CONFIG_SOFTMMU
@@ -1098,6 +1101,7 @@  uintptr_t tcg_qemu_tb_exec(CPUArchState *env, uint8_t *tb_ptr)
             tci_write_reg8(t0, tmp8);
             break;
         case INDEX_op_qemu_ld8s:
+            save_tb_ptr(tb_ptr);
             t0 = *tb_ptr++;
             taddr = tci_read_ulong(&tb_ptr);
 #ifdef CONFIG_SOFTMMU
@@ -1109,6 +1113,7 @@  uintptr_t tcg_qemu_tb_exec(CPUArchState *env, uint8_t *tb_ptr)
             tci_write_reg8s(t0, tmp8);
             break;
         case INDEX_op_qemu_ld16u:
+            save_tb_ptr(tb_ptr);
             t0 = *tb_ptr++;
             taddr = tci_read_ulong(&tb_ptr);
 #ifdef CONFIG_SOFTMMU
@@ -1120,6 +1125,7 @@  uintptr_t tcg_qemu_tb_exec(CPUArchState *env, uint8_t *tb_ptr)
             tci_write_reg16(t0, tmp16);
             break;
         case INDEX_op_qemu_ld16s:
+            save_tb_ptr(tb_ptr);
             t0 = *tb_ptr++;
             taddr = tci_read_ulong(&tb_ptr);
 #ifdef CONFIG_SOFTMMU
@@ -1132,6 +1138,7 @@  uintptr_t tcg_qemu_tb_exec(CPUArchState *env, uint8_t *tb_ptr)
             break;
 #if TCG_TARGET_REG_BITS == 64
         case INDEX_op_qemu_ld32u:
+            save_tb_ptr(tb_ptr);
             t0 = *tb_ptr++;
             taddr = tci_read_ulong(&tb_ptr);
 #ifdef CONFIG_SOFTMMU
@@ -1143,6 +1150,7 @@  uintptr_t tcg_qemu_tb_exec(CPUArchState *env, uint8_t *tb_ptr)
             tci_write_reg32(t0, tmp32);
             break;
         case INDEX_op_qemu_ld32s:
+            save_tb_ptr(tb_ptr);
             t0 = *tb_ptr++;
             taddr = tci_read_ulong(&tb_ptr);
 #ifdef CONFIG_SOFTMMU
@@ -1155,6 +1163,7 @@  uintptr_t tcg_qemu_tb_exec(CPUArchState *env, uint8_t *tb_ptr)
             break;
 #endif /* TCG_TARGET_REG_BITS == 64 */
         case INDEX_op_qemu_ld32:
+            save_tb_ptr(tb_ptr);
             t0 = *tb_ptr++;
             taddr = tci_read_ulong(&tb_ptr);
 #ifdef CONFIG_SOFTMMU
@@ -1166,6 +1175,7 @@  uintptr_t tcg_qemu_tb_exec(CPUArchState *env, uint8_t *tb_ptr)
             tci_write_reg32(t0, tmp32);
             break;
         case INDEX_op_qemu_ld64:
+            save_tb_ptr(tb_ptr);
             t0 = *tb_ptr++;
 #if TCG_TARGET_REG_BITS == 32
             t1 = *tb_ptr++;
@@ -1183,6 +1193,7 @@  uintptr_t tcg_qemu_tb_exec(CPUArchState *env, uint8_t *tb_ptr)
 #endif
             break;
         case INDEX_op_qemu_st8:
+            save_tb_ptr(tb_ptr);
             t0 = tci_read_r8(&tb_ptr);
             taddr = tci_read_ulong(&tb_ptr);
 #ifdef CONFIG_SOFTMMU
@@ -1194,6 +1205,7 @@  uintptr_t tcg_qemu_tb_exec(CPUArchState *env, uint8_t *tb_ptr)
 #endif
             break;
         case INDEX_op_qemu_st16:
+            save_tb_ptr(tb_ptr);
             t0 = tci_read_r16(&tb_ptr);
             taddr = tci_read_ulong(&tb_ptr);
 #ifdef CONFIG_SOFTMMU
@@ -1205,6 +1217,7 @@  uintptr_t tcg_qemu_tb_exec(CPUArchState *env, uint8_t *tb_ptr)
 #endif
             break;
         case INDEX_op_qemu_st32:
+            save_tb_ptr(tb_ptr);
             t0 = tci_read_r32(&tb_ptr);
             taddr = tci_read_ulong(&tb_ptr);
 #ifdef CONFIG_SOFTMMU
@@ -1216,6 +1229,7 @@  uintptr_t tcg_qemu_tb_exec(CPUArchState *env, uint8_t *tb_ptr)
 #endif
             break;
         case INDEX_op_qemu_st64:
+            save_tb_ptr(tb_ptr);
             tmp64 = tci_read_r64(&tb_ptr);
             taddr = tci_read_ulong(&tb_ptr);
 #ifdef CONFIG_SOFTMMU