@@ -654,7 +654,6 @@ static inline void mmap_unlock(void) {}
#define WITH_MMAP_LOCK_GUARD()
void tlb_reset_dirty(CPUState *cpu, ram_addr_t start1, ram_addr_t length);
-void tlb_set_dirty(CPUState *cpu, vaddr addr);
MemoryRegionSection *
address_space_translate_for_iotlb(CPUState *cpu, int asidx, hwaddr addr,
@@ -18,10 +18,6 @@ void tb_flush(CPUState *cpu)
{
}
-void tlb_set_dirty(CPUState *cpu, vaddr vaddr)
-{
-}
-
int probe_access_flags(CPUArchState *env, vaddr addr, int size,
MMUAccessType access_type, int mmu_idx,
bool nonfault, void **phost, uintptr_t retaddr)
@@ -1001,10 +1001,17 @@ static inline void copy_tlb_helper_locked(CPUTLBEntry *d, const CPUTLBEntry *s)
*d = *s;
}
-/* This is a cross vCPU call (i.e. another vCPU resetting the flags of
+/*
+ * This is a cross vCPU call (i.e. another vCPU resetting the flags of
* the target vCPU).
* We must take tlb_c.lock to avoid racing with another vCPU update. The only
* thing actually updated is the target TLB entry ->addr_write flags.
+ *
+ * This can race between the physical memory dirty bits becoming all set and
+ * tlb_set_page_full or tlb_try_set_dirty creating dirty entries: it is
+ * harmless to set TLB_NOTDIRTY on a !clean physical page, it might just
+ * cause a notdirty_write() slowpath on the next write which clears out the
+ * spurious TLB_NOTDIRTY.
*/
void tlb_reset_dirty(CPUState *cpu, ram_addr_t start1, ram_addr_t length)
{
@@ -1037,9 +1044,11 @@ static inline void tlb_set_dirty1_locked(CPUTLBEntry *tlb_entry,
}
}
-/* update the TLB corresponding to virtual page vaddr
- so that it is no longer dirty */
-void tlb_set_dirty(CPUState *cpu, vaddr addr)
+/*
+ * Update the TLB corresponding to virtual page vaddr if it no longer
+ * requires the TLB_NOTDIRTY bit.
+ */
+static void tlb_try_set_dirty(CPUState *cpu, vaddr addr, ram_addr_t ram_addr)
{
int mmu_idx;
@@ -1047,6 +1056,12 @@ void tlb_set_dirty(CPUState *cpu, vaddr addr)
addr &= TARGET_PAGE_MASK;
qemu_spin_lock(&cpu->neg.tlb.c.lock);
+ if (cpu_physical_memory_is_clean(ram_addr)) {
+ /* Must be checked under lock, like tlb_set_page_full */
+ qemu_spin_unlock(&cpu->neg.tlb.c.lock);
+ return;
+ }
+
for (mmu_idx = 0; mmu_idx < NB_MMU_MODES; mmu_idx++) {
tlb_set_dirty1_locked(tlb_entry(cpu, mmu_idx, addr), addr);
}
@@ -1165,6 +1180,19 @@ void tlb_set_page_full(CPUState *cpu, int mmu_idx,
addend = 0;
}
+ /*
+ * Hold the TLB lock for the rest of the function. We could acquire/release
+ * the lock several times in the function, but it is faster to amortize the
+ * acquisition cost by acquiring it just once. Note that this leads to
+ * a longer critical section, but this is not a concern since the TLB lock
+ * is unlikely to be contended.
+ *
+ * The TLB lock must be held over checking cpu_physical_memory_is_clean
+ * and inserting the entry into the TLB, so clearing phsyical memory
+ * dirty status can find and set TLB_NOTDIRTY on the entries.
+ */
+ qemu_spin_lock(&tlb->c.lock);
+
write_flags = read_flags;
if (is_ram) {
iotlb = memory_region_get_ram_addr(section->mr) + xlat;
@@ -1200,15 +1228,6 @@ void tlb_set_page_full(CPUState *cpu, int mmu_idx,
index = tlb_index(cpu, mmu_idx, addr_page);
te = tlb_entry(cpu, mmu_idx, addr_page);
- /*
- * Hold the TLB lock for the rest of the function. We could acquire/release
- * the lock several times in the function, but it is faster to amortize the
- * acquisition cost by acquiring it just once. Note that this leads to
- * a longer critical section, but this is not a concern since the TLB lock
- * is unlikely to be contended.
- */
- qemu_spin_lock(&tlb->c.lock);
-
/* Note that the tlb is no longer clean. */
tlb->c.dirty |= 1 << mmu_idx;
@@ -1409,7 +1428,7 @@ static void notdirty_write(CPUState *cpu, vaddr mem_vaddr, unsigned size,
/* We remove the notdirty callback only if the code has been flushed. */
if (!cpu_physical_memory_is_clean(ram_addr)) {
trace_memory_notdirty_set_dirty(mem_vaddr);
- tlb_set_dirty(cpu, mem_vaddr);
+ tlb_try_set_dirty(cpu, mem_vaddr, ram_addr);
}
}
The TCG TLB keeps a TLB_NOTDIRTY bit that must be kept coherent with the cpu physical dirty memory bitmaps. If any bits are clear, TLB_NOTDIRTY *must* be set in all CPUs so that the nodirty_write() slowpath is guaranteed to be called on a write access. TLB_NOTDIRTY may be set if none of the bits are clear, it just results in a superfluous nodirty_write() call (which should remove the bit). There is a race with clearing physical dirty bits and setting TLB_NOTDIRTY vs setting or creating TLB entries without the TLB_NOTDIRTY bit, that can cause the bit to get lost and notdirty_write() updates to be missed. nodirty_write() 1. cpu_physical_memory_set_dirty_range() 2. if (!cpu_physical_memory_is_clean()) 3. tlb_set_dirty() vs cpu_physical_memory_test_and_clear_dirty() A. dirty = bitmap_test_and_clear_atomic() if (dirty) B. tlb_reset_dirty_range_all() 1 executes, then 2 finds no bits clean, then A clears the dirty bit and runs B to set TLB_NOTDIRTY on the TLB entries, then 3 clears the TLB_NOTDIRTY bits from the TLB entries. This results in the physical dirty bitmap having a clear bit with a TLB entry pointing to it without TLB_NOTDIRTY, which means stores through that TLB are performed without the notdirty_write() call to set dirty bits (or invalidate tb). Fix this by checking the physical dirty bitmap while holding the TLB lock that also covers TLB entry insertion / TLB_NOTDIRTY clearing. Signed-off-by: Nicholas Piggin <npiggin@gmail.com> --- Hi Thomas, This is the other dirty bitmap race I saw, if you are hunting migration bugs... Thanks, Nick include/exec/exec-all.h | 1 - accel/stubs/tcg-stub.c | 4 ---- accel/tcg/cputlb.c | 47 +++++++++++++++++++++++++++++------------ 3 files changed, 33 insertions(+), 19 deletions(-)