WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

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

To: Stefano Stabellini <Stefano.Stabellini@xxxxxxxxxxxxx>
Subject: Re: [Xen-devel] [PATCH 2/2] xen: Introduce VGA sync dirty bitmap support
From: Anthony PERARD <anthony.perard@xxxxxxxxxx>
Date: Wed, 12 Jan 2011 17:15:08 +0000 (GMT)
Cc: Xen Devel <xen-devel@xxxxxxxxxxxxxxxxxxx>, QEMU-devel <qemu-devel@xxxxxxxxxx>
Delivery-date: Wed, 12 Jan 2011 09:17:42 -0800
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
In-reply-to: <alpine.DEB.2.00.1101121108110.7277@kaball-desktop>
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
References: <1294760223-26151-1-git-send-email-anthony.perard@xxxxxxxxxx> <1294760223-26151-3-git-send-email-anthony.perard@xxxxxxxxxx> <alpine.DEB.2.00.1101121108110.7277@kaball-desktop>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
User-agent: Alpine 1.10 (DEB 962 2008-03-14)
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