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

["Herongguang (Stephen)" <herongguang.he@huawei.com>]

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.
>
>> 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();
>
Tested-by: He Rongguang <herongguang.he@huawei.com>

> .
>