Re: [Qemu-devel] [RFC/PATCH] migration: SMRAM dirty bitmap not fetched from kvm-kmod and not send to destination

[Paolo Bonzini]

On 22/09/2016 15:16, Herongguang (Stephen) wrote:
> I have some concern:
> 1. For example, vhost does not know about as_id, I wonder if guests in
> SMM can operate disk or ether card, as in
> that case vhost would not logging dirty pages correctly, without knowing
> as_id.

In the end memory is logged by ram_addr_t, not by address space.  So if
vhost_sync_dirty_bitmap is called on the right region everything works.

Guests in SMM can operate on storage devices, but storage devices cannot
write to 0xA0000-0xBFFFF so that's safe.

> 2. If a memory region is disabled/enabled/disabled frequently, since
> disabled memory regions would be removed
> from memory slots in kvm-kmod, dirty pages would be discarded in
> kvm-kmod and qemu when disabled, thus missing.
> Is my assumption correct?

As you found, this is handled by kvm_set_phys_mem:

        if (mem->flags & KVM_MEM_LOG_DIRTY_PAGES) {
            kvm_physical_sync_dirty_bitmap(kml, section);
        }

> 3. I agree your opinion that the right solution is to get dirty-page
> information for all memory region from
> kvm-kmod. But I found it’s somewhat hard to implement since
> kvm_log_sync() expects a MemoryRegionSection*
> parameter. Do you have good idea?

You're right, memory_region_sync_dirty_bitmap handles this but it's 
inefficient.

So your patch is correct, but it breaks for !x86 as you probably know.  
We need to look at the address spaces which have a listener that 
implements log_sync.

> As to all the ram memory regions, I think they are all in the
> ram_list.blocks, so there is no need to create
> a notifier, is this correct?

Yes, makes sense.

The patch here is a bit of a hack but is efficient.  It looks at
each address space just once and calls log_sync just once per
FlatRange.

Paolo

diff --git a/include/exec/memory.h b/include/exec/memory.h
index 3e4d416..23b086d 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -1154,12 +1154,11 @@ MemoryRegionSection memory_region_find(MemoryRegion *mr,
                                        hwaddr addr, uint64_t size);
 
 /**
- * address_space_sync_dirty_bitmap: synchronize the dirty log for all memory
+ * memory_global_dirty_log_sync: synchronize the dirty log for all memory
  *
- * Synchronizes the dirty page log for an entire address space.
- * @as: the address space that contains the memory being synchronized
+ * Synchronizes the dirty page log for all address spaces.
  */
-void address_space_sync_dirty_bitmap(AddressSpace *as);
+void memory_global_dirty_log_sync(void);
 
 /**
  * memory_region_transaction_begin: Start a transaction.
diff --git a/memory.c b/memory.c
index 1a1baf5..6cac674 100644
--- a/memory.c
+++ b/memory.c
@@ -158,15 +158,11 @@ static bool memory_listener_match(MemoryListener 
*listener,
 
 /* No need to ref/unref .mr, the FlatRange keeps it alive.  */
 #define MEMORY_LISTENER_UPDATE_REGION(fr, as, dir, callback, _args...)  \
-    MEMORY_LISTENER_CALL(callback, dir, (&(MemoryRegionSection) {       \
-        .mr = (fr)->mr,                                                 \
-        .address_space = (as),                                          \
-        .offset_within_region = (fr)->offset_in_region,                 \
-        .size = (fr)->addr.size,                                        \
-        .offset_within_address_space = int128_get64((fr)->addr.start),  \
-        .readonly = (fr)->readonly,                                     \
-              }), ##_args)
-
+    do {                                                                \
+        MemoryRegionSection mrs = section_from_flat_range(fr, as);      \
+        MEMORY_LISTENER_CALL(callback, dir, &mrs, ##_args);             \
+    } while(0)
+
 struct CoalescedMemoryRange {
     AddrRange addr;
     QTAILQ_ENTRY(CoalescedMemoryRange) link;
@@ -245,6 +241,19 @@ typedef struct AddressSpaceOps AddressSpaceOps;
 #define FOR_EACH_FLAT_RANGE(var, view)          \
     for (var = (view)->ranges; var < (view)->ranges + (view)->nr; ++var)
 
+static inline MemoryRegionSection
+section_from_flat_range(FlatRange *fr, AddressSpace *as)
+{
+    return (MemoryRegionSection) {
+        .mr = fr->mr,
+        .address_space = as,
+        .offset_within_region = fr->offset_in_region,
+        .size = fr->addr.size,
+        .offset_within_address_space = int128_get64(fr->addr.start),
+        .readonly = fr->readonly,
+    };
+}
+
 static bool flatrange_equal(FlatRange *a, FlatRange *b)
 {
     return a->mr == b->mr
@@ -2124,16 +2133,25 @@ bool memory_region_present(MemoryRegion *container, 
hwaddr addr)
     return mr && mr != container;
 }
 
-void address_space_sync_dirty_bitmap(AddressSpace *as)
+void memory_global_dirty_log_sync(void)
 {
+    MemoryListener *listener;
+    AddressSpace *as;
     FlatView *view;
     FlatRange *fr;
 
-    view = address_space_get_flatview(as);
-    FOR_EACH_FLAT_RANGE(fr, view) {
-        MEMORY_LISTENER_UPDATE_REGION(fr, as, Forward, log_sync);
+    QTAILQ_FOREACH(listener, &memory_listeners, link) {
+        if (!listener->log_sync || !listener->address_space_filter) {
+            continue;
+        }
+        as = listener->address_space_filter;
+        view = address_space_get_flatview(as);
+        FOR_EACH_FLAT_RANGE(fr, view) {
+            MemoryRegionSection mrs = section_from_flat_range(fr, as);
+            listener->log_sync(listener, &mrs);
+        }
+        flatview_unref(view);
     }
-    flatview_unref(view);
 }
 
 void memory_global_dirty_log_start(void)
diff --git a/migration/ram.c b/migration/ram.c
index a6e1c63..e33ff5e 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -626,7 +626,7 @@ static void migration_bitmap_sync(void)
     }
 
     trace_migration_bitmap_sync_start();
-    address_space_sync_dirty_bitmap(&address_space_memory);
+    memory_global_dirty_log_sync();
 
     qemu_mutex_lock(&migration_bitmap_mutex);
     rcu_read_lock();