Re: [Qemu-devel] [PATCH] Introduce kvm logging interface.

[Anthony Liguori]

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 <address@hidden>
> ---
>  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;
>