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

[Pierrick Bouvier]

On 7/3/24 11:58, Richard Henderson wrote:
> 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.

That's a good suggestion. Moved it here for v5.

> > +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?

This was to truncate upper value, but as you mentioned, I simply clip it 
later, so it's not important.

> (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.

I implemented selective store for plugin_gen_mem_callbacks function for 
next version.

However, for helpers based memory accesses, I prefer to keep existing 
version. It would require to create several small functions 
(atomic_trace_rmw_post, plugin_load/store_cb and qemu_plugin_vcpu_mem_cb 
for every size). It's something that could be desirable later when we'll 
introduce bigger tcg word size though.

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