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

[Alex Bennée]

Emilio G. Cota <cota@braap.org> writes:

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

I was just conscious the data is not always valid - clearing it up with
RCU at least ensured it went away eventually. However we could certainly
just park it in the general CPUState and assert the match on the fall
through case of tlb_plugin_lookup.

My only worry is do we ever see us storing data that is valid but the
re-filled data in the real TLB could still match and be incorrect?

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

I think that is certainly an option for evolving the API but I'd rather
just get this fix in for now while we ponder what else will be in v2 of
the API.

So far my proposal for qemu_plugin_hwaddr_device_name has some mild push
back from Peter which I'd like to resolve first:

  Date: Wed, 3 Jun 2020 16:48:33 +0100
  Message-ID: 
<CAFEAcA-kPZoumxfLgjxPfCPDmPgsAFCjB-zdicsiGeqSOPOH7Q@mail.gmail.com>

So far I've been adding stuff to solve any particular problems I've had
and I'd like to see what other API/hooks are being proposed so we don't
have browser style version ticks ;-)

>
> Thanks,
>               E.


-- 
Alex Bennée