qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [RFC/PATCH] migration: SMRAM dirty bitmap not fetched f


From: Herongguang (Stephen)
Subject: Re: [Qemu-devel] [RFC/PATCH] migration: SMRAM dirty bitmap not fetched from kvm-kmod and not send to destination
Date: Fri, 23 Sep 2016 16:51:14 +0800
User-agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:38.0) Gecko/20100101 Thunderbird/38.5.1



On 2016/9/23 15:17, Paolo Bonzini wrote:


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.
You are right, as vhost does not have a smram address space listener. So is this
a qemu's flaw that virtual devices can not write to SMRAM?


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();

Fine, your patch is simpler than I thought and functions right.

Reviewed-by: He Rongguang <address@hidden>

.





reply via email to

[Prev in Thread] Current Thread [Next in Thread]