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

[Thomas Huth <thuth@redhat.com>]

  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 <npiggin@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);
>