Re: [PATCH v2 04/28] cputlb: Move body of cpu_ldst_template.h out of line

["Alex Bennée" <alex.bennee@linaro.org>]

Alex Bennée <alex.bennee@linaro.org> writes:

> Richard Henderson <richard.henderson@linaro.org> writes:
>
>> With the tracing hooks, the inline functions are no longer
>> so simple.  Once out-of-line, the current tlb_entry lookup
>> is redundant with the one in the main load/store_helper.
>>
>> This also begins the introduction of a new target facing
>> interface, with suffix *_mmuidx_ra.  This is not yet
>> official because the interface is not done for user-only.
>>
>> Use abi_ptr instead of target_ulong in preparation for
>> user-only; the two types are identical for softmmu.
>>
>> What remains in cpu_ldst_template.h are the expansions
>> for _code, _data, and MMU_MODE<N>_SUFFIX.
>>
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>
> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

Although this seems to have broken --enable-plugins builds:

    CC      aarch64-softmmu/plugins/api.o
  /home/alex.bennee/lsrc/qemu.git/plugins/api.c: In function ‘qemu_plugin_mem_size_shift’:
  /home/alex.bennee/lsrc/qemu.git/plugins/api.c:229:19: error: ‘TRACE_MEM_SZ_SHIFT_MASK’ undeclared (first use in this function)
       return info & TRACE_MEM_SZ_SHIFT_MASK;
                     ^~~~~~~~~~~~~~~~~~~~~~~
  /home/alex.bennee/lsrc/qemu.git/plugins/api.c:229:19: note: each undeclared identifier is reported only once for each function it appears in
  /home/alex.bennee/lsrc/qemu.git/plugins/api.c: In function ‘qemu_plugin_mem_is_sign_extended’:
  /home/alex.bennee/lsrc/qemu.git/plugins/api.c:234:22: error: ‘TRACE_MEM_SE’ undeclared (first use in this function)
       return !!(info & TRACE_MEM_SE);
                        ^~~~~~~~~~~~
  /home/alex.bennee/lsrc/qemu.git/plugins/api.c: In function ‘qemu_plugin_mem_is_big_endian’:
  /home/alex.bennee/lsrc/qemu.git/plugins/api.c:239:22: error: ‘TRACE_MEM_BE’ undeclared (first use in this function)
       return !!(info & TRACE_MEM_BE);
                        ^~~~~~~~~~~~
  /home/alex.bennee/lsrc/qemu.git/plugins/api.c: In function ‘qemu_plugin_mem_is_store’:
  /home/alex.bennee/lsrc/qemu.git/plugins/api.c:244:22: error: ‘TRACE_MEM_ST’ undeclared (first use in this function)
       return !!(info & TRACE_MEM_ST);
                        ^~~~~~~~~~~~
  /home/alex.bennee/lsrc/qemu.git/plugins/api.c: In function ‘qemu_plugin_get_hwaddr’:
  /home/alex.bennee/lsrc/qemu.git/plugins/api.c:258:36: error: ‘TRACE_MEM_MMU_SHIFT’ undeclared (first use in this function); did you mean ‘TCP_MAX_WINSHIFT’?
       unsigned int mmu_idx = info >> TRACE_MEM_MMU_SHIFT;
                                      ^~~~~~~~~~~~~~~~~~~
                                      TCP_MAX_WINSHIFT
  /home/alex.bennee/lsrc/qemu.git/plugins/api.c:259:35: error: ‘TRACE_MEM_ST’ undeclared (first use in this function); did you mean ‘TRACE_MEM_MMU_SHIFT’?
       hwaddr_info.is_store = info & TRACE_MEM_ST;
                                     ^~~~~~~~~~~~
                                     TRACE_MEM_MMU_SHIFT
  /home/alex.bennee/lsrc/qemu.git/plugins/api.c: In function ‘qemu_plugin_mem_size_shift’:
  /home/alex.bennee/lsrc/qemu.git/plugins/api.c:230:1: error: control reaches end of non-void function [-Werror=return-type]
   }
   ^
  /home/alex.bennee/lsrc/qemu.git/plugins/api.c: In function ‘qemu_plugin_mem_is_sign_extended’:
  /home/alex.bennee/lsrc/qemu.git/plugins/api.c:235:1: error: control reaches end of non-void function [-Werror=return-type]
   }
   ^
  /home/alex.bennee/lsrc/qemu.git/plugins/api.c: In function ‘qemu_plugin_mem_is_big_endian’:
  /home/alex.bennee/lsrc/qemu.git/plugins/api.c:240:1: error: control reaches end of non-void function [-Werror=return-type]
   }
   ^
  /home/alex.bennee/lsrc/qemu.git/plugins/api.c: In function ‘qemu_plugin_mem_is_store’:
  /home/alex.bennee/lsrc/qemu.git/plugins/api.c:245:1: error: control reaches end of non-void function [-Werror=return-type]
   }
   ^
  cc1: all warnings being treated as errors
  /home/alex.bennee/lsrc/qemu.git/rules.mak:69: recipe for target 'plugins/api.o' failed
  make[1]: *** [plugins/api.o] Error 1
  Makefile:491: recipe for target 'aarch64-softmmu/all' failed
  make: *** [aarch64-softmmu/all] Error 2


