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
|
Home