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.

squash! cputlb: ensure we save the IOTLB entry in case of reset
---
 accel/tcg/cputlb.c | 63 ++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 61 insertions(+), 2 deletions(-)

diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
index eb2cf9de5e6..9bf9e479c7c 100644
--- a/accel/tcg/cputlb.c
+++ b/accel/tcg/cputlb.c
@@ -1058,6 +1058,47 @@ static uint64_t io_readx(CPUArchState *env, CPUIOTLBEntry *iotlbentry,
     return val;
 }
 
+#ifdef CONFIG_PLUGIN
+
+typedef struct SavedIOTLB {
+    struct rcu_head rcu;
+    struct SavedIOTLB **save_loc;
+    MemoryRegionSection *section;
+    hwaddr mr_offset;
+} SavedIOTLB;
+
+static void clean_saved_entry(SavedIOTLB *s)
+{
+    atomic_rcu_set(s->save_loc, NULL);
+    g_free(s);
+}
+
+static __thread SavedIOTLB *saved_for_plugin;
+
+/*
+ * 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(MemoryRegionSection *section, hwaddr mr_offset)
+{
+    SavedIOTLB *s = g_new(SavedIOTLB, 1);
+    s->save_loc = &saved_for_plugin;
+    s->section = section;
+    s->mr_offset = mr_offset;
+    atomic_rcu_set(&saved_for_plugin, s);
+    call_rcu(s, clean_saved_entry, rcu);
+}
+
+#else
+static void save_iotlb_data(MemoryRegionSection *section, hwaddr mr_offset)
+{
+    /* do nothing */
+}
+#endif
+
 static void io_writex(CPUArchState *env, CPUIOTLBEntry *iotlbentry,
                       int mmu_idx, uint64_t val, target_ulong addr,
                       uintptr_t retaddr, MemOp op)
@@ -1077,6 +1118,12 @@ static void io_writex(CPUArchState *env, CPUIOTLBEntry *iotlbentry,
     }
     cpu->mem_io_pc = retaddr;
 
+    /*
+     * The memory_region_dispatch may trigger a flush/resize
+     * so for plugins we save the iotlb_data just in case.
+     */
+    save_iotlb_data(section, mr_offset);
+
     if (mr->global_locking && !qemu_mutex_iothread_locked()) {
         qemu_mutex_lock_iothread();
         locked = true;
@@ -1091,6 +1138,7 @@ static void io_writex(CPUArchState *env, CPUIOTLBEntry *iotlbentry,
                                MMU_DATA_STORE, mmu_idx, iotlbentry->attrs, r,
                                retaddr);
     }
+
     if (locked) {
         qemu_mutex_unlock_iothread();
     }
@@ -1366,8 +1414,11 @@ void *tlb_vaddr_to_host(CPUArchState *env, abi_ptr addr,
  * in the softmmu lookup code (or helper). We don't handle re-fills or
  * checking the victim table. This is purely informational.
  *
- * This should never fail as the memory access being instrumented
- * should have just filled the TLB.
+ * This almost never fails as the memory access being instrumented
+ * should have just filled the TLB. The one corner case is io_writex
+ * which can cause TLB flushes and potential resizing of the TLBs
+ * loosing the information we need. In those cases we need to recover
+ * data from a thread local copy of the io_tlb entry.
  */
 
 bool tlb_plugin_lookup(CPUState *cpu, target_ulong addr, int mmu_idx,
@@ -1391,6 +1442,14 @@ bool tlb_plugin_lookup(CPUState *cpu, target_ulong addr, int mmu_idx,
             data->v.ram.hostaddr = addr + tlbe->addend;
         }
         return true;
+    } else {
+        SavedIOTLB *saved = atomic_rcu_read(&saved_for_plugin);
+        if (saved) {
+            data->is_io = true;
+            data->v.io.section = saved->section;
+            data->v.io.offset = saved->mr_offset;
+            return true;
+        }
     }
     return false;
 }
