Re: [PATCH RFC 5/9] KVM: Provide helper to get kvm dirty log

[Dr. David Alan Gilbert]

* Peter Xu (address@hidden) wrote:
> Provide a helper kvm_slot_get_dirty_log() to make the function
> kvm_physical_sync_dirty_bitmap() clearer.  We can even cache the as_id
> into KVMSlot when it is created, so that we don't even need to pass it
> down every time.
> 
> Since at it, remove return value of kvm_physical_sync_dirty_bitmap()
> because it should never fail.
> 
> Signed-off-by: Peter Xu <address@hidden>
> ---
>  accel/kvm/kvm-all.c      | 39 +++++++++++++++++++--------------------
>  include/sysemu/kvm_int.h |  2 ++
>  2 files changed, 21 insertions(+), 20 deletions(-)
> 
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index bb635c775f..608216fd53 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -553,6 +553,18 @@ static void kvm_slot_init_dirty_bitmap(KVMSlot *mem)
>      mem->dirty_bmap = g_malloc0(bitmap_size);
>  }
>  
> +/* Sync dirty bitmap from kernel to KVMSlot.dirty_bmap */
> +static void kvm_slot_get_dirty_log(KVMState *s, KVMSlot *slot)
> +{
> +    struct kvm_dirty_log d = {};
> +    int ret;
> +
> +    d.dirty_bitmap = slot->dirty_bmap;
> +    d.slot = slot->slot | (slot->as_id << 16);
> +    ret = kvm_vm_ioctl(s, KVM_GET_DIRTY_LOG, &d);
> +    assert(ret != -1);

As discussed on irc, that -1 check seems odd given your previous check
but there seems to be some history as to why it was still there.  Hmm.

It also seems very trusting that it can never possibly fail!

Dave

> +}
> +
>  /**
>   * kvm_physical_sync_dirty_bitmap - Sync dirty bitmap from kernel space
>   *
> @@ -564,15 +576,13 @@ static void kvm_slot_init_dirty_bitmap(KVMSlot *mem)
>   * @kml: the KVM memory listener object
>   * @section: the memory section to sync the dirty bitmap with
>   */
> -static int kvm_physical_sync_dirty_bitmap(KVMMemoryListener *kml,
> -                                          MemoryRegionSection *section)
> +static void kvm_physical_sync_dirty_bitmap(KVMMemoryListener *kml,
> +                                           MemoryRegionSection *section)
>  {
>      KVMState *s = kvm_state;
> -    struct kvm_dirty_log d = {};
>      KVMSlot *mem;
>      hwaddr start_addr, size;
>      hwaddr slot_size, slot_offset = 0;
> -    int ret = 0;
>  
>      size = kvm_align_section(section, &start_addr);
>      while (size) {
> @@ -582,27 +592,19 @@ static int 
> kvm_physical_sync_dirty_bitmap(KVMMemoryListener *kml,
>          mem = kvm_lookup_matching_slot(kml, start_addr, slot_size);
>          if (!mem) {
>              /* We don't have a slot if we want to trap every access. */
> -            goto out;
> +            return;
>          }
>  
> -        d.dirty_bitmap = mem->dirty_bmap;
> -        d.slot = mem->slot | (kml->as_id << 16);
> -        if (kvm_vm_ioctl(s, KVM_GET_DIRTY_LOG, &d) == -1) {
> -            DPRINTF("ioctl failed %d\n", errno);
> -            ret = -1;
> -            goto out;
> -        }
> +        kvm_slot_get_dirty_log(s, mem);
>  
>          subsection.offset_within_region += slot_offset;
>          subsection.size = int128_make64(slot_size);
> -        kvm_get_dirty_pages_log_range(&subsection, d.dirty_bitmap);
> +        kvm_get_dirty_pages_log_range(&subsection, mem->dirty_bmap);
>  
>          slot_offset += slot_size;
>          start_addr += slot_size;
>          size -= slot_size;
>      }
> -out:
> -    return ret;
>  }
>  
>  /* Alignment requirement for KVM_CLEAR_DIRTY_LOG - 64 pages */
> @@ -1077,6 +1079,7 @@ static void kvm_set_phys_mem(KVMMemoryListener *kml,
>      do {
>          slot_size = MIN(kvm_max_slot_size, size);
>          mem = kvm_alloc_slot(kml);
> +        mem->as_id = kml->as_id;
>          mem->memory_size = slot_size;
>          mem->start_addr = start_addr;
>          mem->ram = ram;
> @@ -1119,14 +1122,10 @@ static void kvm_log_sync(MemoryListener *listener,
>                           MemoryRegionSection *section)
>  {
>      KVMMemoryListener *kml = container_of(listener, KVMMemoryListener, 
> listener);
> -    int r;
>  
>      kvm_slots_lock(kml);
> -    r = kvm_physical_sync_dirty_bitmap(kml, section);
> +    kvm_physical_sync_dirty_bitmap(kml, section);
>      kvm_slots_unlock(kml);
> -    if (r < 0) {
> -        abort();
> -    }
>  }
>  
>  static void kvm_log_clear(MemoryListener *listener,
> diff --git a/include/sysemu/kvm_int.h b/include/sysemu/kvm_int.h
> index ac2d1f8b56..4434e15ec7 100644
> --- a/include/sysemu/kvm_int.h
> +++ b/include/sysemu/kvm_int.h
> @@ -23,6 +23,8 @@ typedef struct KVMSlot
>      int old_flags;
>      /* Dirty bitmap cache for the slot */
>      unsigned long *dirty_bmap;
> +    /* Cache of the address space ID */
> +    int as_id;
>  } KVMSlot;
>  
>  typedef struct KVMMemoryListener {
> -- 
> 2.24.1
> 
> 
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK