Re: [Qemu-devel] [PATCH v2 6/7] tcg: Introduce zero and sign-extended versions of load helpers

[Aurelien Jarno]

On Thu, Aug 29, 2013 at 03:06:00PM -0700, Richard Henderson wrote:
> Signed-off-by: Richard Henderson <address@hidden>
> ---
>  include/exec/softmmu_template.h | 58 
> ++++++++++++++++++++++++++++++++---------
>  tcg/i386/tcg-target.c           |  6 ++---
>  tcg/tcg.h                       | 21 ++++++++++-----
>  3 files changed, 64 insertions(+), 21 deletions(-)
> 
> diff --git a/include/exec/softmmu_template.h b/include/exec/softmmu_template.h
> index f9922e2..5bbc56a 100644
> --- a/include/exec/softmmu_template.h
> +++ b/include/exec/softmmu_template.h
> @@ -29,23 +29,39 @@
>  #if DATA_SIZE == 8
>  #define SUFFIX q
>  #define LSUFFIX q
> -#define DATA_TYPE uint64_t
> +#define SDATA_TYPE  int64_t
>  #elif DATA_SIZE == 4
>  #define SUFFIX l
>  #define LSUFFIX l
> -#define DATA_TYPE uint32_t
> +#define SDATA_TYPE  int32_t
>  #elif DATA_SIZE == 2
>  #define SUFFIX w
>  #define LSUFFIX uw
> -#define DATA_TYPE uint16_t
> +#define SDATA_TYPE  int16_t
>  #elif DATA_SIZE == 1
>  #define SUFFIX b
>  #define LSUFFIX ub
> -#define DATA_TYPE uint8_t
> +#define SDATA_TYPE  int8_t
>  #else
>  #error unsupported data size
>  #endif
>  
> +#define DATA_TYPE   glue(u, SDATA_TYPE)
> +
> +/* For the benefit of TCG generated code, we want to avoid the complication
> +   of ABI-specific return type promotion and always return a value extended
> +   to the register size of the host.  This is tcg_target_long, except in the
> +   case of a 32-bit host and 64-bit data, and for that we always have
> +   uint64_t.  Don't bother with this widened value for SOFTMMU_CODE_ACCESS.  
> */
> +#if defined(SOFTMMU_CODE_ACCESS) || DATA_SIZE == 8
> +# define WORD_TYPE  DATA_TYPE
> +# define USUFFIX    SUFFIX
> +#else
> +# define WORD_TYPE  tcg_target_ulong
> +# define USUFFIX    glue(u, SUFFIX)
> +# define SSUFFIX    glue(s, SUFFIX)
> +#endif
> +
>  #ifdef SOFTMMU_CODE_ACCESS
>  #define READ_ACCESS_TYPE 2
>  #define ADDR_READ addr_code
> @@ -77,10 +93,10 @@ static inline DATA_TYPE glue(io_read, 
> SUFFIX)(CPUArchState *env,
>  #ifdef SOFTMMU_CODE_ACCESS
>  static
>  #endif
> -DATA_TYPE
> -glue(glue(helper_ret_ld, SUFFIX), MMUSUFFIX)(CPUArchState *env,
> -                                             target_ulong addr, int mmu_idx,
> -                                             uintptr_t retaddr)
> +WORD_TYPE
> +glue(glue(helper_ret_ld, USUFFIX), MMUSUFFIX)(CPUArchState *env,
> +                                              target_ulong addr, int mmu_idx,
> +                                              uintptr_t retaddr)
>  {
>      int index = (addr >> TARGET_PAGE_BITS) & (CPU_TLB_SIZE - 1);
>      target_ulong tlb_addr = env->tlb_table[mmu_idx][index].ADDR_READ;
> @@ -126,9 +142,9 @@ glue(glue(helper_ret_ld, SUFFIX), MMUSUFFIX)(CPUArchState 
> *env,
>          addr2 = addr1 + DATA_SIZE;
>          /* Note the adjustment at the beginning of the function.
>             Undo that for the recursion.  */
> -        res1 = glue(glue(helper_ret_ld, SUFFIX), MMUSUFFIX)
> +        res1 = glue(glue(helper_ret_ld, USUFFIX), MMUSUFFIX)
>              (env, addr1, mmu_idx, retaddr + GETPC_ADJ);
> -        res2 = glue(glue(helper_ret_ld, SUFFIX), MMUSUFFIX)
> +        res2 = glue(glue(helper_ret_ld, USUFFIX), MMUSUFFIX)
>              (env, addr2, mmu_idx, retaddr + GETPC_ADJ);
>          shift = (addr & (DATA_SIZE - 1)) * 8;
>  #ifdef TARGET_WORDS_BIGENDIAN
> @@ -147,19 +163,33 @@ glue(glue(helper_ret_ld, SUFFIX), 
> MMUSUFFIX)(CPUArchState *env,
>  #endif
>  
>      haddr = addr + env->tlb_table[mmu_idx][index].addend;
> -    return glue(glue(ld, LSUFFIX), _raw)((uint8_t *)haddr);
> +    /* Note that ldl_raw is defined with type "int".  */
> +    return (DATA_TYPE) glue(glue(ld, LSUFFIX), _raw)((uint8_t *)haddr);
>  }
>  
>  DATA_TYPE
>  glue(glue(helper_ld, SUFFIX), MMUSUFFIX)(CPUArchState *env, target_ulong 
> addr,
>                                           int mmu_idx)
>  {
> -    return glue(glue(helper_ret_ld, SUFFIX), MMUSUFFIX)(env, addr, mmu_idx,
> +    return glue(glue(helper_ret_ld, USUFFIX), MMUSUFFIX)(env, addr, mmu_idx,
>                                                          GETRA_EXT());
>  }
>  
>  #ifndef SOFTMMU_CODE_ACCESS
>  
> +/* Provide signed versions of the load routines as well.  We can of course
> +   avoid this for 64-bit data, or for 32-bit data on 32-bit host.  */
> +#if DATA_SIZE * 8 < TCG_TARGET_REG_BITS
> +WORD_TYPE
> +glue(glue(helper_ret_ld, SSUFFIX), MMUSUFFIX)(CPUArchState *env,
> +                                              target_ulong addr, int mmu_idx,
> +                                              uintptr_t retaddr)
> +{
> +    return (SDATA_TYPE) glue(glue(helper_ret_ld, USUFFIX), MMUSUFFIX)
> +        (env, addr, mmu_idx, retaddr);
> +}
> +#endif
> +
>  static inline void glue(io_write, SUFFIX)(CPUArchState *env,
>                                            hwaddr physaddr,
>                                            DATA_TYPE val,
> @@ -267,3 +297,7 @@ glue(glue(helper_st, SUFFIX), MMUSUFFIX)(CPUArchState 
> *env, target_ulong addr,
>  #undef LSUFFIX
>  #undef DATA_SIZE
>  #undef ADDR_READ
> +#undef WORD_TYPE
> +#undef SDATA_TYPE
> +#undef USUFFIX
> +#undef SSUFFIX
> diff --git a/tcg/i386/tcg-target.c b/tcg/i386/tcg-target.c
> index 4c98cc4..5aee0fa 100644
> --- a/tcg/i386/tcg-target.c
> +++ b/tcg/i386/tcg-target.c
> @@ -1026,9 +1026,9 @@ static void tcg_out_jmp(TCGContext *s, tcg_target_long 
> dest)
>   *                                     int mmu_idx, uintptr_t ra)
>   */
>  static const void * const qemu_ld_helpers[4] = {
> -    helper_ret_ldb_mmu,
> -    helper_ret_ldw_mmu,
> -    helper_ret_ldl_mmu,
> +    helper_ret_ldub_mmu,
> +    helper_ret_lduw_mmu,
> +    helper_ret_ldul_mmu,
>      helper_ret_ldq_mmu,
>  };
>  
> diff --git a/tcg/tcg.h b/tcg/tcg.h
> index eeff6b7..27f6ee5 100644
> --- a/tcg/tcg.h
> +++ b/tcg/tcg.h
> @@ -750,15 +750,24 @@ void tcg_out_tb_finalize(TCGContext *s);
>   * Memory helpers that will be used by TCG generated code.
>   */
>  #ifdef CONFIG_SOFTMMU
> -uint8_t helper_ret_ldb_mmu(CPUArchState *env, target_ulong addr,
> -                           int mmu_idx, uintptr_t retaddr);
> -uint16_t helper_ret_ldw_mmu(CPUArchState *env, target_ulong addr,
> -                            int mmu_idx, uintptr_t retaddr);
> -uint32_t helper_ret_ldl_mmu(CPUArchState *env, target_ulong addr,
> -                            int mmu_idx, uintptr_t retaddr);
> +/* Value zero-extended to tcg register size.  */
> +tcg_target_ulong helper_ret_ldub_mmu(CPUArchState *env, target_ulong addr,
> +                                     int mmu_idx, uintptr_t retaddr);
> +tcg_target_ulong helper_ret_lduw_mmu(CPUArchState *env, target_ulong addr,
> +                                     int mmu_idx, uintptr_t retaddr);
> +tcg_target_ulong helper_ret_ldul_mmu(CPUArchState *env, target_ulong addr,
> +                                     int mmu_idx, uintptr_t retaddr);
>  uint64_t helper_ret_ldq_mmu(CPUArchState *env, target_ulong addr,
>                              int mmu_idx, uintptr_t retaddr);
>  
> +/* Value sign-extended to tcg register size.  */
> +tcg_target_ulong helper_ret_ldsb_mmu(CPUArchState *env, target_ulong addr,
> +                                     int mmu_idx, uintptr_t retaddr);
> +tcg_target_ulong helper_ret_ldsw_mmu(CPUArchState *env, target_ulong addr,
> +                                     int mmu_idx, uintptr_t retaddr);
> +tcg_target_ulong helper_ret_ldsl_mmu(CPUArchState *env, target_ulong addr,
> +                                     int mmu_idx, uintptr_t retaddr);
> +
>  void helper_ret_stb_mmu(CPUArchState *env, target_ulong addr, uint8_t val,
>                          int mmu_idx, uintptr_t retaddr);
>  void helper_ret_stw_mmu(CPUArchState *env, target_ulong addr, uint16_t val,

While it works for x86 and some other architectures, it makes the
assumption that only part of the register can be used later by the TCG
code. It won't be the case if we later (and I hope we will) implement a
MIPS64 TCG target. In that case, a 32-bit value has to be returned
signed extended, which won't be the case for example for a 32-bit guest
loading a 16-bit unsigned value.

-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
address@hidden                 http://www.aurel32.net