-- 
2.20.1

On Wed, Jun 10, 2020 at 16:55:06 +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>
> 
> ---
> v2
>   - save the entry instead of re-running the tlb_fill.
> 
> squash! cputlb: ensure we save the IOTLB entry in case of reset
> ---
>  accel/tcg/cputlb.c | 63 ++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 61 insertions(+), 2 deletions(-)
> 
> diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
> index eb2cf9de5e6..9bf9e479c7c 100644
> --- a/accel/tcg/cputlb.c
> +++ b/accel/tcg/cputlb.c
> @@ -1058,6 +1058,47 @@ static uint64_t io_readx(CPUArchState *env, CPUIOTLBEntry *iotlbentry,
>      return val;
>  }
>  
> +#ifdef CONFIG_PLUGIN
> +
> +typedef struct SavedIOTLB {
> +    struct rcu_head rcu;
> +    struct SavedIOTLB **save_loc;
> +    MemoryRegionSection *section;
> +    hwaddr mr_offset;
> +} SavedIOTLB;
> +
> +static void clean_saved_entry(SavedIOTLB *s)
> +{
> +    atomic_rcu_set(s->save_loc, NULL);

This will race with the CPU thread that sets saved_for_plugin in
save_iotlb_data().

> +    g_free(s);
> +}
> +
> +static __thread SavedIOTLB *saved_for_plugin;

Apologies if this has been discussed, but why is this using TLS
variables and not state embedded in CPUState?
I see that qemu_plugin_get_hwaddr does not take a cpu_index, but
maybe it should? We could then just embed the RCU pointer in CPUState.

> +
> +/*
> + * 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(MemoryRegionSection *section, hwaddr mr_offset)
> +{
> +    SavedIOTLB *s = g_new(SavedIOTLB, 1);
> +    s->save_loc = &saved_for_plugin;
> +    s->section = section;
> +    s->mr_offset = mr_offset;
> +    atomic_rcu_set(&saved_for_plugin, s);
> +    call_rcu(s, clean_saved_entry, rcu);

Here we could just publish the new pointer and g_free_rcu the old
one, if any.

> +}
> +
> +#else
> +static void save_iotlb_data(MemoryRegionSection *section, hwaddr mr_offset)
> +{
> +    /* do nothing */
> +}
> +#endif
> +
>  static void io_writex(CPUArchState *env, CPUIOTLBEntry *iotlbentry,
>                        int mmu_idx, uint64_t val, target_ulong addr,
>                        uintptr_t retaddr, MemOp op)
> @@ -1077,6 +1118,12 @@ static void io_writex(CPUArchState *env, CPUIOTLBEntry *iotlbentry,
>      }
>      cpu->mem_io_pc = retaddr;
>  
> +    /*
> +     * The memory_region_dispatch may trigger a flush/resize
> +     * so for plugins we save the iotlb_data just in case.
> +     */
> +    save_iotlb_data(section, mr_offset);
> +
>      if (mr->global_locking && !qemu_mutex_iothread_locked()) {
>          qemu_mutex_lock_iothread();
>          locked = true;
> @@ -1091,6 +1138,7 @@ static void io_writex(CPUArchState *env, CPUIOTLBEntry *iotlbentry,
>                                 MMU_DATA_STORE, mmu_idx, iotlbentry->attrs, r,
>                                 retaddr);
>      }
> +

Stray whitespace change.

