Re: [PATCH v2 28/55] trace: Split guest_mem_before

[Philippe Mathieu-Daudé]

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.henderson@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 <vilanova@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é <f4bug@amsat.org>

(also, maybe "trace/mem:" in subject).