From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1KzaMk-0005lI-8n for qemu-devel@nongnu.org; Mon, 10 Nov 2008 12:15:54 -0500 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1KzaMh-0005jl-PK for qemu-devel@nongnu.org; Mon, 10 Nov 2008 12:15:53 -0500 Received: from [199.232.76.173] (port=52291 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1KzaMh-0005jd-97 for qemu-devel@nongnu.org; Mon, 10 Nov 2008 12:15:51 -0500 Received: from el-out-1112.google.com ([209.85.162.178]:41172) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1KzaMf-00025X-KE for qemu-devel@nongnu.org; Mon, 10 Nov 2008 12:15:51 -0500 Received: by el-out-1112.google.com with SMTP id s27so1223272ele.19 for ; Mon, 10 Nov 2008 09:15:43 -0800 (PST) Message-ID: <49186C3B.4070909@codemonkey.ws> Date: Mon, 10 Nov 2008 11:15:39 -0600 From: Anthony Liguori MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH] Introduce kvm logging interface. References: <1226342253-8887-1-git-send-email-glommer@redhat.com> <1226342253-8887-2-git-send-email-glommer@redhat.com> <1226342253-8887-3-git-send-email-glommer@redhat.com> <1226342253-8887-4-git-send-email-glommer@redhat.com> In-Reply-To: <1226342253-8887-4-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 | 140 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > kvm.h | 5 ++ > 2 files changed, 145 insertions(+), 0 deletions(-) > > diff --git a/kvm-all.c b/kvm-all.c > index 6d50609..c2c253f 100644 > --- a/kvm-all.c > +++ b/kvm-all.c > @@ -31,8 +31,15 @@ > do { } while (0) > #endif > > +#define warning(fmt, ...) \ > + do { fprintf(stderr, "%s:%d" fmt, __func__, __LINE__, ## __VA_ARGS__); } while (0) > I'd rather this be a dprintf(). Error paths should not be chatty for normal users because then a malicious can potentially start a crap-flood. > typedef struct kvm_userspace_memory_region KVMSlot; > > +#define kvm_uaddr(addr) ((addr) + (ram_addr_t)phys_ram_base) > Should always be a static inline when possible, but I'm suspicious that this function should even exist. > +typedef struct kvm_dirty_log KVMDirty; > KVMDirtyLog? > int kvm_allowed = 0; > > struct KVMState > @@ -71,6 +78,24 @@ static KVMSlot *kvm_lookup_slot(KVMState *s, target_phys_addr_t start_addr) > return NULL; > } > > +/* find the slot correspondence using userspace_addr as a key */ > +static KVMSlot *kvm_lookup_slot_uaddr(KVMState *s, ram_addr_t addr) > +{ > + int i; > + > + uint64_t uaddr = (uint64_t)kvm_uaddr(addr); > Since this is the only use of kvm_uaddr, you could just fold the #define into here. > +void kvm_physical_sync_dirty_bitmap(ram_addr_t start_addr, ram_addr_t end_addr) > +{ > + KVMState *s = kvm_state; > + KVMSlot *mem = kvm_lookup_slot_uaddr(s, start_addr); > + KVMDirty d; > + unsigned long alloc_size = mem->memory_size >> TARGET_PAGE_BITS / sizeof(d.dirty_bitmap); > + ram_addr_t addr; > + > + dprintf("sync addr: %lx %llx %llx\n", start_addr, mem->guest_phys_addr, kvm_uaddr(start_addr)); > + if (mem == NULL) { > + fprintf(stderr, "BUG: %s: invalid parameters\n", __func__); > + return; > + } > + > + d.dirty_bitmap = qemu_mallocz(alloc_size); > + > + if (d.dirty_bitmap == NULL) { > + warning("Could not allocate dirty bitmap\n"); > + return; > + } > + > + d.slot = mem->slot; > + dprintf("slot %d, phys_addr %llx, uaddr: %llx\n", > + d.slot, mem->guest_phys_addr, mem->userspace_addr); > + > + if (kvm_vm_ioctl(s, KVM_GET_DIRTY_LOG, &d) == -1) { > + warning("ioctl failed %d\n", errno); > + goto out; > + } > + > + for (addr = start_addr; addr < end_addr; addr += TARGET_PAGE_SIZE){ > + unsigned nr; > + nr = (uint32_t)((uint64_t)kvm_uaddr(addr) - mem->userspace_addr) >> TARGET_PAGE_BITS; > + phys_ram_dirty[addr >> TARGET_PAGE_BITS] |= lookup_bitmap_phys((unsigned long *)d.dirty_bitmap, nr); > This is only setting the VGA_DIRTY_FLAG and really only by happy coincidence. If that was on purpose, then shame on you ;-) You need to set all bits in order for live migration to work. You should use cpu_physical_memory_set_dirty(). I also think you could simplify things by folding lookup_bitmap_phys() into this function but that's not a requirement for merging. > + } > +out: > + qemu_free(d.dirty_bitmap); > +} > + > int kvm_init(int smp_cpus) > { > KVMState *s; > diff --git a/kvm.h b/kvm.h > index 37102b4..39e9048 100644 > --- a/kvm.h > +++ b/kvm.h > @@ -38,6 +38,11 @@ void kvm_set_phys_mem(target_phys_addr_t start_addr, > ram_addr_t size, > ram_addr_t phys_offset); > > +int kvm_physical_memory_get_dirty(ram_addr_t addr); > This function no longer exists. If you fix these issues, then I think it's ready for merging. Regards, Anthony Liguori > +void kvm_physical_sync_dirty_bitmap(ram_addr_t start_addr, ram_addr_t end_addr); > + > +int kvm_log_start(target_phys_addr_t phys_addr, target_phys_addr_t len); > +int kvm_log_stop(target_phys_addr_t phys_addr, target_phys_addr_t len); > /* internal API */ > > struct KVMState; >