QEMU-Devel Archive on lore.kernel.org
 help / color / Atom feed
From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
To: Peter Xu <peterx@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>,
	qemu-devel@nongnu.org, "Michael S . Tsirkin" <mst@redhat.com>
Subject: Re: [PATCH RFC 5/9] KVM: Provide helper to get kvm dirty log
Date: Wed, 25 Mar 2020 17:52:20 +0000
Message-ID: <20200325175220.GD2635@work-vm> (raw)
In-Reply-To: <20200205141749.378044-6-peterx@redhat.com>

* Peter Xu (peterx@redhat.com) 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 <peterx@redhat.com>
> ---
>  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 / dgilbert@redhat.com / Manchester, UK



  reply index

Thread overview: 31+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-02-05 14:17 [PATCH RFC 0/9] KVM: Dirty ring support (QEMU part) Peter Xu
2020-02-05 14:17 ` [PATCH RFC 1/9] KVM: Fixup kvm_log_clear_one_slot() ioctl return check Peter Xu
2020-03-25 16:34   ` Dr. David Alan Gilbert
2020-03-25 17:43   ` Peter Xu
2020-02-05 14:17 ` [PATCH RFC 2/9] linux-headers: Update Peter Xu
2020-02-05 14:17 ` [PATCH RFC 3/9] memory: Introduce log_sync_global() to memory listener Peter Xu
2020-03-25 16:52   ` Dr. David Alan Gilbert
2020-03-25 17:02     ` Peter Xu
2020-02-05 14:17 ` [PATCH RFC 4/9] KVM: Create the KVMSlot dirty bitmap on flag changes Peter Xu
2020-03-25 17:44   ` Dr. David Alan Gilbert
2020-02-05 14:17 ` [PATCH RFC 5/9] KVM: Provide helper to get kvm dirty log Peter Xu
2020-03-25 17:52   ` Dr. David Alan Gilbert [this message]
2020-03-25 18:15     ` Peter Xu
2020-02-05 14:17 ` [PATCH RFC 6/9] KVM: Provide helper to sync dirty bitmap from slot to ramblock Peter Xu
2020-03-25 18:47   ` Dr. David Alan Gilbert
2020-02-05 14:17 ` [PATCH RFC 7/9] KVM: Cache kvm slot dirty bitmap size Peter Xu
2020-03-25 18:49   ` Dr. David Alan Gilbert
2020-02-05 14:17 ` [PATCH RFC 8/9] KVM: Add dirty-ring-size property Peter Xu
2020-03-25 20:00   ` Dr. David Alan Gilbert
2020-03-25 20:42     ` Peter Xu
2020-03-26 13:41       ` Dr. David Alan Gilbert
2020-03-26 16:03         ` Peter Xu
2020-03-25 20:14   ` Dr. David Alan Gilbert
2020-03-25 20:48     ` Peter Xu
2020-02-05 14:17 ` [PATCH RFC 9/9] KVM: Dirty ring support Peter Xu
2020-03-25 20:41   ` Dr. David Alan Gilbert
2020-03-25 21:32     ` Peter Xu
2020-03-26 14:14       ` Dr. David Alan Gilbert
2020-03-26 16:10         ` Peter Xu
2020-02-05 14:51 ` [PATCH RFC 0/9] KVM: Dirty ring support (QEMU part) no-reply
2020-03-03 17:32 ` Peter Xu

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200325175220.GD2635@work-vm \
    --to=dgilbert@redhat.com \
    --cc=mst@redhat.com \
    --cc=pbonzini@redhat.com \
    --cc=peterx@redhat.com \
    --cc=qemu-devel@nongnu.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

QEMU-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/qemu-devel/0 qemu-devel/git/0.git
	git clone --mirror https://lore.kernel.org/qemu-devel/1 qemu-devel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 qemu-devel qemu-devel/ https://lore.kernel.org/qemu-devel \
		qemu-devel@nongnu.org
	public-inbox-index qemu-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.nongnu.qemu-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git