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

[Richard Henderson]

On 6/27/24 12:25, Pierrick Bouvier wrote:
> On 6/27/24 12:10, Richard Henderson wrote:
> > 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.
>
> Didn't notice that, but I'll stick to this. Any preference on the naming as well while I'm 
> at it? (low/hi vs upper/lower)?

I guess we mostly use high/low, hi/lo, and variations thereof elsewhere as well.  I don't 
see any uses of upper/lower.

> > 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"?
>
> For now, I sticked to native tcg ops (up to 128 bits), with this simple solution. Do you 
> think tcg core will be extended to support more, or will helper simply load/store four 
> 128bits words, emitting four callbacks as well?

I assume we'll support a 256-bit (non-atomic) memory operation.
That avoids some of the "probe for write, perform stores after we know it's safe" sort of 
affair.

I don't think I'll do it while i686 is still a supported host though.

> If you have a better idea, I'm open to implement an alternative, but didn't want to think 
> too much ahead.

Fair.


r~