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

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

On 7/2/24 11:44, 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.
> 
> For now, we can have access only up to 128 bits, thus split this in two
> 64 bits words. When QEMU will support wider operations, we'll be able to
> reconsider this.
> 
> 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(-)

It looks correct so,
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

Possibilities for follow-up improvement:


> --- a/tcg/tcg-op-ldst.c
> +++ b/tcg/tcg-op-ldst.c
> @@ -148,14 +148,24 @@ static TCGv_i64 plugin_maybe_preserve_addr(TCGTemp *addr)
>       return NULL;
>   }
>   
> +#ifdef CONFIG_PLUGIN
>   static void
> -plugin_gen_mem_callbacks(TCGv_i64 copy_addr, TCGTemp *orig_addr, MemOpIdx oi,
> +plugin_gen_mem_callbacks(TCGv_i64 value_low, TCGv_i64 value_high,
> +                         TCGv_i64 copy_addr, TCGTemp *orig_addr, MemOpIdx oi,
>                            enum qemu_plugin_mem_rw rw)
>   {
> -#ifdef CONFIG_PLUGIN
>       if (tcg_ctx->plugin_insn != NULL) {
>           qemu_plugin_meminfo_t info = make_plugin_meminfo(oi, rw);
>   
> +        TCGv_ptr plugin_state = tcg_temp_ebb_new_ptr();
> +        tcg_gen_ld_ptr(plugin_state, tcg_env,
> +                       offsetof(CPUState, plugin_state) - sizeof(CPUState));
> +        tcg_gen_st_i64(value_low, plugin_state,
> +                       offsetof(CPUPluginState, mem_value_low));
> +        tcg_gen_st_i64(value_high, plugin_state,
> +                       offsetof(CPUPluginState, mem_value_high));

Maybe better to place this data at the beginning of CPUNegativeOffsetState?
This would eliminate a load dependency and most hosts would be able to use (relatively) 
small negative offset efficiently.

> +static void
> +plugin_gen_mem_callbacks_i32(TCGv_i32 val,
> +                             TCGv_i64 copy_addr, TCGTemp *orig_addr,
> +                             MemOpIdx oi, enum qemu_plugin_mem_rw rw)
> +{
> +#ifdef CONFIG_PLUGIN
> +    if (tcg_ctx->plugin_insn != NULL) {
> +        TCGv_i64 ext_val = tcg_temp_ebb_new_i64();
> +        tcg_gen_extu_i32_i64(ext_val, val);
> +        plugin_gen_mem_callbacks(ext_val, tcg_constant_i64(0),

This zero extension got me to thinking:
(1) why zero extension and not sign-extension based on MO_SIGN from oi?
(2) given that the callback will have oi, do we really need any extension
     at all here?  We could allow the bits outside oi be garbage.
     This would eliminate the store to value_high entirely for most ops,
     and would allow this i32 op to avoid the extension -- simply perform
     a 32-bit store into the low half of value_low.

That appears to be what you're doing anyway with qemu_plugin_mem_value in the next patch.


r~