[Qemu-devel] Re: [Xen-devel] [PATCH 2/2] xen: Introduce VGA sync dirty bitmap support

[Stefano Stabellini]

On Tue, 11 Jan 2011, address@hidden wrote:
> From: Anthony PERARD <address@hidden>
> 
> This patch introduces phys memory client for Xen.
> 
> Only sync dirty_bitmap and set_memory are actually implemented.
> migration_log will stay empty for the moment.
> 
> Xen can only log one range for bit change, so only the range in the
> first call will be synced.

The patch looks much better than I expected, one of the benefits of
reusing the ramblock architecture!

 
> +static XenPhysmap *link_exist(target_phys_addr_t start_addr)
> +{
> +    XenPhysmap *physmap = NULL;
> +
> +    start_addr = TARGET_PAGE_ALIGN(start_addr);
> +    QLIST_FOREACH(physmap, &xen_physmap, list) {
> +        if (physmap->size > 0 && physmap->start_addr == start_addr) {
> +            return physmap;
> +        }
> +    }
> +    return NULL;
> +}
> +
> +#if CONFIG_XEN_CTRL_INTERFACE_VERSION >= 340
> +static int already_physmapped(target_phys_addr_t phys_offset)
> +{
> +    XenPhysmap *physmap = NULL;
> +
> +    phys_offset = TARGET_PAGE_ALIGN(phys_offset);
> +    QLIST_FOREACH(physmap, &xen_physmap, list) {
> +        if (physmap->size > 0 && physmap->phys_offset <= phys_offset &&
> +            phys_offset <= (physmap->phys_offset + physmap->size)) {
> +            return 1;
> +        }
> +    }
> +    return 0;
> +}
> +

why are you searching for an address within a range in
already_physmapped while you are just looking for a simple match in
link_exist? Shouldn't it be the same in both?


> +static int xen_sync_dirty_bitmap(target_phys_addr_t start_addr,
> +                                 ram_addr_t size)
> +{
> +    target_phys_addr_t npages = size >> TARGET_PAGE_BITS;
> +    target_phys_addr_t vram_offset = 0;
> +    const int width = sizeof(unsigned long) * 8;
> +    unsigned long bitmap[(npages + width - 1) / width];
> +    int rc, i, j;
> +    XenPhysmap *physmap = NULL;
> +
> +    physmap = link_exist(start_addr);
> +    if (physmap) {
> +        vram_offset = physmap->phys_offset;
> +    } else {
> +        vram_offset = start_addr;
> +    }

link_exist is always supposed to return a value here anyway, right?
With the current code is not possible to get here without a mapping, I
think.
However if you are trying to make xen_sync_dirty_bitmap more generic
you also need to handle the case in which start_addr is contained within
an already mapped range.