diff mbox series

[v2,06/11] cputlb: ensure we save the IOTLB data in case of reset

Message ID 20200713200415.26214-7-alex.bennee@linaro.org
State Superseded
Headers show
Series misc fixes for rc0 (docker, plugins, softfloat) | expand

Commit Message

Alex Bennée July 13, 2020, 8:04 p.m. UTC 
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
v4
  - don't both with RCU, just store it in CPUState
  - clean-up #ifdef'ery
  - checkpatch
---
 include/hw/core/cpu.h   | 16 ++++++++++++++++
 include/qemu/typedefs.h |  1 +
 accel/tcg/cputlb.c      | 38 +++++++++++++++++++++++++++++++++++---
 3 files changed, 52 insertions(+), 3 deletions(-)

-- 
2.20.1

Comments

Richard Henderson July 13, 2020, 9:58 p.m. UTC | #1 
On 7/13/20 1:04 PM, 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: Richard Henderson <richard.henderson@linaro.org>


r~
Emilio Cota July 18, 2020, 8:51 p.m. UTC | #2 
On Mon, Jul 13, 2020 at 21:04:10 +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>

(snip)
> +/*

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


As I mentioned in the previous iteration of this series, this comment is
outdated -- there is no thread storage nor RCU to worry about here.

> + * 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 copy of the io_tlb entry.

>   */


s/loosing/losing/

About the approach in this patch: it works as long as the caller is in
the same vCPU thread, otherwise we'd need a seqlock to avoid races
between readers and the writing vCPU. I see that qemu_plugin_get_hwaddr
does not even take a vCPU index, so this should be OK -- as long as this
is called only from a mem callback, it's in the same vCPU thread and it's
therefore safe.

With the above comments fixed,

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


Thanks,
		Emilio
diff mbox series

Patch

diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
index 5542577d2b..8f145733ce 100644
--- a/include/hw/core/cpu.h
+++ b/include/hw/core/cpu.h
@@ -259,6 +259,18 @@  struct CPUWatchpoint {
     QTAILQ_ENTRY(CPUWatchpoint) entry;
 };
 
+#ifdef CONFIG_PLUGIN
+/*
+ * For plugins we sometime need to save the resolved iotlb data before
+ * the memory regions get moved around  by io_writex.
+ */
+typedef struct SavedIOTLB {
+    hwaddr addr;
+    MemoryRegionSection *section;
+    hwaddr mr_offset;
+} SavedIOTLB;
+#endif
+
 struct KVMState;
 struct kvm_run;
 
@@ -417,7 +429,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 15f5047bf1..427027a970 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 1e815357c7..d370aedb47 100644
--- a/accel/tcg/cputlb.c
+++ b/accel/tcg/cputlb.c
@@ -1073,6 +1073,24 @@  static uint64_t io_readx(CPUArchState *env, CPUIOTLBEntry *iotlbentry,
     return val;
 }
 
+/*
+ * 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)
+{
+#ifdef CONFIG_PLUGIN
+    SavedIOTLB *saved = &cs->saved_iotlb;
+    saved->addr = addr;
+    saved->section = section;
+    saved->mr_offset = mr_offset;
+#endif
+}
+
 static void io_writex(CPUArchState *env, CPUIOTLBEntry *iotlbentry,
                       int mmu_idx, uint64_t val, target_ulong addr,
                       uintptr_t retaddr, MemOp op)
@@ -1092,6 +1110,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(cpu, iotlbentry->addr, section, mr_offset);
+
     if (mr->global_locking && !qemu_mutex_iothread_locked()) {
         qemu_mutex_lock_iothread();
         locked = true;
@@ -1381,8 +1405,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 copy of the io_tlb entry.
  */
 
 bool tlb_plugin_lookup(CPUState *cpu, target_ulong addr, int mmu_idx,
@@ -1406,8 +1433,13 @@  bool tlb_plugin_lookup(CPUState *cpu, target_ulong addr, int mmu_idx,
             data->v.ram.hostaddr = addr + tlbe->addend;
         }
         return true;
+    } else {
+        SavedIOTLB *saved = &cpu->saved_iotlb;
+        data->is_io = true;
+        data->v.io.section = saved->section;
+        data->v.io.offset = saved->mr_offset;
+        return true;
     }
-    return false;
 }
 
 #endif