>
>> ---
>>  include/exec/cpu_ldst.h          |  25 ++++++-
>>  include/exec/cpu_ldst_template.h | 125 +++++++------------------------
>>  accel/tcg/cputlb.c               | 116 ++++++++++++++++++++++++++++
>>  3 files changed, 166 insertions(+), 100 deletions(-)
>>
>> diff --git a/include/exec/cpu_ldst.h b/include/exec/cpu_ldst.h
>> index fd499f7e2f..cf8af36dbc 100644
>> --- a/include/exec/cpu_ldst.h
>> +++ b/include/exec/cpu_ldst.h
>> @@ -152,7 +152,7 @@ static inline void clear_helper_retaddr(void)
>>  
>>  #else
>>  
>> -/* The memory helpers for tcg-generated code need tcg_target_long etc.  */
>> +/* Needed for TCG_OVERSIZED_GUEST */
>>  #include "tcg.h"
>>  
>>  static inline target_ulong tlb_addr_write(const CPUTLBEntry *entry)
>> @@ -185,6 +185,29 @@ static inline CPUTLBEntry *tlb_entry(CPUArchState *env, uintptr_t mmu_idx,
>>      return &env_tlb(env)->f[mmu_idx].table[tlb_index(env, mmu_idx, addr)];
>>  }
>>  
>> +uint32_t cpu_ldub_mmuidx_ra(CPUArchState *env, abi_ptr addr,
>> +                            int mmu_idx, uintptr_t ra);
>> +uint32_t cpu_lduw_mmuidx_ra(CPUArchState *env, abi_ptr addr,
>> +                            int mmu_idx, uintptr_t ra);
>> +uint32_t cpu_ldl_mmuidx_ra(CPUArchState *env, abi_ptr addr,
>> +                           int mmu_idx, uintptr_t ra);
>> +uint64_t cpu_ldq_mmuidx_ra(CPUArchState *env, abi_ptr addr,
>> +                           int mmu_idx, uintptr_t ra);
>> +
>> +int cpu_ldsb_mmuidx_ra(CPUArchState *env, abi_ptr addr,
>> +                       int mmu_idx, uintptr_t ra);
>> +int cpu_ldsw_mmuidx_ra(CPUArchState *env, abi_ptr addr,
>> +                       int mmu_idx, uintptr_t ra);
>> +
>> +void cpu_stb_mmuidx_ra(CPUArchState *env, abi_ptr addr, uint32_t val,
>> +                       int mmu_idx, uintptr_t retaddr);
>> +void cpu_stw_mmuidx_ra(CPUArchState *env, abi_ptr addr, uint32_t val,
>> +                       int mmu_idx, uintptr_t retaddr);
>> +void cpu_stl_mmuidx_ra(CPUArchState *env, abi_ptr addr, uint32_t val,
>> +                       int mmu_idx, uintptr_t retaddr);
>> +void cpu_stq_mmuidx_ra(CPUArchState *env, abi_ptr addr, uint64_t val,
>> +                       int mmu_idx, uintptr_t retaddr);
>> +
>>  #ifdef MMU_MODE0_SUFFIX
>>  #define CPU_MMU_INDEX 0
>>  #define MEMSUFFIX MMU_MODE0_SUFFIX
>> diff --git a/include/exec/cpu_ldst_template.h b/include/exec/cpu_ldst_template.h
>> index 0ad5de3ef9..ea39e29c19 100644
>> --- a/include/exec/cpu_ldst_template.h
>> +++ b/include/exec/cpu_ldst_template.h
>> @@ -24,13 +24,6 @@
>>   * License along with this library; if not, see <http://www.gnu.org/licenses/>.
>>   */
>>  
>> -#if !defined(SOFTMMU_CODE_ACCESS)
>> -#include "trace-root.h"
>> -#endif
>> -
>> -#include "qemu/plugin.h"
>> -#include "trace/mem.h"
>> -
>>  #if DATA_SIZE == 8
>>  #define SUFFIX q
>>  #define USUFFIX q
>> @@ -63,56 +56,40 @@
>>  #define RES_TYPE uint32_t
>>  #endif
>>  
>> +/* generic load/store macros */
>> +
>>  #ifdef SOFTMMU_CODE_ACCESS
>> -#define ADDR_READ addr_code
>> -#define MMUSUFFIX _cmmu
>> -#define URETSUFFIX USUFFIX
>> -#define SRETSUFFIX glue(s, SUFFIX)
>> -#else
>> -#define ADDR_READ addr_read
>> -#define MMUSUFFIX _mmu
>> -#define URETSUFFIX USUFFIX
>> -#define SRETSUFFIX glue(s, SUFFIX)
>> +
>> +static inline RES_TYPE
>> +glue(glue(cpu_ld, USUFFIX), _code)(CPUArchState *env, target_ulong ptr)
>> +{
>> +    TCGMemOpIdx oi = make_memop_idx(MO_TE | SHIFT, CPU_MMU_INDEX);
>> +    return glue(glue(helper_ret_ld, USUFFIX), _cmmu)(env, ptr, oi, 0);
>> +}
>> +
>> +#if DATA_SIZE <= 2
>> +static inline int
>> +glue(glue(cpu_lds, SUFFIX), _code)(CPUArchState *env, target_ulong ptr)
>> +{
>> +    return (DATA_STYPE)glue(glue(cpu_ld, USUFFIX), _code)(env, ptr);
>> +}
>>  #endif
>>  
>> -/* generic load/store macros */
>> +#else
>>  
>>  static inline RES_TYPE
>>  glue(glue(glue(cpu_ld, USUFFIX), MEMSUFFIX), _ra)(CPUArchState *env,
>>                                                    target_ulong ptr,
>>                                                    uintptr_t retaddr)
>>  {
>> -    CPUTLBEntry *entry;
>> -    RES_TYPE res;
>> -    target_ulong addr;
>> -    int mmu_idx = CPU_MMU_INDEX;
>> -    MemOp op = MO_TE | SHIFT;
>> -#if !defined(SOFTMMU_CODE_ACCESS)
>> -    uint16_t meminfo = trace_mem_get_info(op, mmu_idx, false);
>> -    trace_guest_mem_before_exec(env_cpu(env), ptr, meminfo);
>> -#endif
>> -
>> -    addr = ptr;
>> -    entry = tlb_entry(env, mmu_idx, addr);
>> -    if (unlikely(entry->ADDR_READ !=
>> -                 (addr & (TARGET_PAGE_MASK | (DATA_SIZE - 1))))) {
>> -        TCGMemOpIdx oi = make_memop_idx(op, mmu_idx);
>> -        res = glue(glue(helper_ret_ld, URETSUFFIX), MMUSUFFIX)(env, addr,
>> -                                                               oi, retaddr);
>> -    } else {
>> -        uintptr_t hostaddr = addr + entry->addend;
>> -        res = glue(glue(ld, USUFFIX), _p)((uint8_t *)hostaddr);
>> -    }
>> -#ifndef SOFTMMU_CODE_ACCESS
>> -    qemu_plugin_vcpu_mem_cb(env_cpu(env), ptr, meminfo);
>> -#endif
>> -    return res;
>> +    return glue(glue(cpu_ld, USUFFIX), _mmuidx_ra)(env, ptr, CPU_MMU_INDEX,
>> +                                                   retaddr);
>>  }
>>  
>>  static inline RES_TYPE
>>  glue(glue(cpu_ld, USUFFIX), MEMSUFFIX)(CPUArchState *env, target_ulong ptr)
>>  {
>> -    return glue(glue(glue(cpu_ld, USUFFIX), MEMSUFFIX), _ra)(env, ptr, 0);
>> +    return glue(glue(cpu_ld, USUFFIX), _mmuidx_ra)(env, ptr, CPU_MMU_INDEX, 0);
>>  }
>>  
>>  #if DATA_SIZE <= 2
>> @@ -121,42 +98,17 @@ glue(glue(glue(cpu_lds, SUFFIX), MEMSUFFIX), _ra)(CPUArchState *env,
>>                                                    target_ulong ptr,
>>                                                    uintptr_t retaddr)
>>  {
>> -    CPUTLBEntry *entry;
>> -    int res;
>> -    target_ulong addr;
>> -    int mmu_idx = CPU_MMU_INDEX;
>> -    MemOp op = MO_TE | MO_SIGN | SHIFT;
>> -#ifndef SOFTMMU_CODE_ACCESS
>> -    uint16_t meminfo = trace_mem_get_info(op, mmu_idx, false);
>> -    trace_guest_mem_before_exec(env_cpu(env), ptr, meminfo);
>> -#endif
>> -
>> -    addr = ptr;
>> -    entry = tlb_entry(env, mmu_idx, addr);
>> -    if (unlikely(entry->ADDR_READ !=
>> -                 (addr & (TARGET_PAGE_MASK | (DATA_SIZE - 1))))) {
>> -        TCGMemOpIdx oi = make_memop_idx(op & ~MO_SIGN, mmu_idx);
>> -        res = (DATA_STYPE)glue(glue(helper_ret_ld, SRETSUFFIX),
>> -                               MMUSUFFIX)(env, addr, oi, retaddr);
>> -    } else {
>> -        uintptr_t hostaddr = addr + entry->addend;
>> -        res = glue(glue(lds, SUFFIX), _p)((uint8_t *)hostaddr);
>> -    }
>> -#ifndef SOFTMMU_CODE_ACCESS
>> -    qemu_plugin_vcpu_mem_cb(env_cpu(env), ptr, meminfo);
>> -#endif
>> -    return res;
>> +    return glue(glue(cpu_lds, SUFFIX), _mmuidx_ra)(env, ptr, CPU_MMU_INDEX,
>> +                                                   retaddr);
>>  }
>>  
>>  static inline int
>>  glue(glue(cpu_lds, SUFFIX), MEMSUFFIX)(CPUArchState *env, target_ulong ptr)
>>  {
>> -    return glue(glue(glue(cpu_lds, SUFFIX), MEMSUFFIX), _ra)(env, ptr, 0);
>> +    return glue(glue(cpu_lds, SUFFIX), _mmuidx_ra)(env, ptr, CPU_MMU_INDEX, 0);
>>  }
>>  #endif
>>  
>> -#ifndef SOFTMMU_CODE_ACCESS
>> -
>>  /* generic store macro */
>>  
>>  static inline void
>> @@ -164,36 +116,15 @@ glue(glue(glue(cpu_st, SUFFIX), MEMSUFFIX), _ra)(CPUArchState *env,
>>                                                   target_ulong ptr,
>>                                                   RES_TYPE v, uintptr_t retaddr)
>>  {
>> -    CPUTLBEntry *entry;
>> -    target_ulong addr;
>> -    int mmu_idx = CPU_MMU_INDEX;
>> -    MemOp op = MO_TE | SHIFT;
>> -#if !defined(SOFTMMU_CODE_ACCESS)
>> -    uint16_t meminfo = trace_mem_get_info(op, mmu_idx, true);
>> -    trace_guest_mem_before_exec(env_cpu(env), ptr, meminfo);
>> -#endif
>> -
>> -    addr = ptr;
>> -    entry = tlb_entry(env, mmu_idx, addr);
>> -    if (unlikely(tlb_addr_write(entry) !=
>> -                 (addr & (TARGET_PAGE_MASK | (DATA_SIZE - 1))))) {
>> -        TCGMemOpIdx oi = make_memop_idx(op, mmu_idx);
>> -        glue(glue(helper_ret_st, SUFFIX), MMUSUFFIX)(env, addr, v, oi,
>> -                                                     retaddr);
>> -    } else {
>> -        uintptr_t hostaddr = addr + entry->addend;
>> -        glue(glue(st, SUFFIX), _p)((uint8_t *)hostaddr, v);
>> -    }
>> -#ifndef SOFTMMU_CODE_ACCESS
>> -    qemu_plugin_vcpu_mem_cb(env_cpu(env), ptr, meminfo);
>> -#endif
>> +    glue(glue(cpu_st, SUFFIX), _mmuidx_ra)(env, ptr, v, CPU_MMU_INDEX,
>> +                                           retaddr);
>>  }
>>  
>>  static inline void
>>  glue(glue(cpu_st, SUFFIX), MEMSUFFIX)(CPUArchState *env, target_ulong ptr,
>>                                        RES_TYPE v)
>>  {
>> -    glue(glue(glue(cpu_st, SUFFIX), MEMSUFFIX), _ra)(env, ptr, v, 0);
>> +    glue(glue(cpu_st, SUFFIX), _mmuidx_ra)(env, ptr, v, CPU_MMU_INDEX, 0);
>>  }
>>  
>>  #endif /* !SOFTMMU_CODE_ACCESS */
>> @@ -204,8 +135,4 @@ glue(glue(cpu_st, SUFFIX), MEMSUFFIX)(CPUArchState *env, target_ulong ptr,
>>  #undef SUFFIX
>>  #undef USUFFIX
>>  #undef DATA_SIZE
>> -#undef MMUSUFFIX
>> -#undef ADDR_READ
>> -#undef URETSUFFIX
>> -#undef SRETSUFFIX
>>  #undef SHIFT
>> diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
>> index 98221948d6..ddd19718bf 100644
>> --- a/accel/tcg/cputlb.c
>> +++ b/accel/tcg/cputlb.c
>> @@ -34,6 +34,9 @@
>>  #include "qemu/atomic.h"
>>  #include "qemu/atomic128.h"
>>  #include "translate-all.h"
>> +#include "trace-root.h"
>> +#include "qemu/plugin.h"
>> +#include "trace/mem.h"
>>  #ifdef CONFIG_PLUGIN
>>  #include "qemu/plugin-memory.h"
>>  #endif
>> @@ -1625,6 +1628,75 @@ tcg_target_ulong helper_be_ldsl_mmu(CPUArchState *env, target_ulong addr,
>>      return (int32_t)helper_be_ldul_mmu(env, addr, oi, retaddr);
>>  }
>>  
>> +/*
>> + * Load helpers for cpu_ldst.h.
>> + */
>> +
>> +static inline uint64_t cpu_load_helper(CPUArchState *env, abi_ptr addr,
>> +                                       int mmu_idx, uintptr_t retaddr,
>> +                                       MemOp op, FullLoadHelper *full_load)
>> +{
>> +    uint16_t meminfo;
>> +    TCGMemOpIdx oi;
>> +    uint64_t ret;
>> +
>> +    meminfo = trace_mem_get_info(op, mmu_idx, false);
>> +    trace_guest_mem_before_exec(env_cpu(env), addr, meminfo);
>> +
>> +    op &= ~MO_SIGN;
>> +    oi = make_memop_idx(op, mmu_idx);
>> +    ret = full_load(env, addr, oi, retaddr);
>> +
>> +    qemu_plugin_vcpu_mem_cb(env_cpu(env), addr, meminfo);
>> +
>> +    return ret;
>> +}
>> +
>> +uint32_t cpu_ldub_mmuidx_ra(CPUArchState *env, abi_ptr addr,
>> +                            int mmu_idx, uintptr_t ra)
>> +{
>> +    return cpu_load_helper(env, addr, mmu_idx, ra, MO_UB, full_ldub_mmu);
>> +}
>> +
>> +int cpu_ldsb_mmuidx_ra(CPUArchState *env, abi_ptr addr,
>> +                       int mmu_idx, uintptr_t ra)
>> +{
>> +    return (int8_t)cpu_load_helper(env, addr, mmu_idx, ra, MO_SB,
>> +                                   full_ldub_mmu);
>> +}
>> +
>> +uint32_t cpu_lduw_mmuidx_ra(CPUArchState *env, abi_ptr addr,
>> +                            int mmu_idx, uintptr_t ra)
>> +{
>> +    return cpu_load_helper(env, addr, mmu_idx, ra, MO_TEUW,
>> +                           MO_TE == MO_LE
>> +                           ? full_le_lduw_mmu : full_be_lduw_mmu);
>> +}
>> +
>> +int cpu_ldsw_mmuidx_ra(CPUArchState *env, abi_ptr addr,
>> +                       int mmu_idx, uintptr_t ra)
>> +{
>> +    return (int16_t)cpu_load_helper(env, addr, mmu_idx, ra, MO_TESW,
>> +                                    MO_TE == MO_LE
>> +                                    ? full_le_lduw_mmu : full_be_lduw_mmu);
>> +}
>> +
>> +uint32_t cpu_ldl_mmuidx_ra(CPUArchState *env, abi_ptr addr,
>> +                           int mmu_idx, uintptr_t ra)
>> +{
>> +    return cpu_load_helper(env, addr, mmu_idx, ra, MO_TEUL,
>> +                           MO_TE == MO_LE
>> +                           ? full_le_ldul_mmu : full_be_ldul_mmu);
>> +}
>> +
>> +uint64_t cpu_ldq_mmuidx_ra(CPUArchState *env, abi_ptr addr,
>> +                           int mmu_idx, uintptr_t ra)
>> +{
>> +    return cpu_load_helper(env, addr, mmu_idx, ra, MO_TEQ,
>> +                           MO_TE == MO_LE
>> +                           ? helper_le_ldq_mmu : helper_be_ldq_mmu);
>> +}
>> +
>>  /*
>>   * Store Helpers
>>   */
>> @@ -1854,6 +1926,50 @@ void helper_be_stq_mmu(CPUArchState *env, target_ulong addr, uint64_t val,
>>      store_helper(env, addr, val, oi, retaddr, MO_BEQ);
>>  }
>>  
>> +/*
>> + * Store Helpers for cpu_ldst.h
>> + */
>> +
>> +static inline void QEMU_ALWAYS_INLINE
>> +cpu_store_helper(CPUArchState *env, target_ulong addr, uint64_t val,
>> +                 int mmu_idx, uintptr_t retaddr, MemOp op)
>> +{
>> +    TCGMemOpIdx oi;
>> +    uint16_t meminfo;
>> +
>> +    meminfo = trace_mem_get_info(op, mmu_idx, true);
>> +    trace_guest_mem_before_exec(env_cpu(env), addr, meminfo);
>> +
>> +    oi = make_memop_idx(op, mmu_idx);
>> +    store_helper(env, addr, val, oi, retaddr, op);
>> +
>> +    qemu_plugin_vcpu_mem_cb(env_cpu(env), addr, meminfo);
>> +}
>> +
>> +void cpu_stb_mmuidx_ra(CPUArchState *env, target_ulong addr, uint32_t val,
>> +                       int mmu_idx, uintptr_t retaddr)
>> +{
>> +    cpu_store_helper(env, addr, val, mmu_idx, retaddr, MO_UB);
>> +}
>> +
>> +void cpu_stw_mmuidx_ra(CPUArchState *env, target_ulong addr, uint32_t val,
>> +                       int mmu_idx, uintptr_t retaddr)
>> +{
>> +    cpu_store_helper(env, addr, val, mmu_idx, retaddr, MO_TEUW);
>> +}
>> +
>> +void cpu_stl_mmuidx_ra(CPUArchState *env, target_ulong addr, uint32_t val,
>> +                       int mmu_idx, uintptr_t retaddr)
>> +{
>> +    cpu_store_helper(env, addr, val, mmu_idx, retaddr, MO_TEUL);
>> +}
>> +
>> +void cpu_stq_mmuidx_ra(CPUArchState *env, target_ulong addr, uint64_t val,
>> +                       int mmu_idx, uintptr_t retaddr)
>> +{
>> +    cpu_store_helper(env, addr, val, mmu_idx, retaddr, MO_TEQ);
>> +}
>> +
>>  /* First set of helpers allows passing in of OI and RETADDR.  This makes
>>     them callable from other helpers.  */


-- 
Alex Bennée