[PATCH v2 02/28] cputlb: Use trace_mem_get_info instead of trace_mem_build_info

Richard Henderson posted 28 patches 6 years, 1 month ago
There is a newer version of this series
[PATCH v2 02/28] cputlb: Use trace_mem_get_info instead of trace_mem_build_info
Posted by Richard Henderson 6 years, 1 month ago
In the cpu_ldst templates, we already require a MemOp, and it
is cleaner and clearer to pass that instead of 3 separate
arguments describing the memory operation.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 include/exec/cpu_ldst_template.h          | 22 +++++++++++-----------
 include/exec/cpu_ldst_useronly_template.h | 12 ++++++------
 2 files changed, 17 insertions(+), 17 deletions(-)

diff --git a/include/exec/cpu_ldst_template.h b/include/exec/cpu_ldst_template.h
index 54b5e858ce..0ad5de3ef9 100644
--- a/include/exec/cpu_ldst_template.h
+++ b/include/exec/cpu_ldst_template.h
@@ -86,9 +86,9 @@ glue(glue(glue(cpu_ld, USUFFIX), MEMSUFFIX), _ra)(CPUArchState *env,
     RES_TYPE res;
     target_ulong addr;
     int mmu_idx = CPU_MMU_INDEX;
-    TCGMemOpIdx oi;
+    MemOp op = MO_TE | SHIFT;
 #if !defined(SOFTMMU_CODE_ACCESS)
-    uint16_t meminfo = trace_mem_build_info(SHIFT, false, MO_TE, false, mmu_idx);
+    uint16_t meminfo = trace_mem_get_info(op, mmu_idx, false);
     trace_guest_mem_before_exec(env_cpu(env), ptr, meminfo);
 #endif
 
@@ -96,9 +96,9 @@ glue(glue(glue(cpu_ld, USUFFIX), MEMSUFFIX), _ra)(CPUArchState *env,
     entry = tlb_entry(env, mmu_idx, addr);
     if (unlikely(entry->ADDR_READ !=
                  (addr & (TARGET_PAGE_MASK | (DATA_SIZE - 1))))) {
-        oi = make_memop_idx(SHIFT, mmu_idx);
+        TCGMemOpIdx oi = make_memop_idx(op, mmu_idx);
         res = glue(glue(helper_ret_ld, URETSUFFIX), MMUSUFFIX)(env, addr,
-                                                            oi, retaddr);
+                                                               oi, retaddr);
     } else {
         uintptr_t hostaddr = addr + entry->addend;
         res = glue(glue(ld, USUFFIX), _p)((uint8_t *)hostaddr);
@@ -125,9 +125,9 @@ glue(glue(glue(cpu_lds, SUFFIX), MEMSUFFIX), _ra)(CPUArchState *env,
     int res;
     target_ulong addr;
     int mmu_idx = CPU_MMU_INDEX;
-    TCGMemOpIdx oi;
-#if !defined(SOFTMMU_CODE_ACCESS)
-    uint16_t meminfo = trace_mem_build_info(SHIFT, true, MO_TE, false, mmu_idx);
+    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
 
@@ -135,7 +135,7 @@ glue(glue(glue(cpu_lds, SUFFIX), MEMSUFFIX), _ra)(CPUArchState *env,
     entry = tlb_entry(env, mmu_idx, addr);
     if (unlikely(entry->ADDR_READ !=
                  (addr & (TARGET_PAGE_MASK | (DATA_SIZE - 1))))) {
-        oi = make_memop_idx(SHIFT, mmu_idx);
+        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 {
@@ -167,9 +167,9 @@ glue(glue(glue(cpu_st, SUFFIX), MEMSUFFIX), _ra)(CPUArchState *env,
     CPUTLBEntry *entry;
     target_ulong addr;
     int mmu_idx = CPU_MMU_INDEX;
-    TCGMemOpIdx oi;
+    MemOp op = MO_TE | SHIFT;
 #if !defined(SOFTMMU_CODE_ACCESS)
-    uint16_t meminfo = trace_mem_build_info(SHIFT, false, MO_TE, true, mmu_idx);
+    uint16_t meminfo = trace_mem_get_info(op, mmu_idx, true);
     trace_guest_mem_before_exec(env_cpu(env), ptr, meminfo);
 #endif
 
@@ -177,7 +177,7 @@ glue(glue(glue(cpu_st, SUFFIX), MEMSUFFIX), _ra)(CPUArchState *env,
     entry = tlb_entry(env, mmu_idx, addr);
     if (unlikely(tlb_addr_write(entry) !=
                  (addr & (TARGET_PAGE_MASK | (DATA_SIZE - 1))))) {
-        oi = make_memop_idx(SHIFT, mmu_idx);
+        TCGMemOpIdx oi = make_memop_idx(op, mmu_idx);
         glue(glue(helper_ret_st, SUFFIX), MMUSUFFIX)(env, addr, v, oi,
                                                      retaddr);
     } else {
diff --git a/include/exec/cpu_ldst_useronly_template.h b/include/exec/cpu_ldst_useronly_template.h
index dbdc7a845d..e5a3d1983a 100644
--- a/include/exec/cpu_ldst_useronly_template.h
+++ b/include/exec/cpu_ldst_useronly_template.h
@@ -70,8 +70,8 @@ glue(glue(cpu_ld, USUFFIX), MEMSUFFIX)(CPUArchState *env, abi_ptr ptr)
     ret = glue(glue(ld, USUFFIX), _p)(g2h(ptr));
     clear_helper_retaddr();
 #else
-    uint16_t meminfo = trace_mem_build_info(SHIFT, false, MO_TE, false,
-                                            MMU_USER_IDX);
+    MemOp op = MO_TE | SHIFT;
+    uint16_t meminfo = trace_mem_get_info(op, MMU_USER_IDX, false);
     trace_guest_mem_before_exec(env_cpu(env), ptr, meminfo);
     ret = glue(glue(ld, USUFFIX), _p)(g2h(ptr));
 #endif
@@ -102,8 +102,8 @@ glue(glue(cpu_lds, SUFFIX), MEMSUFFIX)(CPUArchState *env, abi_ptr ptr)
     ret = glue(glue(lds, SUFFIX), _p)(g2h(ptr));
     clear_helper_retaddr();
 #else
-    uint16_t meminfo = trace_mem_build_info(SHIFT, true, MO_TE, false,
-                                            MMU_USER_IDX);
+    MemOp op = MO_TE | MO_SIGN | SHIFT;
+    uint16_t meminfo = trace_mem_get_info(op, MMU_USER_IDX, false);
     trace_guest_mem_before_exec(env_cpu(env), ptr, meminfo);
     ret = glue(glue(lds, SUFFIX), _p)(g2h(ptr));
     qemu_plugin_vcpu_mem_cb(env_cpu(env), ptr, meminfo);
@@ -131,8 +131,8 @@ static inline void
 glue(glue(cpu_st, SUFFIX), MEMSUFFIX)(CPUArchState *env, abi_ptr ptr,
                                       RES_TYPE v)
 {
-    uint16_t meminfo = trace_mem_build_info(SHIFT, false, MO_TE, true,
-                                            MMU_USER_IDX);
+    MemOp op = MO_TE | SHIFT;
+    uint16_t meminfo = trace_mem_get_info(op, MMU_USER_IDX, true);
     trace_guest_mem_before_exec(env_cpu(env), ptr, meminfo);
     glue(glue(st, SUFFIX), _p)(g2h(ptr), v);
     qemu_plugin_vcpu_mem_cb(env_cpu(env), ptr, meminfo);
-- 
2.20.1


Re: [PATCH v2 02/28] cputlb: Use trace_mem_get_info instead of trace_mem_build_info
Posted by Philippe Mathieu-Daudé 6 years, 1 month ago
On 12/16/19 11:11 PM, Richard Henderson wrote:
> In the cpu_ldst templates, we already require a MemOp, and it
> is cleaner and clearer to pass that instead of 3 separate
> arguments describing the memory operation.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   include/exec/cpu_ldst_template.h          | 22 +++++++++++-----------
>   include/exec/cpu_ldst_useronly_template.h | 12 ++++++------
>   2 files changed, 17 insertions(+), 17 deletions(-)
> 
> diff --git a/include/exec/cpu_ldst_template.h b/include/exec/cpu_ldst_template.h
> index 54b5e858ce..0ad5de3ef9 100644
> --- a/include/exec/cpu_ldst_template.h
> +++ b/include/exec/cpu_ldst_template.h
> @@ -86,9 +86,9 @@ glue(glue(glue(cpu_ld, USUFFIX), MEMSUFFIX), _ra)(CPUArchState *env,
>       RES_TYPE res;
>       target_ulong addr;
>       int mmu_idx = CPU_MMU_INDEX;
> -    TCGMemOpIdx oi;
> +    MemOp op = MO_TE | SHIFT;
>   #if !defined(SOFTMMU_CODE_ACCESS)
> -    uint16_t meminfo = trace_mem_build_info(SHIFT, false, MO_TE, false, mmu_idx);
> +    uint16_t meminfo = trace_mem_get_info(op, mmu_idx, false);
>       trace_guest_mem_before_exec(env_cpu(env), ptr, meminfo);
>   #endif
>   
> @@ -96,9 +96,9 @@ glue(glue(glue(cpu_ld, USUFFIX), MEMSUFFIX), _ra)(CPUArchState *env,
>       entry = tlb_entry(env, mmu_idx, addr);
>       if (unlikely(entry->ADDR_READ !=
>                    (addr & (TARGET_PAGE_MASK | (DATA_SIZE - 1))))) {
> -        oi = make_memop_idx(SHIFT, mmu_idx);
> +        TCGMemOpIdx oi = make_memop_idx(op, mmu_idx);
>           res = glue(glue(helper_ret_ld, URETSUFFIX), MMUSUFFIX)(env, addr,
> -                                                            oi, retaddr);
> +                                                               oi, retaddr);
>       } else {
>           uintptr_t hostaddr = addr + entry->addend;
>           res = glue(glue(ld, USUFFIX), _p)((uint8_t *)hostaddr);
> @@ -125,9 +125,9 @@ glue(glue(glue(cpu_lds, SUFFIX), MEMSUFFIX), _ra)(CPUArchState *env,
>       int res;
>       target_ulong addr;
>       int mmu_idx = CPU_MMU_INDEX;
> -    TCGMemOpIdx oi;
> -#if !defined(SOFTMMU_CODE_ACCESS)
> -    uint16_t meminfo = trace_mem_build_info(SHIFT, true, MO_TE, false, mmu_idx);
> +    MemOp op = MO_TE | MO_SIGN | SHIFT;

Good, the sign-extend is easier to review this way (than 'true').

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> +#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
>   
> @@ -135,7 +135,7 @@ glue(glue(glue(cpu_lds, SUFFIX), MEMSUFFIX), _ra)(CPUArchState *env,
>       entry = tlb_entry(env, mmu_idx, addr);
>       if (unlikely(entry->ADDR_READ !=
>                    (addr & (TARGET_PAGE_MASK | (DATA_SIZE - 1))))) {
> -        oi = make_memop_idx(SHIFT, mmu_idx);
> +        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 {
> @@ -167,9 +167,9 @@ glue(glue(glue(cpu_st, SUFFIX), MEMSUFFIX), _ra)(CPUArchState *env,
>       CPUTLBEntry *entry;
>       target_ulong addr;
>       int mmu_idx = CPU_MMU_INDEX;
> -    TCGMemOpIdx oi;
> +    MemOp op = MO_TE | SHIFT;
>   #if !defined(SOFTMMU_CODE_ACCESS)
> -    uint16_t meminfo = trace_mem_build_info(SHIFT, false, MO_TE, true, mmu_idx);
> +    uint16_t meminfo = trace_mem_get_info(op, mmu_idx, true);
>       trace_guest_mem_before_exec(env_cpu(env), ptr, meminfo);
>   #endif
>   
> @@ -177,7 +177,7 @@ glue(glue(glue(cpu_st, SUFFIX), MEMSUFFIX), _ra)(CPUArchState *env,
>       entry = tlb_entry(env, mmu_idx, addr);
>       if (unlikely(tlb_addr_write(entry) !=
>                    (addr & (TARGET_PAGE_MASK | (DATA_SIZE - 1))))) {
> -        oi = make_memop_idx(SHIFT, mmu_idx);
> +        TCGMemOpIdx oi = make_memop_idx(op, mmu_idx);
>           glue(glue(helper_ret_st, SUFFIX), MMUSUFFIX)(env, addr, v, oi,
>                                                        retaddr);
>       } else {
> diff --git a/include/exec/cpu_ldst_useronly_template.h b/include/exec/cpu_ldst_useronly_template.h
> index dbdc7a845d..e5a3d1983a 100644
> --- a/include/exec/cpu_ldst_useronly_template.h
> +++ b/include/exec/cpu_ldst_useronly_template.h
> @@ -70,8 +70,8 @@ glue(glue(cpu_ld, USUFFIX), MEMSUFFIX)(CPUArchState *env, abi_ptr ptr)
>       ret = glue(glue(ld, USUFFIX), _p)(g2h(ptr));
>       clear_helper_retaddr();
>   #else
> -    uint16_t meminfo = trace_mem_build_info(SHIFT, false, MO_TE, false,
> -                                            MMU_USER_IDX);
> +    MemOp op = MO_TE | SHIFT;
> +    uint16_t meminfo = trace_mem_get_info(op, MMU_USER_IDX, false);
>       trace_guest_mem_before_exec(env_cpu(env), ptr, meminfo);
>       ret = glue(glue(ld, USUFFIX), _p)(g2h(ptr));
>   #endif
> @@ -102,8 +102,8 @@ glue(glue(cpu_lds, SUFFIX), MEMSUFFIX)(CPUArchState *env, abi_ptr ptr)
>       ret = glue(glue(lds, SUFFIX), _p)(g2h(ptr));
>       clear_helper_retaddr();
>   #else
> -    uint16_t meminfo = trace_mem_build_info(SHIFT, true, MO_TE, false,
> -                                            MMU_USER_IDX);
> +    MemOp op = MO_TE | MO_SIGN | SHIFT;
> +    uint16_t meminfo = trace_mem_get_info(op, MMU_USER_IDX, false);
>       trace_guest_mem_before_exec(env_cpu(env), ptr, meminfo);
>       ret = glue(glue(lds, SUFFIX), _p)(g2h(ptr));
>       qemu_plugin_vcpu_mem_cb(env_cpu(env), ptr, meminfo);
> @@ -131,8 +131,8 @@ static inline void
>   glue(glue(cpu_st, SUFFIX), MEMSUFFIX)(CPUArchState *env, abi_ptr ptr,
>                                         RES_TYPE v)
>   {
> -    uint16_t meminfo = trace_mem_build_info(SHIFT, false, MO_TE, true,
> -                                            MMU_USER_IDX);
> +    MemOp op = MO_TE | SHIFT;
> +    uint16_t meminfo = trace_mem_get_info(op, MMU_USER_IDX, true);
>       trace_guest_mem_before_exec(env_cpu(env), ptr, meminfo);
>       glue(glue(st, SUFFIX), _p)(g2h(ptr), v);
>       qemu_plugin_vcpu_mem_cb(env_cpu(env), ptr, meminfo);
> 


Re: [PATCH v2 02/28] cputlb: Use trace_mem_get_info instead of trace_mem_build_info
Posted by Alex Bennée 6 years, 1 month ago
Richard Henderson <richard.henderson@linaro.org> writes:

> In the cpu_ldst templates, we already require a MemOp, and it
> is cleaner and clearer to pass that instead of 3 separate
> arguments describing the memory operation.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

> ---
>  include/exec/cpu_ldst_template.h          | 22 +++++++++++-----------
>  include/exec/cpu_ldst_useronly_template.h | 12 ++++++------
>  2 files changed, 17 insertions(+), 17 deletions(-)
>
> diff --git a/include/exec/cpu_ldst_template.h b/include/exec/cpu_ldst_template.h
> index 54b5e858ce..0ad5de3ef9 100644
> --- a/include/exec/cpu_ldst_template.h
> +++ b/include/exec/cpu_ldst_template.h
> @@ -86,9 +86,9 @@ glue(glue(glue(cpu_ld, USUFFIX), MEMSUFFIX), _ra)(CPUArchState *env,
>      RES_TYPE res;
>      target_ulong addr;
>      int mmu_idx = CPU_MMU_INDEX;
> -    TCGMemOpIdx oi;
> +    MemOp op = MO_TE | SHIFT;
>  #if !defined(SOFTMMU_CODE_ACCESS)
> -    uint16_t meminfo = trace_mem_build_info(SHIFT, false, MO_TE, false, mmu_idx);
> +    uint16_t meminfo = trace_mem_get_info(op, mmu_idx, false);
>      trace_guest_mem_before_exec(env_cpu(env), ptr, meminfo);
>  #endif
>  
> @@ -96,9 +96,9 @@ glue(glue(glue(cpu_ld, USUFFIX), MEMSUFFIX), _ra)(CPUArchState *env,
>      entry = tlb_entry(env, mmu_idx, addr);
>      if (unlikely(entry->ADDR_READ !=
>                   (addr & (TARGET_PAGE_MASK | (DATA_SIZE - 1))))) {
> -        oi = make_memop_idx(SHIFT, mmu_idx);
> +        TCGMemOpIdx oi = make_memop_idx(op, mmu_idx);
>          res = glue(glue(helper_ret_ld, URETSUFFIX), MMUSUFFIX)(env, addr,
> -                                                            oi, retaddr);
> +                                                               oi, retaddr);
>      } else {
>          uintptr_t hostaddr = addr + entry->addend;
>          res = glue(glue(ld, USUFFIX), _p)((uint8_t *)hostaddr);
> @@ -125,9 +125,9 @@ glue(glue(glue(cpu_lds, SUFFIX), MEMSUFFIX), _ra)(CPUArchState *env,
>      int res;
>      target_ulong addr;
>      int mmu_idx = CPU_MMU_INDEX;
> -    TCGMemOpIdx oi;
> -#if !defined(SOFTMMU_CODE_ACCESS)
> -    uint16_t meminfo = trace_mem_build_info(SHIFT, true, MO_TE, false, mmu_idx);
> +    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
>  
> @@ -135,7 +135,7 @@ glue(glue(glue(cpu_lds, SUFFIX), MEMSUFFIX), _ra)(CPUArchState *env,
>      entry = tlb_entry(env, mmu_idx, addr);
>      if (unlikely(entry->ADDR_READ !=
>                   (addr & (TARGET_PAGE_MASK | (DATA_SIZE - 1))))) {
> -        oi = make_memop_idx(SHIFT, mmu_idx);
> +        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 {
> @@ -167,9 +167,9 @@ glue(glue(glue(cpu_st, SUFFIX), MEMSUFFIX), _ra)(CPUArchState *env,
>      CPUTLBEntry *entry;
>      target_ulong addr;
>      int mmu_idx = CPU_MMU_INDEX;
> -    TCGMemOpIdx oi;
> +    MemOp op = MO_TE | SHIFT;
>  #if !defined(SOFTMMU_CODE_ACCESS)
> -    uint16_t meminfo = trace_mem_build_info(SHIFT, false, MO_TE, true, mmu_idx);
> +    uint16_t meminfo = trace_mem_get_info(op, mmu_idx, true);
>      trace_guest_mem_before_exec(env_cpu(env), ptr, meminfo);
>  #endif
>  
> @@ -177,7 +177,7 @@ glue(glue(glue(cpu_st, SUFFIX), MEMSUFFIX), _ra)(CPUArchState *env,
>      entry = tlb_entry(env, mmu_idx, addr);
>      if (unlikely(tlb_addr_write(entry) !=
>                   (addr & (TARGET_PAGE_MASK | (DATA_SIZE - 1))))) {
> -        oi = make_memop_idx(SHIFT, mmu_idx);
> +        TCGMemOpIdx oi = make_memop_idx(op, mmu_idx);
>          glue(glue(helper_ret_st, SUFFIX), MMUSUFFIX)(env, addr, v, oi,
>                                                       retaddr);
>      } else {
> diff --git a/include/exec/cpu_ldst_useronly_template.h b/include/exec/cpu_ldst_useronly_template.h
> index dbdc7a845d..e5a3d1983a 100644
> --- a/include/exec/cpu_ldst_useronly_template.h
> +++ b/include/exec/cpu_ldst_useronly_template.h
> @@ -70,8 +70,8 @@ glue(glue(cpu_ld, USUFFIX), MEMSUFFIX)(CPUArchState *env, abi_ptr ptr)
>      ret = glue(glue(ld, USUFFIX), _p)(g2h(ptr));
>      clear_helper_retaddr();
>  #else
> -    uint16_t meminfo = trace_mem_build_info(SHIFT, false, MO_TE, false,
> -                                            MMU_USER_IDX);
> +    MemOp op = MO_TE | SHIFT;
> +    uint16_t meminfo = trace_mem_get_info(op, MMU_USER_IDX, false);
>      trace_guest_mem_before_exec(env_cpu(env), ptr, meminfo);
>      ret = glue(glue(ld, USUFFIX), _p)(g2h(ptr));
>  #endif
> @@ -102,8 +102,8 @@ glue(glue(cpu_lds, SUFFIX), MEMSUFFIX)(CPUArchState *env, abi_ptr ptr)
>      ret = glue(glue(lds, SUFFIX), _p)(g2h(ptr));
>      clear_helper_retaddr();
>  #else
> -    uint16_t meminfo = trace_mem_build_info(SHIFT, true, MO_TE, false,
> -                                            MMU_USER_IDX);
> +    MemOp op = MO_TE | MO_SIGN | SHIFT;
> +    uint16_t meminfo = trace_mem_get_info(op, MMU_USER_IDX, false);
>      trace_guest_mem_before_exec(env_cpu(env), ptr, meminfo);
>      ret = glue(glue(lds, SUFFIX), _p)(g2h(ptr));
>      qemu_plugin_vcpu_mem_cb(env_cpu(env), ptr, meminfo);
> @@ -131,8 +131,8 @@ static inline void
>  glue(glue(cpu_st, SUFFIX), MEMSUFFIX)(CPUArchState *env, abi_ptr ptr,
>                                        RES_TYPE v)
>  {
> -    uint16_t meminfo = trace_mem_build_info(SHIFT, false, MO_TE, true,
> -                                            MMU_USER_IDX);
> +    MemOp op = MO_TE | SHIFT;
> +    uint16_t meminfo = trace_mem_get_info(op, MMU_USER_IDX, true);
>      trace_guest_mem_before_exec(env_cpu(env), ptr, meminfo);
>      glue(glue(st, SUFFIX), _p)(g2h(ptr), v);
>      qemu_plugin_vcpu_mem_cb(env_cpu(env), ptr, meminfo);


-- 
Alex Bennée