[Qemu-devel] [PATCH 0/5] trace: Add events for vCPU memory accesses

[Qemu-devel] [PATCH 0/5] trace: Add events for vCPU memory accesses

[Lluís Vilanova]

NOTE: This series applies on top of "trace: Show vCPU info in guest code events"

This series adds to new events:

* guest_vmem: memory accesses performed by vCPUs (guest code)

* guest_vmem_user_syscall: memory accesses performed by syscall emulation when
  running QEMU in user-mode.

Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
---

Lluís Vilanova (5):
      exec: [tcg] Track which vCPU is performing translation and execution
      trace: [all] Add "guest_vmem" event
      user: Refactor lock_user body into do_lock_user
      user: Set current vCPU during syscall execution
      trace: [all] Add "guest_vmem_user_syscall" event


 bsd-user/qemu.h                           |   21 +++++++++++++++---
 bsd-user/syscall.c                        |    2 ++
 bsd-user/uaccess.c                        |    2 +-
 include/exec/cpu_ldst_template.h          |   17 +++++++++++++++
 include/exec/cpu_ldst_useronly_template.h |   14 ++++++++++++
 linux-user/qemu.h                         |   21 +++++++++++++++---
 linux-user/syscall.c                      |    2 ++
 linux-user/uaccess.c                      |    2 +-
 target-alpha/translate.c                  |    1 +
 target-arm/translate.c                    |    1 +
 target-cris/translate.c                   |    1 +
 target-cris/translate_v10.c               |    1 +
 target-i386/translate.c                   |    1 +
 target-lm32/translate.c                   |    1 +
 target-m68k/translate.c                   |    1 +
 target-microblaze/translate.c             |    1 +
 target-mips/translate.c                   |    1 +
 target-moxie/translate.c                  |    1 +
 target-openrisc/translate.c               |    1 +
 target-ppc/translate.c                    |    1 +
 target-s390x/translate.c                  |    1 +
 target-sh4/translate.c                    |    1 +
 target-sparc/translate.c                  |    1 +
 target-tilegx/translate.c                 |    1 +
 target-tricore/translate.c                |    1 +
 target-unicore32/translate.c              |    1 +
 target-xtensa/translate.c                 |    1 +
 tcg/tcg-op.c                              |   34 ++++++++++++++++++++++++++---
 tcg/tcg.h                                 |    4 +++
 trace-events                              |   23 ++++++++++++++++++++
 translate-all.c                           |    2 ++
 31 files changed, 151 insertions(+), 12 deletions(-)


To: qemu-devel@nongnu.org
Cc: Stefan Hajnoczi <stefanha@redhat.com>

[Qemu-devel] [PATCH 1/5] exec: [tcg] Track which vCPU is performing translation and execution

[Lluís Vilanova]

Information is tracked inside the TCGContext structure, and later used
by tracing events with the 'tcg' and 'vcpu' properties.

The 'cpu' field is used to check tracing of translation-time
events ("*_trans"). The 'tcg_env' field is used to pass it to
execution-time events ("*_exec").

Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
---
 target-alpha/translate.c      |    1 +
 target-arm/translate.c        |    1 +
 target-cris/translate.c       |    1 +
 target-cris/translate_v10.c   |    1 +
 target-i386/translate.c       |    1 +
 target-lm32/translate.c       |    1 +
 target-m68k/translate.c       |    1 +
 target-microblaze/translate.c |    1 +
 target-mips/translate.c       |    1 +
 target-moxie/translate.c      |    1 +
 target-openrisc/translate.c   |    1 +
 target-ppc/translate.c        |    1 +
 target-s390x/translate.c      |    1 +
 target-sh4/translate.c        |    1 +
 target-sparc/translate.c      |    1 +
 target-tilegx/translate.c     |    1 +
 target-tricore/translate.c    |    1 +
 target-unicore32/translate.c  |    1 +
 target-xtensa/translate.c     |    1 +
 tcg/tcg.h                     |    4 ++++
 translate-all.c               |    2 ++
 21 files changed, 25 insertions(+)

diff --git a/target-alpha/translate.c b/target-alpha/translate.c
index 7b798b0..aebe303 100644
--- a/target-alpha/translate.c
+++ b/target-alpha/translate.c
@@ -150,6 +150,7 @@ void alpha_translate_init(void)
     done_init = 1;
 
     cpu_env = tcg_global_reg_new_ptr(TCG_AREG0, "env");
