Re: [PATCH v2 2/7] plugins: save value during memory accesses

[Richard Henderson <richard.henderson@linaro.org>]

On 6/26/24 16:37, Pierrick Bouvier wrote:
> Different code paths handle memory accesses:
> - tcg generated code
> - load/store helpers
> - atomic helpers
> 
> This value is saved in cpu->plugin_state.
> 
> Atomic operations are doing read/write at the same time, so we generate
> two memory callbacks instead of one, to allow plugins to access distinct
> values.
> 
> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> ---
>   accel/tcg/atomic_template.h   | 66 ++++++++++++++++++++++++++++----
>   include/qemu/plugin.h         |  8 ++++
>   plugins/core.c                |  7 ++++
>   tcg/tcg-op-ldst.c             | 72 +++++++++++++++++++++++++++++++----
>   accel/tcg/atomic_common.c.inc | 13 ++++++-
>   accel/tcg/ldst_common.c.inc   | 38 +++++++++++-------
>   6 files changed, 173 insertions(+), 31 deletions(-)
> 
> diff --git a/accel/tcg/atomic_template.h b/accel/tcg/atomic_template.h
> index 1dc2151dafd..830e4f16069 100644
> --- a/accel/tcg/atomic_template.h
> +++ b/accel/tcg/atomic_template.h
> @@ -53,6 +53,14 @@
>   # error unsupported data size
>   #endif
>   
> +#if DATA_SIZE == 16
> +# define UPPER_MEMORY_VALUE(val) int128_gethi(val)
> +# define LOWER_MEMORY_VALUE(val) int128_getlo(val)
> +#else
> +# define UPPER_MEMORY_VALUE(val) 0
> +# define LOWER_MEMORY_VALUE(val) val
> +#endif
> +
>   #if DATA_SIZE >= 4
>   # define ABI_TYPE  DATA_TYPE
>   #else
> @@ -83,7 +91,12 @@ ABI_TYPE ATOMIC_NAME(cmpxchg)(CPUArchState *env, abi_ptr addr,
>       ret = qatomic_cmpxchg__nocheck(haddr, cmpv, newv);
>   #endif
>       ATOMIC_MMU_CLEANUP;
> -    atomic_trace_rmw_post(env, addr, oi);
> +    atomic_trace_rmw_post(env, addr,
> +                          UPPER_MEMORY_VALUE(ret),
> +                          LOWER_MEMORY_VALUE(ret),
> +                          UPPER_MEMORY_VALUE(newv),
> +                          LOWER_MEMORY_VALUE(newv),
> +                          oi);

Just a nit, but tcg is consistent in using little-endian argument ordering for values 
passed by parts.  I would prefer we continue with that.


> @@ -142,9 +142,13 @@ struct qemu_plugin_tb {
>   /**
>    * struct CPUPluginState - per-CPU state for plugins
>    * @event_mask: plugin event bitmap. Modified only via async work.
> + * @mem_value_upper_bits: 64 upper bits of latest accessed mem value.
> + * @mem_value_lower_bits: 64 lower bits of latest accessed mem value.
>    */
>   struct CPUPluginState {
>       DECLARE_BITMAP(event_mask, QEMU_PLUGIN_EV_MAX);
> +    uint64_t mem_value_upper_bits;
> +    uint64_t mem_value_lower_bits;
>   };

At some point we may well support 32 byte acceses, for better guest vector support.  Do we 
have a plan for this beyond "add more fields here"?


r~