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.ben...@linaro.org>
Reviewed-by: Emilio G. Cota <c...@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
