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

[Anthony Liguori <anthony@codemonkey.ws>]

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 <glommer@redhat.com>
> ---
>  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