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

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

On Wed, 12 Jan 2011, Stefano Stabellini wrote:

> On Tue, 11 Jan 2011, anthony.perard@xxxxxxxxxx wrote:
> > From: Anthony PERARD <anthony.perard@xxxxxxxxxx>
> >
> > 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?

For link_exist, I want to be sure to remove the same physmapping adds
previously, I should check also the size.

> > +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.

Actually, before I sent the patch, the second case was an error, and it
worked well. Now, sync_dirty_bitmap is done for two regions, for the
physmapped memory and for the isa memory. :(

But I will add the case when the start_addr is inside the region.

-- 
Anthony PERARD

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel