Re: [PATCH v1 04/13] cputlb: ensure we save the IOTLB data in case of reset

[Emilio G. Cota]

On Fri, Jul 10, 2020 at 14:03:27 -0700, Richard Henderson wrote:
> On 7/9/20 7:13 AM, Alex Bennée wrote:
> > Any write to a device might cause a re-arrangement of memory
> > triggering a TLB flush and potential re-size of the TLB invalidating
> > previous entries. This would cause users of qemu_plugin_get_hwaddr()
> > to see the warning:
> > 
> >   invalid use of qemu_plugin_get_hwaddr
> > 
> > because of the failed tlb_lookup which should always succeed. To
> > prevent this we save the IOTLB data in case it is later needed by a
> > plugin doing a lookup.
> > 
> > Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> > 
> > ---
> > v2
> >   - save the entry instead of re-running the tlb_fill.
> > v3
> >   - don't abuse TLS, use CPUState to store data
> >   - just use g_free_rcu() to avoid ugliness
> >   - verify addr matches before returning data
> >   - ws fix
> > ---
> >  include/hw/core/cpu.h   |  4 +++
> >  include/qemu/typedefs.h |  1 +
> >  accel/tcg/cputlb.c      | 57 +++++++++++++++++++++++++++++++++++++++--
> >  3 files changed, 60 insertions(+), 2 deletions(-)
> > 
> > diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
> > index b3f4b7931823..bedbf098dc57 100644
> > --- a/include/hw/core/cpu.h
> > +++ b/include/hw/core/cpu.h
> > @@ -417,7 +417,11 @@ struct CPUState {
> >  
> >      DECLARE_BITMAP(plugin_mask, QEMU_PLUGIN_EV_MAX);
> >  
> > +#ifdef CONFIG_PLUGIN
> >      GArray *plugin_mem_cbs;
> > +    /* saved iotlb data from io_writex */
> > +    SavedIOTLB *saved_iotlb;
> > +#endif
> >  
> >      /* TODO Move common fields from CPUArchState here. */
> >      int cpu_index;
> > diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
> > index 15f5047bf1dc..427027a9707a 100644
> > --- a/include/qemu/typedefs.h
> > +++ b/include/qemu/typedefs.h
> > @@ -116,6 +116,7 @@ typedef struct QObject QObject;
> >  typedef struct QString QString;
> >  typedef struct RAMBlock RAMBlock;
> >  typedef struct Range Range;
> > +typedef struct SavedIOTLB SavedIOTLB;
> >  typedef struct SHPCDevice SHPCDevice;
> >  typedef struct SSIBus SSIBus;
> >  typedef struct VirtIODevice VirtIODevice;
> > diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
> > index 1e815357c709..8636b66e036a 100644
> > --- a/accel/tcg/cputlb.c
> > +++ b/accel/tcg/cputlb.c
> > @@ -1073,6 +1073,42 @@ static uint64_t io_readx(CPUArchState *env, 
> > CPUIOTLBEntry *iotlbentry,
> >      return val;
> >  }
> >  
> > +#ifdef CONFIG_PLUGIN
> > +
> > +typedef struct SavedIOTLB {
> > +    struct rcu_head rcu;
> > +    hwaddr addr;
> > +    MemoryRegionSection *section;
> > +    hwaddr mr_offset;
> > +} SavedIOTLB;
> > +
> > +/*
> > + * Save a potentially trashed IOTLB entry for later lookup by plugin.
> > + *
> > + * We also need to track the thread storage address because the RCU
> > + * cleanup that runs when we leave the critical region (the current
> > + * execution) is actually in a different thread.
> > + */
> > +static void save_iotlb_data(CPUState *cs, hwaddr addr, MemoryRegionSection 
> > *section, hwaddr mr_offset)
> 
> Overlong line.
> 
> > +{
> > +    SavedIOTLB *old, *new = g_new(SavedIOTLB, 1);
> > +    new->addr = addr;
> > +    new->section = section;
> > +    new->mr_offset = mr_offset;
> > +    old = atomic_rcu_read(&cs->saved_iotlb);
> > +    atomic_rcu_set(&cs->saved_iotlb, new);
> > +    if (old) {
> > +        g_free_rcu(old, rcu);
> > +    }
> > +}
> 
> I'm a bit confused by this.  Why all the multiple allocation?  How many
> consumers are you expecting, and more are you expecting multiple memory
> operations in flight at once?
> 
> If multiple memory operations in flight, then why aren't we chaining them
> together, so that you can search through multiple alternatives.
> 
> If only one memory operation in flight, why are you allocating memory at all,
> much less managing it with rcu?  Just put one structure (or a collection of
> fields) into CPUState and be done.

Oh I just saw this reply. I subscribe all of the above, please shelve my R-b
tag until these are resolved.

An alternative is to emit the hwaddr directly in the mem_cb -- IIRC this is
how I did it originally. The API is a larger/uglier (plugins can subscribe
to either hwaddr or vaddr callbacks) but there is no state to keep and
no overhead of calling several functions in a hot path.

Thanks,
                E.