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.ben...@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.
