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

[Emilio G. Cota]

On Thu, Jul 09, 2020 at 15:13:18 +0100, 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>

Reviewed-by: Emilio G. Cota <cota@braap.org>

Some minor comments below.


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

Mentioning the thread storage is now outdated -- I think this comment
(starting from 'We') can be removed.

> + */
> +static void save_iotlb_data(CPUState *cs, hwaddr addr, MemoryRegionSection 
> *section, hwaddr mr_offset)
> +{
> +    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);
> +    }

Using atomic_rcu_read here is not necessary (only this thread ever writes
to this field) and might confuse a reader when trying to find the
atomic_rcu_read that matches the atomic_rcu_set (that read is in
tlb_plugin_lookup).

Consider doing
        old = cs->saved_iotlb;
instead.

Thanks,
                Emilio