[PATCH-for-9.0 v2 0/3] system/physmem: Fix migration dirty bitmap coherency with TCG memory access

[PATCH-for-9.0 v2 0/3] system/physmem: Fix migration dirty bitmap coherency with TCG memory access

[Philippe Mathieu-Daudé]

Respin of Nicholas patch, without introducing
tcg_cpu_physical_memory_dirty_bits_cleared(),
and split in more digestible parts.

Nicholas Piggin (2):
  physmem: Factor cpu_physical_memory_dirty_bits_cleared() out
  physmem: Fix migration dirty bitmap coherency with TCG memory access

Philippe Mathieu-Daudé (1):
  physmem: Expose tlb_reset_dirty_range_all()

 include/exec/exec-all.h |  1 +
 include/exec/ram_addr.h | 12 ++++++++++++
 system/physmem.c        | 10 ++++------
 3 files changed, 17 insertions(+), 6 deletions(-)

-- 
2.41.0

[PATCH-for-9.0 v2 1/3] physmem: Expose tlb_reset_dirty_range_all()

[Philippe Mathieu-Daudé]

In order to call tlb_reset_dirty_range_all() outside of
system/physmem.c, expose its prototype.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/exec/exec-all.h | 1 +
 system/physmem.c        | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
index ce36bb10d4..3e53501691 100644
--- a/include/exec/exec-all.h
+++ b/include/exec/exec-all.h
@@ -655,6 +655,7 @@ static inline void mmap_unlock(void) {}
 
 void tlb_reset_dirty(CPUState *cpu, ram_addr_t start1, ram_addr_t length);
 void tlb_set_dirty(CPUState *cpu, vaddr addr);
+void tlb_reset_dirty_range_all(ram_addr_t start, ram_addr_t length);
 
 MemoryRegionSection *
 address_space_translate_for_iotlb(CPUState *cpu, int asidx, hwaddr addr,
diff --git a/system/physmem.c b/system/physmem.c
index 6cfb7a80ab..5441480ff0 100644
--- a/system/physmem.c
+++ b/system/physmem.c
@@ -819,7 +819,7 @@ found:
     return block;
 }
 