>      if (locked) {
>          qemu_mutex_unlock_iothread();
>      }
> @@ -1366,8 +1414,11 @@ void *tlb_vaddr_to_host(CPUArchState *env, abi_ptr addr,
>   * in the softmmu lookup code (or helper). We don't handle re-fills or
>   * checking the victim table. This is purely informational.
>   *
> - * This should never fail as the memory access being instrumented
> - * should have just filled the TLB.
> + * This almost never fails as the memory access being instrumented
> + * should have just filled the TLB. The one corner case is io_writex
> + * which can cause TLB flushes and potential resizing of the TLBs
> + * loosing the information we need. In those cases we need to recover
> + * data from a thread local copy of the io_tlb entry.
>   */
>  
>  bool tlb_plugin_lookup(CPUState *cpu, target_ulong addr, int mmu_idx,
> @@ -1391,6 +1442,14 @@ bool tlb_plugin_lookup(CPUState *cpu, target_ulong addr, int mmu_idx,
>              data->v.ram.hostaddr = addr + tlbe->addend;
>          }
>          return true;
> +    } else {
> +        SavedIOTLB *saved = atomic_rcu_read(&saved_for_plugin);
> +        if (saved) {
> +            data->is_io = true;
> +            data->v.io.section = saved->section;
> +            data->v.io.offset = saved->mr_offset;
> +            return true;
> +        }

Shouldn't we check that the contents of the saved IOTLB match the
parameters of the lookup? Otherwise passing a random address is likely
to land here.

Thanks,
		Emilio

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

> On Wed, Jun 10, 2020 at 16:55:06 +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>
>> 
>> ---
>> v2
>>   - save the entry instead of re-running the tlb_fill.
>> 
>> squash! cputlb: ensure we save the IOTLB entry in case of reset
>> ---
>>  accel/tcg/cputlb.c | 63 ++++++++++++++++++++++++++++++++++++++++++++--
>>  1 file changed, 61 insertions(+), 2 deletions(-)
>> 
>> diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
>> index eb2cf9de5e6..9bf9e479c7c 100644
>> --- a/accel/tcg/cputlb.c
>> +++ b/accel/tcg/cputlb.c
>> @@ -1058,6 +1058,47 @@ static uint64_t io_readx(CPUArchState *env, CPUIOTLBEntry *iotlbentry,
>>      return val;
>>  }
>>  
>> +#ifdef CONFIG_PLUGIN
>> +
>> +typedef struct SavedIOTLB {
>> +    struct rcu_head rcu;
>> +    struct SavedIOTLB **save_loc;
>> +    MemoryRegionSection *section;
>> +    hwaddr mr_offset;
>> +} SavedIOTLB;
>> +
>> +static void clean_saved_entry(SavedIOTLB *s)
>> +{
>> +    atomic_rcu_set(s->save_loc, NULL);
>
> This will race with the CPU thread that sets saved_for_plugin in
> save_iotlb_data().

Surely that only happens outside the critical section?

>
>> +    g_free(s);
>> +}
>> +
>> +static __thread SavedIOTLB *saved_for_plugin;
>
> Apologies if this has been discussed, but why is this using TLS
> variables and not state embedded in CPUState?

Good point - I guess I;m being lazy.

> I see that qemu_plugin_get_hwaddr does not take a cpu_index, but
> maybe it should? We could then just embed the RCU pointer in CPUState.
>
>> +
>> +/*
>> + * 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(MemoryRegionSection *section, hwaddr mr_offset)
>> +{
>> +    SavedIOTLB *s = g_new(SavedIOTLB, 1);
>> +    s->save_loc = &saved_for_plugin;
>> +    s->section = section;
>> +    s->mr_offset = mr_offset;
>> +    atomic_rcu_set(&saved_for_plugin, s);
>> +    call_rcu(s, clean_saved_entry, rcu);
>
> Here we could just publish the new pointer and g_free_rcu the old
> one, if any.

That would be simpler. I'll re-spin.

>
>> +}
>> +
>> +#else
>> +static void save_iotlb_data(MemoryRegionSection *section, hwaddr mr_offset)
>> +{
>> +    /* do nothing */
>> +}
>> +#endif
>> +
>>  static void io_writex(CPUArchState *env, CPUIOTLBEntry *iotlbentry,
>>                        int mmu_idx, uint64_t val, target_ulong addr,
>>                        uintptr_t retaddr, MemOp op)
>> @@ -1077,6 +1118,12 @@ static void io_writex(CPUArchState *env, CPUIOTLBEntry *iotlbentry,
>>      }
>>      cpu->mem_io_pc = retaddr;
>>  
>> +    /*
>> +     * The memory_region_dispatch may trigger a flush/resize
>> +     * so for plugins we save the iotlb_data just in case.
>> +     */
>> +    save_iotlb_data(section, mr_offset);
>> +
>>      if (mr->global_locking && !qemu_mutex_iothread_locked()) {
>>          qemu_mutex_lock_iothread();
>>          locked = true;
>> @@ -1091,6 +1138,7 @@ static void io_writex(CPUArchState *env, CPUIOTLBEntry *iotlbentry,
>>                                 MMU_DATA_STORE, mmu_idx, iotlbentry->attrs, r,
>>                                 retaddr);
>>      }
>> +
>
> Stray whitespace change.
>
>>      if (locked) {
>>          qemu_mutex_unlock_iothread();
>>      }
>> @@ -1366,8 +1414,11 @@ void *tlb_vaddr_to_host(CPUArchState *env, abi_ptr addr,
>>   * in the softmmu lookup code (or helper). We don't handle re-fills or
>>   * checking the victim table. This is purely informational.
>>   *
>> - * This should never fail as the memory access being instrumented
>> - * should have just filled the TLB.
>> + * This almost never fails as the memory access being instrumented
>> + * should have just filled the TLB. The one corner case is io_writex
>> + * which can cause TLB flushes and potential resizing of the TLBs
>> + * loosing the information we need. In those cases we need to recover
>> + * data from a thread local copy of the io_tlb entry.
>>   */
>>  
>>  bool tlb_plugin_lookup(CPUState *cpu, target_ulong addr, int mmu_idx,
>> @@ -1391,6 +1442,14 @@ bool tlb_plugin_lookup(CPUState *cpu, target_ulong addr, int mmu_idx,
>>              data->v.ram.hostaddr = addr + tlbe->addend;
>>          }
>>          return true;
>> +    } else {
>> +        SavedIOTLB *saved = atomic_rcu_read(&saved_for_plugin);
>> +        if (saved) {
>> +            data->is_io = true;
>> +            data->v.io.section = saved->section;
>> +            data->v.io.offset = saved->mr_offset;
>> +            return true;
>> +        }
>
> Shouldn't we check that the contents of the saved IOTLB match the
> parameters of the lookup? Otherwise passing a random address is likely
> to land here.

Good point - I'm being too trusting here ;-)

Thanks for the review.

-- 
Alex Bennée

On Mon, Jun 22, 2020 at 10:02:50 +0100, Alex Bennée wrote:
> Emilio G. Cota <cota@braap.org> writes:
> > On Wed, Jun 10, 2020 at 16:55:06 +0100, Alex Bennée wrote:
(snip)
> >> +#ifdef CONFIG_PLUGIN
> >> +
> >> +typedef struct SavedIOTLB {
> >> +    struct rcu_head rcu;
> >> +    struct SavedIOTLB **save_loc;
> >> +    MemoryRegionSection *section;
> >> +    hwaddr mr_offset;
> >> +} SavedIOTLB;
> >> +
> >> +static void clean_saved_entry(SavedIOTLB *s)
> >> +{
> >> +    atomic_rcu_set(s->save_loc, NULL);
> >
> > This will race with the CPU thread that sets saved_for_plugin in
> > save_iotlb_data().
> 
> Surely that only happens outside the critical section?

I am not sure which critical section you're referring to.

With call_rcu() we defer the execution of the function to the RCU
thread at a later time, where "later time" is defined as any time
after the pre-existing RCU read critical sections have elapsed.

So we could have the RCU thread clearing the variable while the
CPU thread, which is in a _later_ RCU read critical section, is
setting said variable. This is the race I was referring to.

Thanks,
	
		Emilio