From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1L2opP-0005sa-R1 for qemu-devel@nongnu.org; Wed, 19 Nov 2008 10:18:51 -0500 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1L2opO-0005ro-Tm for qemu-devel@nongnu.org; Wed, 19 Nov 2008 10:18:51 -0500 Received: from [199.232.76.173] (port=41408 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1L2opO-0005rZ-O4 for qemu-devel@nongnu.org; Wed, 19 Nov 2008 10:18:50 -0500 Received: from qb-out-1314.google.com ([72.14.204.174]:42024) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1L2opN-0006Kl-V5 for qemu-devel@nongnu.org; Wed, 19 Nov 2008 10:18:50 -0500 Received: by qb-out-1314.google.com with SMTP id e19so3514602qba.8 for ; Wed, 19 Nov 2008 07:18:48 -0800 (PST) Message-ID: <49242E53.6000802@codemonkey.ws> Date: Wed, 19 Nov 2008 09:18:43 -0600 From: Anthony Liguori MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH 5/6] Introduce kvm logging interface. References: <1227108377-8442-1-git-send-email-glommer@redhat.com> <1227108377-8442-2-git-send-email-glommer@redhat.com> <1227108377-8442-3-git-send-email-glommer@redhat.com> <1227108377-8442-4-git-send-email-glommer@redhat.com> <1227108377-8442-5-git-send-email-glommer@redhat.com> <1227108377-8442-6-git-send-email-glommer@redhat.com> In-Reply-To: <1227108377-8442-6-git-send-email-glommer@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Reply-To: qemu-devel@nongnu.org List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-devel@nongnu.org Glauber Costa wrote: > Introduce functions to stop and start logging of memory regions. > We select region based on its start address. > > Signed-off-by: Glauber Costa > --- > kvm-all.c | 153 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > kvm.h | 5 ++ > 2 files changed, 158 insertions(+), 0 deletions(-) > > diff --git a/kvm-all.c b/kvm-all.c > index 9700e50..c522205 100644 > --- a/kvm-all.c > +++ b/kvm-all.c > @@ -32,8 +32,13 @@ > do { } while (0) > #endif > > +#define warning(fmt, ...) \ > + do { dprintf("%s:%d" fmt, __func__, __LINE__, ## __VA_ARGS__); } while (0) > I don't think this macro really serves a purpose. You can just add "%s:%d: " to the beginning of dprintf() if you want. Also note that it gives goofy output right now because of the missing space after %d. > +/* > + * dirty pages logging control > + */ > +static int kvm_dirty_pages_log_change(KVMSlot *mem, > + unsigned flags, > + unsigned mask) > +{ > + int r = -1; > + KVMState *s = kvm_state; > + > + flags = (mem->flags & ~mask) | flags; > + if (flags == mem->flags) > + return 0; > + > + mem->flags = flags; > + > + r = kvm_vm_ioctl(s, KVM_SET_USER_MEMORY_REGION, mem); > + if (r == -1) > + fprintf(stderr, "%s: %m\n", __FUNCTION__); > + > + return r; > +} > + > +int kvm_log_start(target_phys_addr_t phys_addr, target_phys_addr_t end_addr) > +{ > + KVMState *s = kvm_state; > + KVMSlot *mem = kvm_lookup_slot(s, phys_addr); > + > + /* > + * We don't need to do dirty tracking of alias slots. If we track the > + * memory the alias is pointing to, it should be enough > + */ > + if (kvm_arch_is_alias_slot(phys_addr)) > + return 0; > + > + if (mem == NULL) { > + warning("invalid parameters %llx-%llx\n", phys_addr, end_addr); > + return -EINVAL; > + } > + > + /* Already logging, no need to issue ioctl */ > + if (mem->flags & KVM_MEM_LOG_DIRTY_PAGES) > + return 0; > + > + dprintf("slot %d: enable logging (phys %llx, uaddr %llx)\n", > + mem->slot, mem->guest_phys_addr, mem->userspace_addr); > + > + return kvm_dirty_pages_log_change(mem, > + KVM_MEM_LOG_DIRTY_PAGES, > + KVM_MEM_LOG_DIRTY_PAGES); > +} > + > +int kvm_log_stop(target_phys_addr_t phys_addr, target_phys_addr_t end_addr) > +{ > + > + KVMState *s = kvm_state; > + KVMSlot *mem = kvm_lookup_slot(s, phys_addr); > + > + if (kvm_arch_is_alias_slot(phys_addr)) > + return 0; > + > + if (mem == NULL) { > + warning("invalid parameters %llx - %llx \n", phys_addr, end_addr); > + return -EINVAL; > + } > + > + /* Not logging, no need to issue ioctl */ > + if (!(mem->flags & KVM_MEM_LOG_DIRTY_PAGES)) > + return 0; > + > + dprintf("slot %d: disabling logging\n", mem->slot); > + return kvm_dirty_pages_log_change(mem, > + 0, > + KVM_MEM_LOG_DIRTY_PAGES); > +} > Note that start and stop are identical except for a different printf(). The call a common helper function. Why not fold everything into kvm_dirty_pages_log_change() and make that the public interface (kvm_set_log). > +static inline int lookup_bitmap_phys(unsigned long *dirty, unsigned nr) > +{ > + unsigned word = nr / (sizeof(*dirty) * 8); > + unsigned bit = nr % (sizeof(*dirty) * 8); > + int ret; > + > + ret = (dirty[word] >> bit) & 1; > + return ret; > +} > + > +/** > + * kvm_physical_sync_dirty_bitmap - Grab dirty bitmap from kernel space > + * If a phys_offset parameter is given, this function updates qemu's dirty > + * bitmap using cpu_physical_memory_set_dirty(). This means all bits are set > + * to dirty. > + * > + * @start_add: start of logged region. This is what we use to search the memslot > + * @end_addr: end of logged region. Only matters if we are updating qemu dirty bitmap. > + * @phys_offset: the phys_offset we want to use for qemu dirty bitmap update. Passing > + * NULL makes the update not happen. In this case, we only grab the bitmap > + * from kernel. > + */ > +void kvm_physical_sync_dirty_bitmap(target_phys_addr_t start_addr, target_phys_addr_t end_addr, > + ram_addr_t phys_offset) > This interface is weird and broken. If we wanted to use this for live migration, we would end up passing phys_offset=0 but that has a special meaning here. But why are we passing phys_offset at all? Why can't we do the lookup here? Regards, Anthony Liguori