-static void tlb_reset_dirty_range_all(ram_addr_t start, ram_addr_t length)
+void tlb_reset_dirty_range_all(ram_addr_t start, ram_addr_t length)
 {
     CPUState *cpu;
     ram_addr_t start1;
-- 
2.41.0

Re: [PATCH-for-9.0 v2 1/3] physmem: Expose tlb_reset_dirty_range_all()

[Nicholas Piggin]

On Wed Mar 13, 2024 at 6:14 AM AEST, Philippe Mathieu-Daudé wrote:
> In order to call tlb_reset_dirty_range_all() outside of
> system/physmem.c, expose its prototype.
>

Acked-by: Nicholas Piggin <npiggin@gmail.com>

> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  include/exec/exec-all.h | 1 +
>  system/physmem.c        | 2 +-
>  2 files changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
> index ce36bb10d4..3e53501691 100644
> --- a/include/exec/exec-all.h
> +++ b/include/exec/exec-all.h
> @@ -655,6 +655,7 @@ static inline void mmap_unlock(void) {}
>  
>  void tlb_reset_dirty(CPUState *cpu, ram_addr_t start1, ram_addr_t length);
>  void tlb_set_dirty(CPUState *cpu, vaddr addr);
> +void tlb_reset_dirty_range_all(ram_addr_t start, ram_addr_t length);
>  
>  MemoryRegionSection *
>  address_space_translate_for_iotlb(CPUState *cpu, int asidx, hwaddr addr,
> diff --git a/system/physmem.c b/system/physmem.c
> index 6cfb7a80ab..5441480ff0 100644
> --- a/system/physmem.c
> +++ b/system/physmem.c
> @@ -819,7 +819,7 @@ found:
>      return block;
>  }
>  
> -static void tlb_reset_dirty_range_all(ram_addr_t start, ram_addr_t length)
> +void tlb_reset_dirty_range_all(ram_addr_t start, ram_addr_t length)
>  {
>      CPUState *cpu;
>      ram_addr_t start1;

[PATCH-for-9.0 v2 2/3] physmem: Factor cpu_physical_memory_dirty_bits_cleared() out

[Philippe Mathieu-Daudé]

From: Nicholas Piggin <npiggin@gmail.com>

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
Tested-by: Thomas Huth <thuth@redhat.com>
Message-ID: <20240219061731.232570-1-npiggin@gmail.com>
[PMD: Split patch in 2: part 1/2]
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/exec/ram_addr.h | 9 +++++++++
 system/physmem.c        | 8 +++-----
 2 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
index 90676093f5..b060ea9176 100644
--- a/include/exec/ram_addr.h
+++ b/include/exec/ram_addr.h
@@ -25,6 +25,7 @@
 #include "sysemu/tcg.h"
 #include "exec/ramlist.h"
 #include "exec/ramblock.h"
+#include "exec/exec-all.h"
 
 extern uint64_t total_dirty_pages;
 
@@ -443,6 +444,14 @@ uint64_t cpu_physical_memory_set_dirty_lebitmap(unsigned long *bitmap,
 }
 #endif /* not _WIN32 */
 
+static inline void cpu_physical_memory_dirty_bits_cleared(ram_addr_t start,
+                                                          ram_addr_t length)
+{
+    if (tcg_enabled()) {
+        tlb_reset_dirty_range_all(start, length);
+    }
+
+}
 bool cpu_physical_memory_test_and_clear_dirty(ram_addr_t start,
                                               ram_addr_t length,
                                               unsigned client);
diff --git a/system/physmem.c b/system/physmem.c
index 5441480ff0..a4fe3d2bf8 100644
--- a/system/physmem.c
+++ b/system/physmem.c
@@ -881,8 +881,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 +929,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);
 
-- 
2.41.0

[PATCH-for-9.0 v2 3/3] physmem: Fix migration dirty bitmap coherency with TCG memory access

[Philippe Mathieu-Daudé]

From: Nicholas Piggin <npiggin@gmail.com>

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 calling cpu_physical_memory_dirty_bits_cleared() when
dirty bits have been cleared.

Fixes: aa8dc044772 ("migration: synchronize memory bitmap 64bits at a time")
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
Tested-by: Thomas Huth <thuth@redhat.com>
Message-ID: <20240219061731.232570-1-npiggin@gmail.com>
[PMD: Split patch in 2: part 2/2, slightly adapt description]
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/exec/ram_addr.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
index b060ea9176..de45ba7bc9 100644
--- a/include/exec/ram_addr.h
+++ b/include/exec/ram_addr.h
@@ -513,6 +513,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) {
             /*
-- 
2.41.0

Re: [PATCH-for-9.0 v2 0/3] system/physmem: Fix migration dirty bitmap coherency with TCG memory access

[Peter Xu]

On Tue, Mar 12, 2024 at 09:14:55PM +0100, Philippe Mathieu-Daudé wrote:
> Respin of Nicholas patch, without introducing
> tcg_cpu_physical_memory_dirty_bits_cleared(),
> and split in more digestible parts.
> 
> Nicholas Piggin (2):
>   physmem: Factor cpu_physical_memory_dirty_bits_cleared() out
>   physmem: Fix migration dirty bitmap coherency with TCG memory access
> 
> Philippe Mathieu-Daudé (1):
>   physmem: Expose tlb_reset_dirty_range_all()
> 
>  include/exec/exec-all.h |  1 +
>  include/exec/ram_addr.h | 12 ++++++++++++
>  system/physmem.c        | 10 ++++------
>  3 files changed, 17 insertions(+), 6 deletions(-)

Yes agree a better split than the single patch.  Tentatively queued while
waiting for any comments.

-- 
Peter Xu

Re: [PATCH-for-9.0 v2 0/3] system/physmem: Fix migration dirty bitmap coherency with TCG memory access

[Nicholas Piggin]

On Wed Mar 13, 2024 at 7:09 AM AEST, Peter Xu wrote:
> On Tue, Mar 12, 2024 at 09:14:55PM +0100, Philippe Mathieu-Daudé wrote:
> > Respin of Nicholas patch, without introducing
> > tcg_cpu_physical_memory_dirty_bits_cleared(),
> > and split in more digestible parts.
> > 
> > Nicholas Piggin (2):
> >   physmem: Factor cpu_physical_memory_dirty_bits_cleared() out
> >   physmem: Fix migration dirty bitmap coherency with TCG memory access
> > 
> > Philippe Mathieu-Daudé (1):
> >   physmem: Expose tlb_reset_dirty_range_all()
> > 
> >  include/exec/exec-all.h |  1 +
> >  include/exec/ram_addr.h | 12 ++++++++++++
> >  system/physmem.c        | 10 ++++------
> >  3 files changed, 17 insertions(+), 6 deletions(-)
>
> Yes agree a better split than the single patch.  Tentatively queued while
> waiting for any comments.

I've run into several other possible races / lost dirty tracking
when stressing this stuff, but this one was the easiest to hit and
most obvious and simple fix, so I think it's still good to go.

Also have a qtest test case that can reproduce this one so I'll
send that after this is merged. It's really just the migration
test case with value verification added in.

Thanks,
Nick

Re: [PATCH-for-9.0 v2 0/3] system/physmem: Fix migration dirty bitmap coherency with TCG memory access

[Richard Henderson]

On 3/12/24 10:14, Philippe Mathieu-Daudé wrote:
> Nicholas Piggin (2):
>    physmem: Factor cpu_physical_memory_dirty_bits_cleared() out
>    physmem: Fix migration dirty bitmap coherency with TCG memory access
> 
> Philippe Mathieu-Daudé (1):
>    physmem: Expose tlb_reset_dirty_range_all()

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~