Re: [PATCH] system/physmem: Fix migration dirty bitmap coherency with TCG memory access

[Thomas Huth]

 Hi Peter, Paolo, David,

this patch fixes a problem with the kvm-unit-tests ... could we get it 
included in QEMU 9.0 ?

 Thanks,
  Thomas


On 19/02/2024 07.17, Nicholas Piggin wrote:
The fastpath in cpu_physical_memory_sync_dirty_bitmap() to test large
aligned ranges forgot to bring the TCG TLB up to date after clearing
some of the dirty memory bitmap bits. This can result in stores though
the TCG TLB not setting the dirty memory bitmap and ultimately causes
memory corruption / lost updates during migration from a TCG host.

Fix this by exporting an abstracted function to call when dirty bits
have been cleared.

Fixes: aa8dc044772 ("migration: synchronize memory bitmap 64bits at a time")
Signed-off-by: Nicholas Piggin <npig...@gmail.com>
---

I reproduced this with a kvm-unit-tests migration stress tester I'm
working on. Tree here with reproduce instructions in latest commit.

https://github.com/npiggin/kvm-unit-tests/tree/qemu-tcg-migration-bug

Thanks,
Nick

  include/exec/ram_addr.h | 13 +++++++++++++
  system/physmem.c        | 14 +++++++++-----
  2 files changed, 22 insertions(+), 5 deletions(-)

diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
index 90676093f5..dadb2deb11 100644
--- a/include/exec/ram_addr.h
+++ b/include/exec/ram_addr.h
@@ -443,6 +443,16 @@ uint64_t cpu_physical_memory_set_dirty_lebitmap(unsigned 
long *bitmap,
  }
  #endif /* not _WIN32 */
  
+void tcg_cpu_physical_memory_dirty_bits_cleared(ram_addr_t start,
+                                                ram_addr_t length);
+static inline void cpu_physical_memory_dirty_bits_cleared(ram_addr_t start,
+                                                          ram_addr_t length)
+{
+    if (tcg_enabled()) {
+        tcg_cpu_physical_memory_dirty_bits_cleared(start, length);
+    }
+
+}
  bool cpu_physical_memory_test_and_clear_dirty(ram_addr_t start,
                                                ram_addr_t length,
                                                unsigned client);
@@ -504,6 +514,9 @@ uint64_t cpu_physical_memory_sync_dirty_bitmap(RAMBlock *rb,
                  idx++;
              }
          }
+        if (num_dirty) {
+            cpu_physical_memory_dirty_bits_cleared(start, length);
+        }
  
          if (rb->clear_bmap) {
              /*
diff --git a/system/physmem.c b/system/physmem.c
index 5e66d9ae36..dc0d8b16aa 100644
--- a/system/physmem.c
+++ b/system/physmem.c
@@ -839,6 +839,12 @@ static void tlb_reset_dirty_range_all(ram_addr_t start, 
ram_addr_t length)
      }
  }
  
+void tcg_cpu_physical_memory_dirty_bits_cleared(ram_addr_t start,
+                                                ram_addr_t length)
+{
+    tlb_reset_dirty_range_all(start, length);
+}
+
  /* Note: start and end must be within the same ram block.  */
  bool cpu_physical_memory_test_and_clear_dirty(ram_addr_t start,
                                                ram_addr_t length,
@@ -881,8 +887,8 @@ bool cpu_physical_memory_test_and_clear_dirty(ram_addr_t 
start,
          memory_region_clear_dirty_bitmap(ramblock->mr, mr_offset, mr_size);
      }
  
-    if (dirty && tcg_enabled()) {
-        tlb_reset_dirty_range_all(start, length);
+    if (dirty) {
+        cpu_physical_memory_dirty_bits_cleared(start, length);
      }
  
      return dirty;
@@ -929,9 +935,7 @@ DirtyBitmapSnapshot 
*cpu_physical_memory_snapshot_and_clear_dirty
          }
      }
  
-    if (tcg_enabled()) {
-        tlb_reset_dirty_range_all(start, length);
-    }
+    cpu_physical_memory_dirty_bits_cleared(start, length);
  
      memory_region_clear_dirty_bitmap(mr, offset, length);