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