On 8/3/21 6:14 AM, Richard Henderson wrote:
> There is no point in encoding load/store within a bit of
> the memory trace info operand. Represent atomic operations
> as a single read-modify-write tracepoint. Use MemOpIdx
> instead of inventing a form specifically for traces.
>
> Signed-off-by: Richard Henderson <richard.hender...@linaro.org>
> ---
> accel/tcg/atomic_template.h | 1 -
> trace/mem.h | 51 -----------------------------------
> accel/tcg/cputlb.c | 7 ++---
> accel/tcg/user-exec.c | 43 ++++++++++-------------------
> tcg/tcg-op.c | 17 +++---------
> accel/tcg/atomic_common.c.inc | 12 +++------
> trace-events | 18 +++----------
> 7 files changed, 27 insertions(+), 122 deletions(-)
> delete mode 100644 trace/mem.h
>
> diff --git a/accel/tcg/atomic_template.h b/accel/tcg/atomic_template.h
> index c08d859a8a..2d917b6b1f 100644
> --- a/accel/tcg/atomic_template.h
> +++ b/accel/tcg/atomic_template.h
> @@ -19,7 +19,6 @@
> */
>
> #include "qemu/plugin.h"
> -#include "trace/mem.h"
>
> #if DATA_SIZE == 16
> # define SUFFIX o
> diff --git a/trace/mem.h b/trace/mem.h
> deleted file mode 100644
> index 699566c661..0000000000
> --- a/trace/mem.h
> +++ /dev/null
> @@ -1,51 +0,0 @@
> -/*
> - * Helper functions for guest memory tracing
> - *
> - * Copyright (C) 2016 Lluís Vilanova <vilan...@ac.upc.edu>
> - *
> - * This work is licensed under the terms of the GNU GPL, version 2 or later.
> - * See the COPYING file in the top-level directory.
> - */
> -
> -#ifndef TRACE__MEM_H
> -#define TRACE__MEM_H
> -
> -#include "exec/memopidx.h"
> -
> -#define TRACE_MEM_SZ_SHIFT_MASK 0xf /* size shift mask */
> -#define TRACE_MEM_SE (1ULL << 4) /* sign extended (y/n) */
> -#define TRACE_MEM_BE (1ULL << 5) /* big endian (y/n) */
> -#define TRACE_MEM_ST (1ULL << 6) /* store (y/n) */
> -#define TRACE_MEM_MMU_SHIFT 8 /* mmu idx */
> -
> -/**
> - * trace_mem_get_info:
> - *
> - * Return a value for the 'info' argument in guest memory access traces.
> - */
> -static inline uint16_t trace_mem_get_info(MemOpIdx oi, bool store)
> -{
> - MemOp op = get_memop(oi);
> - uint32_t size_shift = op & MO_SIZE;
> - bool sign_extend = op & MO_SIGN;
> - bool big_endian = (op & MO_BSWAP) == MO_BE;
> - uint16_t res;
> -
> - res = size_shift & TRACE_MEM_SZ_SHIFT_MASK;
> - if (sign_extend) {
> - res |= TRACE_MEM_SE;
> - }
> - if (big_endian) {
> - res |= TRACE_MEM_BE;
> - }
> - if (store) {
> - res |= TRACE_MEM_ST;
> - }
> -#ifdef CONFIG_SOFTMMU
> - res |= get_mmuidx(oi) << TRACE_MEM_MMU_SHIFT;
> -#endif
> -
> - return res;
> -}
> diff --git a/trace-events b/trace-events
> index c4cca29939..a637a61eba 100644
> --- a/trace-events
> +++ b/trace-events
> @@ -120,26 +120,16 @@ vcpu guest_cpu_reset(void)
> # tcg/tcg-op.c
>
> # @vaddr: Access' virtual address.
> -# @info : Access' information (see below).
> +# @memopidx: Access' information (see below).
> #
> # Start virtual memory access (before any potential access violation).
> -#
> # Does not include memory accesses performed by devices.
> #
> -# Access information can be parsed as:
> -#
> -# struct mem_info {
> -# uint8_t size_shift : 4; /* interpreted as "1 << size_shift" bytes */
> -# bool sign_extend: 1; /* sign-extended */
> -# uint8_t endianness : 1; /* 0: little, 1: big */
> -# bool store : 1; /* whether it is a store operation */
> -# pad : 1;
> -# uint8_t mmuidx : 4; /* mmuidx (softmmu only) */
> -# };
> -#
> # Mode: user, softmmu
> # Targets: TCG(all)
> -vcpu tcg guest_mem_before(TCGv vaddr, uint16_t info) "info=%d",
> "vaddr=0x%016"PRIx64" info=%d"
> +vcpu tcg guest_ld_before(TCGv vaddr, uint32_t memopidx) "info=%d",
> "vaddr=0x%016"PRIx64" memopidx=0x%x"
> +vcpu tcg guest_st_before(TCGv vaddr, uint32_t memopidx) "info=%d",
> "vaddr=0x%016"PRIx64" memopidx=0x%x"
> +vcpu tcg guest_rmw_before(TCGv vaddr, uint32_t memopidx) "info=%d",
> "vaddr=0x%016"PRIx64" memopidx=0x%x"
Alternatively we can implement:
const char *memopidx_name(MemOpIdx oi);
and have the trace events display the MemOpIdx name.
Anyway,
Reviewed-by: Philippe Mathieu-Daudé <f4...@amsat.org>
(also, maybe "trace/mem:" in subject).