+    tcg_ctx.tcg_env = cpu_env;
 
     for (i = 0; i < 31; i++) {
         cpu_std_ir[i] = tcg_global_mem_new_i64(cpu_env,
diff --git a/target-arm/translate.c b/target-arm/translate.c
index e69145d..5f8ea6b 100644
--- a/target-arm/translate.c
+++ b/target-arm/translate.c
@@ -84,6 +84,7 @@ void arm_translate_init(void)
     int i;
 
     cpu_env = tcg_global_reg_new_ptr(TCG_AREG0, "env");
+    tcg_ctx.tcg_env = cpu_env;
 
     for (i = 0; i < 16; i++) {
         cpu_R[i] = tcg_global_mem_new_i32(cpu_env,
diff --git a/target-cris/translate.c b/target-cris/translate.c
index 2a283e0..8d8e699 100644
--- a/target-cris/translate.c
+++ b/target-cris/translate.c
@@ -3364,6 +3364,7 @@ void cris_initialize_tcg(void)
     int i;
 
     cpu_env = tcg_global_reg_new_ptr(TCG_AREG0, "env");
+    tcg_ctx.tcg_env = cpu_env;
     cc_x = tcg_global_mem_new(cpu_env,
                               offsetof(CPUCRISState, cc_x), "cc_x");
     cc_src = tcg_global_mem_new(cpu_env,
diff --git a/target-cris/translate_v10.c b/target-cris/translate_v10.c
index 7607ead..f2e9768 100644
--- a/target-cris/translate_v10.c
+++ b/target-cris/translate_v10.c
@@ -1250,6 +1250,7 @@ void cris_initialize_crisv10_tcg(void)
     int i;
 
     cpu_env = tcg_global_reg_new_ptr(TCG_AREG0, "env");
+    tcg_ctx.tcg_env = cpu_env;
     cc_x = tcg_global_mem_new(cpu_env,
                               offsetof(CPUCRISState, cc_x), "cc_x");
     cc_src = tcg_global_mem_new(cpu_env,
diff --git a/target-i386/translate.c b/target-i386/translate.c
index 9171929..ca2854d 100644
--- a/target-i386/translate.c
+++ b/target-i386/translate.c
@@ -8075,6 +8075,7 @@ void tcg_x86_init(void)
     int i;
 
     cpu_env = tcg_global_reg_new_ptr(TCG_AREG0, "env");
+    tcg_ctx.tcg_env = cpu_env;
     cpu_cc_op = tcg_global_mem_new_i32(cpu_env,
                                        offsetof(CPUX86State, cc_op), "cc_op");
     cpu_cc_dst = tcg_global_mem_new(cpu_env, offsetof(CPUX86State, cc_dst),
diff --git a/target-lm32/translate.c b/target-lm32/translate.c
index 3877993..e373d87 100644
--- a/target-lm32/translate.c
+++ b/target-lm32/translate.c
@@ -1191,6 +1191,7 @@ void lm32_translate_init(void)
     int i;
 
     cpu_env = tcg_global_reg_new_ptr(TCG_AREG0, "env");
+    tcg_ctx.tcg_env = cpu_env;
 
     for (i = 0; i < ARRAY_SIZE(cpu_R); i++) {
         cpu_R[i] = tcg_global_mem_new(cpu_env,
diff --git a/target-m68k/translate.c b/target-m68k/translate.c
index 085cb6a..d325c72 100644
--- a/target-m68k/translate.c
+++ b/target-m68k/translate.c
@@ -77,6 +77,7 @@ void m68k_tcg_init(void)
     int i;
 
     cpu_env = tcg_global_reg_new_ptr(TCG_AREG0, "env");
+    tcg_ctx.tcg_env = cpu_env;
 
 #define DEFO32(name, offset) \
     QREG_##name = tcg_global_mem_new_i32(cpu_env, \
diff --git a/target-microblaze/translate.c b/target-microblaze/translate.c
index 296c4d7..bc7bcf9 100644
--- a/target-microblaze/translate.c
+++ b/target-microblaze/translate.c
@@ -1869,6 +1869,7 @@ void mb_tcg_init(void)
     int i;
 
     cpu_env = tcg_global_reg_new_ptr(TCG_AREG0, "env");
+    tcg_ctx.tcg_env = cpu_env;
 
     env_debug = tcg_global_mem_new(cpu_env,
                     offsetof(CPUMBState, debug),
diff --git a/target-mips/translate.c b/target-mips/translate.c
index 658926d..38969cb 100644
--- a/target-mips/translate.c
+++ b/target-mips/translate.c
@@ -19829,6 +19829,7 @@ void mips_tcg_init(void)
         return;
 
     cpu_env = tcg_global_reg_new_ptr(TCG_AREG0, "env");
+    tcg_ctx.tcg_env = cpu_env;
 
     TCGV_UNUSED(cpu_gpr[0]);
     for (i = 1; i < 32; i++)
diff --git a/target-moxie/translate.c b/target-moxie/translate.c
index bc860a5..fccc011 100644
--- a/target-moxie/translate.c
+++ b/target-moxie/translate.c
@@ -106,6 +106,7 @@ void moxie_translate_init(void)
         return;
     }
     cpu_env = tcg_global_reg_new_ptr(TCG_AREG0, "env");
+    tcg_ctx.tcg_env = cpu_env;
     cpu_pc = tcg_global_mem_new_i32(cpu_env,
                                     offsetof(CPUMoxieState, pc), "$pc");
     for (i = 0; i < 16; i++)
diff --git a/target-openrisc/translate.c b/target-openrisc/translate.c
index d25324e..ae1e73a 100644
--- a/target-openrisc/translate.c
+++ b/target-openrisc/translate.c
@@ -78,6 +78,7 @@ void openrisc_translate_init(void)
     int i;
 
     cpu_env = tcg_global_reg_new_ptr(TCG_AREG0, "env");
+    tcg_ctx.tcg_env = cpu_env;
     cpu_sr = tcg_global_mem_new(cpu_env,
                                 offsetof(CPUOpenRISCState, sr), "sr");
     env_flags = tcg_global_mem_new_i32(cpu_env,
diff --git a/target-ppc/translate.c b/target-ppc/translate.c
index ecc85f0..56f4212 100644
--- a/target-ppc/translate.c
+++ b/target-ppc/translate.c
@@ -87,6 +87,7 @@ void ppc_translate_init(void)
         return;
 
     cpu_env = tcg_global_reg_new_ptr(TCG_AREG0, "env");
+    tcg_ctx.tcg_env = cpu_env;
 
     p = cpu_reg_names;
     cpu_reg_names_size = sizeof(cpu_reg_names);
diff --git a/target-s390x/translate.c b/target-s390x/translate.c
index 82e1165..db02983 100644
--- a/target-s390x/translate.c
+++ b/target-s390x/translate.c
@@ -168,6 +168,7 @@ void s390x_translate_init(void)
     int i;
 
     cpu_env = tcg_global_reg_new_ptr(TCG_AREG0, "env");
+    tcg_ctx.tcg_env = cpu_env;
     psw_addr = tcg_global_mem_new_i64(cpu_env,
                                       offsetof(CPUS390XState, psw.addr),
                                       "psw_addr");
diff --git a/target-sh4/translate.c b/target-sh4/translate.c
index e35d175..2590760 100644
--- a/target-sh4/translate.c
+++ b/target-sh4/translate.c
@@ -100,6 +100,7 @@ void sh4_translate_init(void)
         return;
 
     cpu_env = tcg_global_reg_new_ptr(TCG_AREG0, "env");
+    tcg_ctx.tcg_env = cpu_env;
 
     for (i = 0; i < 24; i++)
         cpu_gregs[i] = tcg_global_mem_new_i32(cpu_env,
diff --git a/target-sparc/translate.c b/target-sparc/translate.c
index 536c4b5..5e893d6 100644
--- a/target-sparc/translate.c
+++ b/target-sparc/translate.c
@@ -5353,6 +5353,7 @@ void gen_intermediate_code_init(CPUSPARCState *env)
         inited = 1;
 
         cpu_env = tcg_global_reg_new_ptr(TCG_AREG0, "env");
+        tcg_ctx.tcg_env = cpu_env;
         cpu_regwptr = tcg_global_mem_new_ptr(cpu_env,
                                              offsetof(CPUSPARCState, regwptr),
                                              "regwptr");
diff --git a/target-tilegx/translate.c b/target-tilegx/translate.c
index 7073aba..b3de5ae 100644
--- a/target-tilegx/translate.c
+++ b/target-tilegx/translate.c
@@ -2442,6 +2442,7 @@ void tilegx_tcg_init(void)
     int i;
 
     cpu_env = tcg_global_reg_new_ptr(TCG_AREG0, "env");
+    tcg_ctx.tcg_env = cpu_env;
     cpu_pc = tcg_global_mem_new_i64(cpu_env, offsetof(CPUTLGState, pc), "pc");
     for (i = 0; i < TILEGX_R_COUNT; i++) {
         cpu_regs[i] = tcg_global_mem_new_i64(cpu_env,
diff --git a/target-tricore/translate.c b/target-tricore/translate.c
index a70fdf7..49ac4a6 100644
--- a/target-tricore/translate.c
+++ b/target-tricore/translate.c
@@ -8368,6 +8368,7 @@ void tricore_tcg_init(void)
         return;
     }
     cpu_env = tcg_global_reg_new_ptr(TCG_AREG0, "env");
+    tcg_ctx.tcg_env = cpu_env;
     /* reg init */
     for (i = 0 ; i < 16 ; i++) {
         cpu_gpr_a[i] = tcg_global_mem_new(cpu_env,
diff --git a/target-unicore32/translate.c b/target-unicore32/translate.c
index 1dd086d..e9efe32 100644
--- a/target-unicore32/translate.c
+++ b/target-unicore32/translate.c
@@ -69,6 +69,7 @@ void uc32_translate_init(void)
     int i;
 
     cpu_env = tcg_global_reg_new_ptr(TCG_AREG0, "env");
+    tcg_ctx.tcg_env = cpu_env;
 
     for (i = 0; i < 32; i++) {
         cpu_R[i] = tcg_global_mem_new_i32(cpu_env,
diff --git a/target-xtensa/translate.c b/target-xtensa/translate.c
index fd03603..4efa1af 100644
--- a/target-xtensa/translate.c
+++ b/target-xtensa/translate.c
@@ -218,6 +218,7 @@ void xtensa_translate_init(void)
     int i;
 
     cpu_env = tcg_global_reg_new_ptr(TCG_AREG0, "env");
+    tcg_ctx.tcg_env = cpu_env;
     cpu_pc = tcg_global_mem_new_i32(cpu_env,
             offsetof(CPUXtensaState, pc), "pc");
 
diff --git a/tcg/tcg.h b/tcg/tcg.h
index 83da5fb..73d9069 100644
--- a/tcg/tcg.h
+++ b/tcg/tcg.h
@@ -566,6 +566,10 @@ struct TCGContext {
 
     TBContext tb_ctx;
 
+    /* Track which vCPU triggers events */
+    CPUState *cpu;                      /* *_trans */
+    TCGv_env tcg_env;                   /* *_exec  */
+
     /* The TCGBackendData structure is private to tcg-target.c.  */
     struct TCGBackendData *be;
 
diff --git a/translate-all.c b/translate-all.c
index e9f409b..c3de346 100644
--- a/translate-all.c
+++ b/translate-all.c
@@ -1091,6 +1091,8 @@ TranslationBlock *tb_gen_code(CPUState *cpu,
     ti = profile_getclock();
 #endif
 
+    tcg_ctx.cpu = ENV_GET_CPU(env);
+
     tcg_func_start(&tcg_ctx);
 
     gen_intermediate_code(env, tb);

Re: [Qemu-devel] [PATCH 1/5] exec: [tcg] Track which vCPU is performing translation and execution

[Peter Maydell]

On 23 February 2016 at 18:22, Lluís Vilanova <vilanova@ac.upc.edu> wrote:
> Information is tracked inside the TCGContext structure, and later used
> by tracing events with the 'tcg' and 'vcpu' properties.
>
> The 'cpu' field is used to check tracing of translation-time
> events ("*_trans"). The 'tcg_env' field is used to pass it to
> execution-time events ("*_exec").
>
> Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[Qemu-devel] [PATCH 2/5] trace: [all] Add "guest_vmem" event

[Lluís Vilanova]

Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
---
 include/exec/cpu_ldst_template.h          |   17 +++++++++++++++
 include/exec/cpu_ldst_useronly_template.h |   14 ++++++++++++
 tcg/tcg-op.c                              |   34 ++++++++++++++++++++++++++---
 trace-events                              |   13 +++++++++++
 4 files changed, 74 insertions(+), 4 deletions(-)

diff --git a/include/exec/cpu_ldst_template.h b/include/exec/cpu_ldst_template.h
index 3091c00..516f378 100644
--- a/include/exec/cpu_ldst_template.h
+++ b/include/exec/cpu_ldst_template.h
@@ -23,6 +23,11 @@
  * You should have received a copy of the GNU Lesser General Public
  * License along with this library; if not, see <http://www.gnu.org/licenses/>.
  */
+
+#if !defined(SOFTMMU_CODE_ACCESS)
+#include "trace.h"
+#endif
+
 #if DATA_SIZE == 8
 #define SUFFIX q
 #define USUFFIX q
@@ -80,6 +85,10 @@ glue(glue(glue(cpu_ld, USUFFIX), MEMSUFFIX), _ra)(CPUArchState *env,
     int mmu_idx;
     TCGMemOpIdx oi;
 
+#if !defined(SOFTMMU_CODE_ACCESS)
+    trace_guest_vmem_exec(ENV_GET_CPU(env), ptr, DATA_SIZE, 0);
+#endif
+
     addr = ptr;
     page_index = (addr >> TARGET_PAGE_BITS) & (CPU_TLB_SIZE - 1);
     mmu_idx = CPU_MMU_INDEX;
@@ -112,6 +121,10 @@ glue(glue(glue(cpu_lds, SUFFIX), MEMSUFFIX), _ra)(CPUArchState *env,
     int mmu_idx;
     TCGMemOpIdx oi;
 
+#if !defined(SOFTMMU_CODE_ACCESS)
+    trace_guest_vmem_exec(ENV_GET_CPU(env), ptr, DATA_SIZE, 0);
+#endif
+
     addr = ptr;
     page_index = (addr >> TARGET_PAGE_BITS) & (CPU_TLB_SIZE - 1);
     mmu_idx = CPU_MMU_INDEX;
@@ -148,6 +161,10 @@ glue(glue(glue(cpu_st, SUFFIX), MEMSUFFIX), _ra)(CPUArchState *env,
     int mmu_idx;
     TCGMemOpIdx oi;
 
+#if !defined(SOFTMMU_CODE_ACCESS)
+    trace_guest_vmem_exec(ENV_GET_CPU(env), ptr, DATA_SIZE, 1);
+#endif
+
     addr = ptr;
     page_index = (addr >> TARGET_PAGE_BITS) & (CPU_TLB_SIZE - 1);
     mmu_idx = CPU_MMU_INDEX;
diff --git a/include/exec/cpu_ldst_useronly_template.h b/include/exec/cpu_ldst_useronly_template.h
index 040b147..cde3d00 100644
--- a/include/exec/cpu_ldst_useronly_template.h
+++ b/include/exec/cpu_ldst_useronly_template.h
@@ -22,6 +22,11 @@
  * You should have received a copy of the GNU Lesser General Public
  * License along with this library; if not, see <http://www.gnu.org/licenses/>.
  */
+
+#if !defined(CODE_ACCESS)
+#include "trace.h"
+#endif
+
 #if DATA_SIZE == 8
 #define SUFFIX q
 #define USUFFIX q
@@ -53,6 +58,9 @@
 static inline RES_TYPE
 glue(glue(cpu_ld, USUFFIX), MEMSUFFIX)(CPUArchState *env, target_ulong ptr)
 {
+#if !defined(CODE_ACCESS)
+    trace_guest_vmem_exec(ENV_GET_CPU(env), ptr, DATA_SIZE, 0);
+#endif
     return glue(glue(ld, USUFFIX), _p)(g2h(ptr));
 }
 
@@ -68,6 +76,9 @@ glue(glue(glue(cpu_ld, USUFFIX), MEMSUFFIX), _ra)(CPUArchState *env,
 static inline int
 glue(glue(cpu_lds, SUFFIX), MEMSUFFIX)(CPUArchState *env, target_ulong ptr)
 {
+#if !defined(CODE_ACCESS)
+    trace_guest_vmem_exec(ENV_GET_CPU(env), ptr, DATA_SIZE, 0);
+#endif
     return glue(glue(lds, SUFFIX), _p)(g2h(ptr));
 }
 
@@ -85,6 +96,9 @@ static inline void
 glue(glue(cpu_st, SUFFIX), MEMSUFFIX)(CPUArchState *env, target_ulong ptr,
                                       RES_TYPE v)
 {
+#if !defined(CODE_ACCESS)
+    trace_guest_vmem_exec(ENV_GET_CPU(env), ptr, DATA_SIZE, 1);
+#endif
     glue(glue(st, SUFFIX), _p)(g2h(ptr), v);
 }
 
diff --git a/tcg/tcg-op.c b/tcg/tcg-op.c
index f554b86..789e427 100644
--- a/tcg/tcg-op.c
+++ b/tcg/tcg-op.c
@@ -25,6 +25,7 @@
 #include "qemu/osdep.h"
 #include "tcg.h"
 #include "tcg-op.h"
+#include "trace-tcg.h"
 
 /* Reduce the number of ifdefs below.  This assumes that all uses of
    TCGV_HIGH and TCGV_LOW are properly protected by a conditional that
@@ -1904,22 +1905,44 @@ static void gen_ldst_i64(TCGOpcode opc, TCGv_i64 val, TCGv addr,
 #endif
 }
 
-void tcg_gen_qemu_ld_i32(TCGv_i32 val, TCGv addr, TCGArg idx, TCGMemOp memop)
+static inline uint8_t tcg_memop_size(TCGMemOp op)
+{
+    return 1 << (op & MO_SIZE);
+}
+
+static inline void do_tcg_gen_qemu_ld_i32(TCGv_i32 val, TCGv addr, TCGArg idx, TCGMemOp memop)
 {
     memop = tcg_canonicalize_memop(memop, 0, 0);
     gen_ldst_i32(INDEX_op_qemu_ld_i32, val, addr, memop, idx);
 }
 
-void tcg_gen_qemu_st_i32(TCGv_i32 val, TCGv addr, TCGArg idx, TCGMemOp memop)
+void tcg_gen_qemu_ld_i32(TCGv_i32 val, TCGv addr, TCGArg idx, TCGMemOp memop)
+{
+    trace_guest_vmem_tcg(tcg_ctx.cpu, tcg_ctx.tcg_env,
+                         addr, tcg_memop_size(memop), 0);
+    do_tcg_gen_qemu_ld_i32(val, addr, idx, memop);
+}
+
+static inline void do_tcg_gen_qemu_st_i32(TCGv_i32 val, TCGv addr, TCGArg idx, TCGMemOp memop)
 {
     memop = tcg_canonicalize_memop(memop, 0, 1);
     gen_ldst_i32(INDEX_op_qemu_st_i32, val, addr, memop, idx);
 }
 
+void tcg_gen_qemu_st_i32(TCGv_i32 val, TCGv addr, TCGArg idx, TCGMemOp memop)
+{
+    trace_guest_vmem_tcg(tcg_ctx.cpu, tcg_ctx.tcg_env,
+                         addr, tcg_memop_size(memop), 1);
+    do_tcg_gen_qemu_st_i32(val, addr, idx, memop);
+}
+
 void tcg_gen_qemu_ld_i64(TCGv_i64 val, TCGv addr, TCGArg idx, TCGMemOp memop)
 {
+    trace_guest_vmem_tcg(tcg_ctx.cpu, tcg_ctx.tcg_env,
+                         addr, tcg_memop_size(memop), 0);
+
     if (TCG_TARGET_REG_BITS == 32 && (memop & MO_SIZE) < MO_64) {
-        tcg_gen_qemu_ld_i32(TCGV_LOW(val), addr, idx, memop);
+        do_tcg_gen_qemu_ld_i32(TCGV_LOW(val), addr, idx, memop);
         if (memop & MO_SIGN) {
             tcg_gen_sari_i32(TCGV_HIGH(val), TCGV_LOW(val), 31);
         } else {
@@ -1934,8 +1957,11 @@ void tcg_gen_qemu_ld_i64(TCGv_i64 val, TCGv addr, TCGArg idx, TCGMemOp memop)
 
 void tcg_gen_qemu_st_i64(TCGv_i64 val, TCGv addr, TCGArg idx, TCGMemOp memop)
 {
+    trace_guest_vmem_tcg(tcg_ctx.cpu, tcg_ctx.tcg_env,
+                         addr, tcg_memop_size(memop), 0);
+
     if (TCG_TARGET_REG_BITS == 32 && (memop & MO_SIZE) < MO_64) {
-        tcg_gen_qemu_st_i32(TCGV_LOW(val), addr, idx, memop);
+        do_tcg_gen_qemu_st_i32(TCGV_LOW(val), addr, idx, memop);
         return;
     }
 
diff --git a/trace-events b/trace-events
index f986c81..1088fe0 100644
--- a/trace-events
+++ b/trace-events
@@ -1890,3 +1890,16 @@ qio_channel_command_new_pid(void *ioc, int writefd, int readfd, int pid) "Comman
 qio_channel_command_new_spawn(void *ioc, const char *binary, int flags) "Command new spawn ioc=%p binary=%s flags=%d"
 qio_channel_command_abort(void *ioc, int pid) "Command abort ioc=%p pid=%d"
 qio_channel_command_wait(void *ioc, int pid, int ret, int status) "Command abort ioc=%p pid=%d ret=%d status=%d"
+
+### Guest events, keep at bottom
+
+# @vaddr: Access' virtual address.
+# @size : Access' size (bytes).
+# @store: Whether the access is a store.
+#
+# Start virtual memory access (before any potential access violation).
+#
+# Does not include memory accesses performed by devices.
+#
+# Targets: TCG(all)
+disable vcpu tcg guest_vmem(TCGv vaddr, uint8_t size, uint8_t store) "size=%d store=%d", "vaddr=0x%016"PRIx64" size=%d store=%d"

Re: [Qemu-devel] [PATCH 2/5] trace: [all] Add "guest_vmem" event

[Peter Maydell]

On 23 February 2016 at 18:22, Lluís Vilanova <vilanova@ac.upc.edu> wrote:
> Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>

> +### Guest events, keep at bottom
> +
> +# @vaddr: Access' virtual address.
> +# @size : Access' size (bytes).
> +# @store: Whether the access is a store.
> +#
> +# Start virtual memory access (before any potential access violation).
> +#
> +# Does not include memory accesses performed by devices.
> +#
> +# Targets: TCG(all)
> +disable vcpu tcg guest_vmem(TCGv vaddr, uint8_t size, uint8_t store) "size=%d store=%d", "vaddr=0x%016"PRIx64" size=%d store=%d"

Shouldn't we also be tracing some of the other information in the memop?
In particular, endianness of the access seems useful. You could also
say whether we're zero- or sign-extending the access, though I guess
you could defend not printing that since we don't print anything about
what the target- code does with the loaded data.

Otherwise I think this looks OK (though the various paths memory accesses
take are a bit opaque since I've forgotten how this bit of QEMU works).

thanks
-- PMM

Re: [Qemu-devel] [PATCH 2/5] trace: [all] Add "guest_vmem" event

[Lluís Vilanova]

Peter Maydell writes:

> On 23 February 2016 at 18:22, Lluís Vilanova <vilanova@ac.upc.edu> wrote:
>> Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>

>> +### Guest events, keep at bottom
>> +
>> +# @vaddr: Access' virtual address.
>> +# @size : Access' size (bytes).
>> +# @store: Whether the access is a store.
>> +#
>> +# Start virtual memory access (before any potential access violation).
>> +#
>> +# Does not include memory accesses performed by devices.
>> +#
>> +# Targets: TCG(all)
>> +disable vcpu tcg guest_vmem(TCGv vaddr, uint8_t size, uint8_t store) "size=%d store=%d", "vaddr=0x%016"PRIx64" size=%d store=%d"

> Shouldn't we also be tracing some of the other information in the memop?
> In particular, endianness of the access seems useful. You could also
> say whether we're zero- or sign-extending the access, though I guess
> you could defend not printing that since we don't print anything about
> what the target- code does with the loaded data.

> Otherwise I think this looks OK (though the various paths memory accesses
> take are a bit opaque since I've forgotten how this bit of QEMU works).

Mmmmm, the endianness seems more of a vCPU property than one of the memory
access. A separate event could be added for that (e.g., at vCPU
initalization/hot-plug and whenever it is dynamically changed like in ARM).

For the sign extension and memory value, what about adding multiple events?
What I did for instructions is have a simple event and one with extended
information, so that we can tweak performance of a tracing QEMU by choosing one
or the other.  We could do the same for memory accesses (e.g., also show the
memory value, sign extension and physical address).

Re: [Qemu-devel] [PATCH 2/5] trace: [all] Add "guest_vmem" event

[Richard Henderson]

On 03/17/2016 12:22 PM, Lluís Vilanova wrote:
> Mmmmm, the endianness seems more of a vCPU property than one of the memory
> access.

On the contrary.  Plenty of cpus have endian-swapping load/store insns: x86
(haswell's movbe), powerpc, sparcv9, s390.  Maybe others I've forgotten.


r~

Re: [Qemu-devel] [PATCH 2/5] trace: [all] Add "guest_vmem" event

[Peter Maydell]

On 17 March 2016 at 19:22, Lluís Vilanova <vilanova@ac.upc.edu> wrote:
> Mmmmm, the endianness seems more of a vCPU property than one of the memory
> access. A separate event could be added for that (e.g., at vCPU
> initalization/hot-plug and whenever it is dynamically changed like in ARM).

We've chosen to implement it in QEMU as a property of the memory
access. Consider for instance instructions like x86 movbe --
sometimes an individual instruction will be a BE access even if the
CPU more usually does LE accesses. Equally, CPUs might have different
accesses for data and code, or other complicated things.
I think we're probably better off tracing as part of the memory
access the things we've implemented as memory access properties
or flags; at least that's consistent.

> For the sign extension and memory value, what about adding multiple events?
> What I did for instructions is have a simple event and one with extended
> information, so that we can tweak performance of a tracing QEMU by choosing one
> or the other.  We could do the same for memory accesses (e.g., also show the
> memory value, sign extension and physical address).

I think the info that's in the memop value is probably worth
just printing always, it won't make much difference.

Trying to trace physaddrs is very tricky -- in the case of
a TLB hit there is no guarantee you can still identify the
physaddr of what you're accessing (the guest might have
changed the page tables and not invalidated the TLB).

thanks
-- PMM

Re: [Qemu-devel] [PATCH 2/5] trace: [all] Add "guest_vmem" event

[Lluís Vilanova]

Peter Maydell writes:

> On 17 March 2016 at 19:22, Lluís Vilanova <vilanova@ac.upc.edu> wrote:
>> Mmmmm, the endianness seems more of a vCPU property than one of the memory
>> access. A separate event could be added for that (e.g., at vCPU
>> initalization/hot-plug and whenever it is dynamically changed like in ARM).

> We've chosen to implement it in QEMU as a property of the memory
> access. Consider for instance instructions like x86 movbe --
> sometimes an individual instruction will be a BE access even if the
> CPU more usually does LE accesses. Equally, CPUs might have different
> accesses for data and code, or other complicated things.
> I think we're probably better off tracing as part of the memory
> access the things we've implemented as memory access properties
> or flags; at least that's consistent.

Oh, I wasn't aware of the endinness swapping, but your argument makes sense.


>> For the sign extension and memory value, what about adding multiple events?
>> What I did for instructions is have a simple event and one with extended
>> information, so that we can tweak performance of a tracing QEMU by choosing one
>> or the other.  We could do the same for memory accesses (e.g., also show the
>> memory value, sign extension and physical address).

> I think the info that's in the memop value is probably worth
> just printing always, it won't make much difference.

But is the endianness, sign extension, etc. valuable information when the data
is not part of the trace?

If it is, I can easily add that; but, there is still some additional
per-argument overhead when generating a TCG call to the exec-time tracing
routine, thus my target on adding two trace events with different levels of
detail. I haven't measured the impact, though.


> Trying to trace physaddrs is very tricky -- in the case of
> a TLB hit there is no guarantee you can still identify the
> physaddr of what you're accessing (the guest might have
> changed the page tables and not invalidated the TLB).

I was looking at how to modify the soft TLB code to generate that information
for the trace event, but it requires changes at every TCG target. But in any
case, that should always provide the same phys address used by QEMU, so the
event info is "correct" in that sense. Or did I miss something?


Cheers,
  Lluis

Re: [Qemu-devel] [PATCH 2/5] trace: [all] Add "guest_vmem" event

[Peter Maydell]

On 18 March 2016 at 18:50, Lluís Vilanova <vilanova@ac.upc.edu> wrote:
> Peter Maydell writes:
>> Trying to trace physaddrs is very tricky -- in the case of
>> a TLB hit there is no guarantee you can still identify the
>> physaddr of what you're accessing (the guest might have
>> changed the page tables and not invalidated the TLB).
>
> I was looking at how to modify the soft TLB code to generate that information
> for the trace event, but it requires changes at every TCG target. But in any
> case, that should always provide the same phys address used by QEMU, so the
> event info is "correct" in that sense. Or did I miss something?

Consider the sequence:
 * guest makes access to vaddr V, currently mapped to physaddr P1
   (which is host address H)
 * we put the mapping V -> H into QEMU's TLB
 * guest changes its page tables so V now maps to P2,
   but doesn't do a TLB flush
 * guest makes another access to vaddr V
 * we hit in QEMU's TLB, so we know V and H; but we don't
   know P1 (because we don't record that in the TLB) and we
   can't even get it by walking the page table because
   at this point V maps to P2, not P1. (And for sw-TLB
   guest archs like MIPS you can't even do a V-to-P lookup
   in QEMU non-intrusively.)

(This is often defined to be unpredictable or similar in the guest
architecture. But a buggy guest might do it, and tracing the
wrong thing would be pretty confusing if you were trying to
track down that bug.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH 2/5] trace: [all] Add "guest_vmem" event

[Lluís Vilanova]

Peter Maydell writes:

> On 18 March 2016 at 18:50, Lluís Vilanova <vilanova@ac.upc.edu> wrote:
>> Peter Maydell writes:
>>> Trying to trace physaddrs is very tricky -- in the case of
>>> a TLB hit there is no guarantee you can still identify the
>>> physaddr of what you're accessing (the guest might have
>>> changed the page tables and not invalidated the TLB).
>> 
>> I was looking at how to modify the soft TLB code to generate that information
>> for the trace event, but it requires changes at every TCG target. But in any
>> case, that should always provide the same phys address used by QEMU, so the
>> event info is "correct" in that sense. Or did I miss something?

> Consider the sequence:
>  * guest makes access to vaddr V, currently mapped to physaddr P1
>    (which is host address H)
>  * we put the mapping V -> H into QEMU's TLB
>  * guest changes its page tables so V now maps to P2,
>    but doesn't do a TLB flush
>  * guest makes another access to vaddr V
>  * we hit in QEMU's TLB, so we know V and H; but we don't
>    know P1 (because we don't record that in the TLB) and we
>    can't even get it by walking the page table because
>    at this point V maps to P2, not P1. (And for sw-TLB
>    guest archs like MIPS you can't even do a V-to-P lookup
>    in QEMU non-intrusively.)

> (This is often defined to be unpredictable or similar in the guest
> architecture. But a buggy guest might do it, and tracing the
> wrong thing would be pretty confusing if you were trying to
> track down that bug.)

Oh! Yes, I seem to remember that code path now, I checked it a really long time
ago. I was assuming that whenever this event is enabled at compile time, I would
have to modify QEMU's TLB to store the guest physical address (then used by the
tracing event).


Cheers,
  Lluis

Re: [Qemu-devel] [PATCH 2/5] trace: [all] Add "guest_vmem" event

[Peter Maydell]

On 20 March 2016 at 18:09, Lluís Vilanova <vilanova@ac.upc.edu> wrote:
> Oh! Yes, I seem to remember that code path now, I checked it a
> really long time ago. I was assuming that whenever this event is
> enabled at compile time, I would have to modify QEMU's TLB to store
> the guest physical address (then used by the tracing event).

I guess we could maybe put that into the iotlb. You definitely
don't want it in the CPUTLBEntry struct as that one is space
critical for performance. (If you're really lucky you can
reconstruct the physaddr from the iotlb addr field but I suspect
you can't.)

Once you've decided to take the hit of keeping the paddr in the
TLB it's probably faster to just unconditionally store it rather
than doing a "store if trace event X enabled" test.

PS: you probably also want to be able to trace the MemTxAttrs
(which tells you whether you have an S or NS access on ARM,
among other things).

thanks
-- PMM

Re: [Qemu-devel] [PATCH 2/5] trace: [all] Add "guest_vmem" event

[Lluís Vilanova]

Peter Maydell writes:

> On 20 March 2016 at 18:09, Lluís Vilanova <vilanova@ac.upc.edu> wrote:
>> Oh! Yes, I seem to remember that code path now, I checked it a
>> really long time ago. I was assuming that whenever this event is
>> enabled at compile time, I would have to modify QEMU's TLB to store
>> the guest physical address (then used by the tracing event).

> I guess we could maybe put that into the iotlb. You definitely
> don't want it in the CPUTLBEntry struct as that one is space
> critical for performance. (If you're really lucky you can
> reconstruct the physaddr from the iotlb addr field but I suspect
> you can't.)

> Once you've decided to take the hit of keeping the paddr in the
> TLB it's probably faster to just unconditionally store it rather
> than doing a "store if trace event X enabled" test.

I meant to make the check at compile time, since we have defines to check which
events are enabled/disabled in trace-events.


> PS: you probably also want to be able to trace the MemTxAttrs
> (which tells you whether you have an S or NS access on ARM,
> among other things).

I'll keep these in mind for a separate series with extended memory info.


Thanks,
  Lluis

[Qemu-devel] [PATCH 3/5] user: Refactor lock_user body into do_lock_user

[Lluís Vilanova]

Lock user will later be hooked to raise a tracing event, while the
internal 'do_lock_user' will be used by non-interface code (e.g., string
length computation).

Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
---
 bsd-user/qemu.h      |   11 ++++++++---
 bsd-user/uaccess.c   |    2 +-
 linux-user/qemu.h    |   11 ++++++++---
 linux-user/uaccess.c |    2 +-
 4 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/bsd-user/qemu.h b/bsd-user/qemu.h
index 1b5f998..4dad254 100644
--- a/bsd-user/qemu.h
+++ b/bsd-user/qemu.h
@@ -348,9 +348,7 @@ abi_long copy_to_user(abi_ulong gaddr, void *hptr, size_t len);
    any byteswapping.  lock_user may return either a pointer to the guest
    memory, or a temporary buffer.  */
 
-/* Lock an area of guest memory into the host.  If copy is true then the
-   host area will have the same contents as the guest.  */
-static inline void *lock_user(int type, abi_ulong guest_addr, long len, int copy)
+static inline void *do_lock_user(int type, abi_ulong guest_addr, long len, int copy)
 {
     if (!access_ok(type, guest_addr, len))
         return NULL;
@@ -369,6 +367,13 @@ static inline void *lock_user(int type, abi_ulong guest_addr, long len, int copy
 #endif
 }
 
+/* Lock an area of guest memory into the host.  If copy is true then the
+   host area will have the same contents as the guest.  */
+static inline void *lock_user(int type, abi_ulong guest_addr, long len, int copy)
+{
+    return do_lock_user(type, guest_addr, len, copy);
+}
+
 /* Unlock an area of guest memory.  The first LEN bytes must be
    flushed back to guest memory. host_ptr = NULL is explicitly
    allowed and does nothing. */
diff --git a/bsd-user/uaccess.c b/bsd-user/uaccess.c
index 7cb6d17..69f00db 100644
--- a/bsd-user/uaccess.c
+++ b/bsd-user/uaccess.c
@@ -47,7 +47,7 @@ abi_long target_strlen(abi_ulong guest_addr1)
     guest_addr = guest_addr1;
     for(;;) {
         max_len = TARGET_PAGE_SIZE - (guest_addr & ~TARGET_PAGE_MASK);
-        ptr = lock_user(VERIFY_READ, guest_addr, max_len, 1);
+        ptr = do_lock_user(VERIFY_READ, guest_addr, max_len, 1);
         if (!ptr)
             return -TARGET_EFAULT;
         len = qemu_strnlen((char *)ptr, max_len);
diff --git a/linux-user/qemu.h b/linux-user/qemu.h
index ba5b433..0b71683 100644
--- a/linux-user/qemu.h
+++ b/linux-user/qemu.h
@@ -372,9 +372,7 @@ abi_long copy_to_user(abi_ulong gaddr, void *hptr, size_t len);
    any byteswapping.  lock_user may return either a pointer to the guest
    memory, or a temporary buffer.  */
 
-/* Lock an area of guest memory into the host.  If copy is true then the
-   host area will have the same contents as the guest.  */
-static inline void *lock_user(int type, abi_ulong guest_addr, long len, int copy)
+static inline void *do_lock_user(int type, abi_ulong guest_addr, long len, int copy)
 {
     if (!access_ok(type, guest_addr, len))
         return NULL;
@@ -393,6 +391,13 @@ static inline void *lock_user(int type, abi_ulong guest_addr, long len, int copy
 #endif
 }
 
+/* Lock an area of guest memory into the host.  If copy is true then the
+   host area will have the same contents as the guest.  */
+static inline void *lock_user(int type, abi_ulong guest_addr, long len, int copy)
+{
+    return do_lock_user(type, guest_addr, len, copy);
+}
+
 /* Unlock an area of guest memory.  The first LEN bytes must be
    flushed back to guest memory. host_ptr = NULL is explicitly
    allowed and does nothing. */
diff --git a/linux-user/uaccess.c b/linux-user/uaccess.c
index 75d890d..fac2464 100644
--- a/linux-user/uaccess.c
+++ b/linux-user/uaccess.c
@@ -47,7 +47,7 @@ abi_long target_strlen(abi_ulong guest_addr1)
     guest_addr = guest_addr1;
     for(;;) {
         max_len = TARGET_PAGE_SIZE - (guest_addr & ~TARGET_PAGE_MASK);
-        ptr = lock_user(VERIFY_READ, guest_addr, max_len, 1);
+        ptr = do_lock_user(VERIFY_READ, guest_addr, max_len, 1);
         if (!ptr)
             return -TARGET_EFAULT;
         len = qemu_strnlen((const char *)ptr, max_len);

[Qemu-devel] [PATCH 4/5] user: Set current vCPU during syscall execution

[Lluís Vilanova]

Will be later used by tracing events.

Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
---
 bsd-user/syscall.c   |    2 ++
 linux-user/syscall.c |    2 ++
 2 files changed, 4 insertions(+)

diff --git a/bsd-user/syscall.c b/bsd-user/syscall.c
index 35f784c..ce941b0 100644
--- a/bsd-user/syscall.c
+++ b/bsd-user/syscall.c
@@ -320,6 +320,7 @@ abi_long do_freebsd_syscall(void *cpu_env, int num, abi_long arg1,
 #ifdef DEBUG
     gemu_log("freebsd syscall %d\n", num);
 #endif
+    current_cpu = cpu;                  /* accessed by tracing events*/
     if(do_strace)
         print_freebsd_syscall(num, arg1, arg2, arg3, arg4, arg5, arg6);
 
@@ -399,6 +400,7 @@ abi_long do_freebsd_syscall(void *cpu_env, int num, abi_long arg1,
 #endif
     if (do_strace)
         print_freebsd_syscall_ret(num, ret);
+    current_cpu = NULL;
     return ret;
  efault:
     ret = -TARGET_EFAULT;
diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 54ce14a..52f9d85 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -5859,6 +5859,7 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
 #ifdef DEBUG
     gemu_log("syscall %d", num);
 #endif
+    current_cpu = cpu;                  /* accessed by tracing events*/
     if(do_strace)
         print_syscall(num, arg1, arg2, arg3, arg4, arg5, arg6);
 
@@ -10240,6 +10241,7 @@ fail:
 #endif
     if(do_strace)
         print_syscall_ret(num, ret);
+    current_cpu = NULL;
     return ret;
 efault:
     ret = -TARGET_EFAULT;

[Qemu-devel] [PATCH 5/5] trace: [all] Add "guest_vmem_user_syscall" event

[Lluís Vilanova]

Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
---
 bsd-user/qemu.h   |   10 ++++++++++
 linux-user/qemu.h |   10 ++++++++++
 trace-events      |   10 ++++++++++
 3 files changed, 30 insertions(+)

diff --git a/bsd-user/qemu.h b/bsd-user/qemu.h
index 4dad254..090c09b 100644
--- a/bsd-user/qemu.h
+++ b/bsd-user/qemu.h
@@ -371,6 +371,16 @@ static inline void *do_lock_user(int type, abi_ulong guest_addr, long len, int c
    host area will have the same contents as the guest.  */
 static inline void *lock_user(int type, abi_ulong guest_addr, long len, int copy)
 {
+    if (type == VERIFY_WRITE) {
+        if (copy) {
+            /* 'VERIFY_WRITE' implies read, but only with 'copy' */
+            trace_guest_vmem_user_syscall(current_cpu, guest_addr, len, false);
+        }
+        trace_guest_vmem_user_syscall(current_cpu, guest_addr, len, true);
+    } else {
+        /* should actually read only with 'copy', but a few places do without */
+        trace_guest_vmem_user_syscall(current_cpu, guest_addr, len, false);
+    }
     return do_lock_user(type, guest_addr, len, copy);
 }
 
diff --git a/linux-user/qemu.h b/linux-user/qemu.h
index 0b71683..3874cbd 100644
--- a/linux-user/qemu.h
+++ b/linux-user/qemu.h
@@ -395,6 +395,16 @@ static inline void *do_lock_user(int type, abi_ulong guest_addr, long len, int c
    host area will have the same contents as the guest.  */
 static inline void *lock_user(int type, abi_ulong guest_addr, long len, int copy)
 {
+    if (type == VERIFY_WRITE) {
+        if (copy) {
+            /* 'VERIFY_WRITE' implies read, but only with 'copy' */
+            trace_guest_vmem_user_syscall(current_cpu, guest_addr, len, false);
+        }
+        trace_guest_vmem_user_syscall(current_cpu, guest_addr, len, true);
+    } else {
+        /* should actually read only with 'copy', but a few places do without */
+        trace_guest_vmem_user_syscall(current_cpu, guest_addr, len, false);
+    }
     return do_lock_user(type, guest_addr, len, copy);
 }
 
diff --git a/trace-events b/trace-events
index 1088fe0..940b0ba 100644
--- a/trace-events
+++ b/trace-events
@@ -1903,3 +1903,13 @@ qio_channel_command_wait(void *ioc, int pid, int ret, int status) "Command abort
 #
 # Targets: TCG(all)
 disable vcpu tcg guest_vmem(TCGv vaddr, uint8_t size, uint8_t store) "size=%d store=%d", "vaddr=0x%016"PRIx64" size=%d store=%d"
+
+# @vaddr: Access' virtual address.
+# @size : Access' size (bytes).
+# @store: Whether the access is a store.
+#
+# Similar to 'guest_vmem' event, but raised inside syscall emulation code when
+# running in user-mode.
+#
+# Targets: TCG(all)
+disable vcpu guest_vmem_user_syscall(uint64_t vaddr, uint64_t size, uint8_t store) "vaddr=0x%016"PRIx64" size=%"PRIu64" store=%d"

Re: [Qemu-devel] [PATCH 0/5] trace: Add events for vCPU memory accesses

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 2774 bytes --]

On Tue, Feb 23, 2016 at 07:22:07PM +0100, Lluís Vilanova wrote:
> NOTE: This series applies on top of "trace: Show vCPU info in guest code events"
> 
> This series adds to new events:
> 
> * guest_vmem: memory accesses performed by vCPUs (guest code)
> 
> * guest_vmem_user_syscall: memory accesses performed by syscall emulation when
>   running QEMU in user-mode.
> 
> Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
> ---
> 
> Lluís Vilanova (5):
>       exec: [tcg] Track which vCPU is performing translation and execution
>       trace: [all] Add "guest_vmem" event
>       user: Refactor lock_user body into do_lock_user
>       user: Set current vCPU during syscall execution
>       trace: [all] Add "guest_vmem_user_syscall" event
> 
> 
>  bsd-user/qemu.h                           |   21 +++++++++++++++---
>  bsd-user/syscall.c                        |    2 ++
>  bsd-user/uaccess.c                        |    2 +-
>  include/exec/cpu_ldst_template.h          |   17 +++++++++++++++
>  include/exec/cpu_ldst_useronly_template.h |   14 ++++++++++++
>  linux-user/qemu.h                         |   21 +++++++++++++++---
>  linux-user/syscall.c                      |    2 ++
>  linux-user/uaccess.c                      |    2 +-
>  target-alpha/translate.c                  |    1 +
>  target-arm/translate.c                    |    1 +
>  target-cris/translate.c                   |    1 +
>  target-cris/translate_v10.c               |    1 +
>  target-i386/translate.c                   |    1 +
>  target-lm32/translate.c                   |    1 +
>  target-m68k/translate.c                   |    1 +
>  target-microblaze/translate.c             |    1 +
>  target-mips/translate.c                   |    1 +
>  target-moxie/translate.c                  |    1 +
>  target-openrisc/translate.c               |    1 +
>  target-ppc/translate.c                    |    1 +
>  target-s390x/translate.c                  |    1 +
>  target-sh4/translate.c                    |    1 +
>  target-sparc/translate.c                  |    1 +
>  target-tilegx/translate.c                 |    1 +
>  target-tricore/translate.c                |    1 +
>  target-unicore32/translate.c              |    1 +
>  target-xtensa/translate.c                 |    1 +
>  tcg/tcg-op.c                              |   34 ++++++++++++++++++++++++++---
>  tcg/tcg.h                                 |    4 +++
>  trace-events                              |   23 ++++++++++++++++++++
>  translate-all.c                           |    2 ++
>  31 files changed, 151 insertions(+), 12 deletions(-)
> 
> 
> To: qemu-devel@nongnu.org
> Cc: Stefan Hajnoczi <stefanha@redhat.com>
> 

Any comments from TCG folks?

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 0/5] trace: Add events for vCPU memory accesses

[Peter Maydell]

On 2 March 2016 at 10:55, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Tue, Feb 23, 2016 at 07:22:07PM +0100, Lluís Vilanova wrote:
>> NOTE: This series applies on top of "trace: Show vCPU info in guest code events"
>>
>> This series adds to new events:
>>
>> * guest_vmem: memory accesses performed by vCPUs (guest code)
>>
>> * guest_vmem_user_syscall: memory accesses performed by syscall emulation when
>>   running QEMU in user-mode.
>>
>> Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
>> ---
>>
>> Lluís Vilanova (5):
>>       exec: [tcg] Track which vCPU is performing translation and execution
>>       trace: [all] Add "guest_vmem" event
>>       user: Refactor lock_user body into do_lock_user
>>       user: Set current vCPU during syscall execution
>>       trace: [all] Add "guest_vmem_user_syscall" event

> Any comments from TCG folks?

The first two patches which add TCG guest data access tracing look
OK to me, but I'm much less sure about the last three which are
adding tracing into linux-user syscall emulation. I'm not sure
that lock_user is the right place to put that tracepoint.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 0/5] trace: Add events for vCPU memory accesses

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1291 bytes --]

On Wed, Mar 16, 2016 at 03:10:01PM +0000, Peter Maydell wrote:
> On 2 March 2016 at 10:55, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> > On Tue, Feb 23, 2016 at 07:22:07PM +0100, Lluís Vilanova wrote:
> >> NOTE: This series applies on top of "trace: Show vCPU info in guest code events"
> >>
> >> This series adds to new events:
> >>
> >> * guest_vmem: memory accesses performed by vCPUs (guest code)
> >>
> >> * guest_vmem_user_syscall: memory accesses performed by syscall emulation when
> >>   running QEMU in user-mode.
> >>
> >> Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
> >> ---
> >>
> >> Lluís Vilanova (5):
> >>       exec: [tcg] Track which vCPU is performing translation and execution
> >>       trace: [all] Add "guest_vmem" event
> >>       user: Refactor lock_user body into do_lock_user
> >>       user: Set current vCPU during syscall execution
> >>       trace: [all] Add "guest_vmem_user_syscall" event
> 
> > Any comments from TCG folks?
> 
> The first two patches which add TCG guest data access tracing look
> OK to me, but I'm much less sure about the last three which are
> adding tracing into linux-user syscall emulation. I'm not sure
> that lock_user is the right place to put that tracepoint.

Any thoughts on this, Lluís?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 0/5] trace: Add events for vCPU memory accesses

[Lluís Vilanova]

Stefan Hajnoczi writes:

> On Wed, Mar 16, 2016 at 03:10:01PM +0000, Peter Maydell wrote:
>> On 2 March 2016 at 10:55, Stefan Hajnoczi <stefanha@gmail.com> wrote:
>> > On Tue, Feb 23, 2016 at 07:22:07PM +0100, Lluís Vilanova wrote:
>> >> NOTE: This series applies on top of "trace: Show vCPU info in guest code events"
>> >>
>> >> This series adds to new events:
>> >>
>> >> * guest_vmem: memory accesses performed by vCPUs (guest code)
>> >>
>> >> * guest_vmem_user_syscall: memory accesses performed by syscall emulation when
>> >>   running QEMU in user-mode.
>> >>
>> >> Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
>> >> ---
>> >>
>> >> Lluís Vilanova (5):
>> >>       exec: [tcg] Track which vCPU is performing translation and execution
>> >>       trace: [all] Add "guest_vmem" event
>> >>       user: Refactor lock_user body into do_lock_user
>> >>       user: Set current vCPU during syscall execution
>> >>       trace: [all] Add "guest_vmem_user_syscall" event
>> 
>> > Any comments from TCG folks?
>> 
>> The first two patches which add TCG guest data access tracing look
>> OK to me, but I'm much less sure about the last three which are
>> adding tracing into linux-user syscall emulation. I'm not sure
>> that lock_user is the right place to put that tracepoint.

> Any thoughts on this, Lluís?

Mmmm, I was struggling to find a place to easily add the tracing events whenever
the syscall emulation code accesses guest memory.

The lock_user function is used precisely for that, but it can be a bit
heavy-handed as to what memory is actually read/written, since it only marks the
"potential" ability of doing so (it's a sort of acquire/release interface that
differentiates between read and write acquires).

So, while it's not the perfect place to do it, I think it's the best one we have
now. Having calls to both lock_user and trace_guest_vmem_user_syscall in syscall
emulation code should be avoided to keep the code cleaner.


Thanks,
  Lluis

Re: [Qemu-devel] [PATCH 0/5] trace: Add events for vCPU memory accesses

[Peter Maydell]

On 22 March 2016 at 14:02, Lluís Vilanova <vilanova@ac.upc.edu> wrote:
> Stefan Hajnoczi writes:
>
>> On Wed, Mar 16, 2016 at 03:10:01PM +0000, Peter Maydell wrote:
>>> The first two patches which add TCG guest data access tracing look
>>> OK to me, but I'm much less sure about the last three which are
>>> adding tracing into linux-user syscall emulation. I'm not sure
>>> that lock_user is the right place to put that tracepoint.
>
>> Any thoughts on this, Lluís?
>
> Mmmm, I was struggling to find a place to easily add the tracing events
> whenever the syscall emulation code accesses guest memory.
>
> The lock_user function is used precisely for that, but it can be a bit
> heavy-handed as to what memory is actually read/written, since it only marks the
> "potential" ability of doing so (it's a sort of acquire/release interface that
> differentiates between read and write acquires).

Well, that's why I'm not certain about it. I would prefer us to
not trace the accesses rather than put a trace in a wrong
position.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 0/5] trace: Add events for vCPU memory accesses

[Lluís Vilanova]

Peter Maydell writes:

> On 22 March 2016 at 14:02, Lluís Vilanova <vilanova@ac.upc.edu> wrote:
>> Stefan Hajnoczi writes:
>> 
>>> On Wed, Mar 16, 2016 at 03:10:01PM +0000, Peter Maydell wrote:
>>>> The first two patches which add TCG guest data access tracing look
>>>> OK to me, but I'm much less sure about the last three which are
>>>> adding tracing into linux-user syscall emulation. I'm not sure
>>>> that lock_user is the right place to put that tracepoint.
>> 
>>> Any thoughts on this, Lluís?
>> 
>> Mmmm, I was struggling to find a place to easily add the tracing events
>> whenever the syscall emulation code accesses guest memory.
>> 
>> The lock_user function is used precisely for that, but it can be a bit
>> heavy-handed as to what memory is actually read/written, since it only marks the
>> "potential" ability of doing so (it's a sort of acquire/release interface that
>> differentiates between read and write acquires).

> Well, that's why I'm not certain about it. I would prefer us to
> not trace the accesses rather than put a trace in a wrong
> position.

Theoretically, that's what DEBUG_REMAP is for. A temporary host buffer is
allocated and data is copied in/out of it according to 'type'
(VERIFY_READ/VERIFY_WRITE) and 'copy'; so if the lock_user/unlock_user calls are
incorrect, QEMU will not read/write the correct guest memory values for syscall
emulation.

I used the logic when DEBUG_REMAP is used to understand when guest memory is
declared to be read/written. So the current approach for tracing is never in a
wrong position.

The only problem I see is that the syscall emulation code can declare it wants
read & write access to some guest memory while a real system would only do one
of the two (or even none). Tracing this is not incorrect either, since it's just
how QEMU is emulating that syscall, but it could be sub-optimal compared to a
real syscall implementation.


Cheers,
  Lluis

Re: [Qemu-devel] [PATCH 0/5] trace: Add events for vCPU memory accesses

[Peter Maydell]

On 22 March 2016 at 19:23, Lluís Vilanova <vilanova@ac.upc.edu> wrote:
> Peter Maydell writes:
>> Well, that's why I'm not certain about it. I would prefer us to
>> not trace the accesses rather than put a trace in a wrong
>> position.
>
> Theoretically, that's what DEBUG_REMAP is for. A temporary host buffer is
> allocated and data is copied in/out of it according to 'type'
> (VERIFY_READ/VERIFY_WRITE) and 'copy'; so if the lock_user/unlock_user calls are
> incorrect, QEMU will not read/write the correct guest memory values for syscall
> emulation.
>
> I used the logic when DEBUG_REMAP is used to understand when guest memory is
> declared to be read/written. So the current approach for tracing is never in a
> wrong position.
>
> The only problem I see is that the syscall emulation code can declare it wants
> read & write access to some guest memory while a real system would only do one
> of the two (or even none). Tracing this is not incorrect either, since it's just
> how QEMU is emulating that syscall, but it could be sub-optimal compared to a
> real syscall implementation.

Also we sometimes lock an entire buffer but then don't read or write
all of it. And a few places call g2h() directly and accesses via
those code paths won't get traced.

But really, an access to implement a syscall isn't a guest memory
access, it's part of the implementation of the emulated kernel ABIs.
We have tracing for that, it's the strace code (though the strace
code is really very poor currently).

Tracing memory accesses made by the emulated CPU makes total sense;
but I think "what tracing should we do for linux-user code" is
a separate question.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 0/5] trace: Add events for vCPU memory accesses

[Lluís Vilanova]

Peter Maydell writes:

> On 22 March 2016 at 19:23, Lluís Vilanova <vilanova@ac.upc.edu> wrote:
>> Peter Maydell writes:
>>> Well, that's why I'm not certain about it. I would prefer us to
>>> not trace the accesses rather than put a trace in a wrong
>>> position.
>> 
>> Theoretically, that's what DEBUG_REMAP is for. A temporary host buffer is
>> allocated and data is copied in/out of it according to 'type'
>> (VERIFY_READ/VERIFY_WRITE) and 'copy'; so if the lock_user/unlock_user calls are
>> incorrect, QEMU will not read/write the correct guest memory values for syscall
>> emulation.
>> 
>> I used the logic when DEBUG_REMAP is used to understand when guest memory is
>> declared to be read/written. So the current approach for tracing is never in a
>> wrong position.
>> 
>> The only problem I see is that the syscall emulation code can declare it wants
>> read & write access to some guest memory while a real system would only do one
>> of the two (or even none). Tracing this is not incorrect either, since it's just
>> how QEMU is emulating that syscall, but it could be sub-optimal compared to a
>> real syscall implementation.

> Also we sometimes lock an entire buffer but then don't read or write
> all of it. And a few places call g2h() directly and accesses via
> those code paths won't get traced.

> But really, an access to implement a syscall isn't a guest memory
> access, it's part of the implementation of the emulated kernel ABIs.
> We have tracing for that, it's the strace code (though the strace
> code is really very poor currently).

> Tracing memory accesses made by the emulated CPU makes total sense;
> but I think "what tracing should we do for linux-user code" is
> a separate question.

I see; I wasn't aware of strace support in QEMU. Maybe tracing that is going to
be an easier alternative if there is no clear way to express "guest memory
accesses from emulated syscalls".

We could just have a very simple pair of events:

  syscallN_pre(uint64_t num, uint64_t arg1, ..., uint64_t argN)
  syscallN_post(uint64_t num, uint64_t res, uint64_t arg1, ..., uint64_t argN)

hooked into the calls to strace.


Cheers,
  Lluis