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