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

[Richard Henderson]

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.

> +
> +#else
> +static void save_iotlb_data(CPUState *cs, hwaddr addr, MemoryRegionSection 
> *section, hwaddr mr_offset)
> +{
> +    /* do nothing */
> +}
> +#endif

Surely better to move the ifdef inside the function so that you don't have to
replicate the definition?

> +        SavedIOTLB *saved = atomic_rcu_read(&cpu->saved_iotlb);
> +        if (saved && saved->addr == tlb_addr) {
> +            data->is_io = true;
> +            data->v.io.section = saved->section;
> +            data->v.io.offset = saved->mr_offset;
> +            return true;
> +        }

Should that test in fact be an assert?  Why would this fail?


r~