Re: [Xen-devel] [PATCHv4 5/5] xen: Set the vram dirty when an error occur.

[Alex Bligh <alex@xxxxxxxxxxx>]

Stefano,

--On 19 February 2013 18:05:46 +0000 Stefano Stabellini 
<stefano.stabellini@xxxxxxxxxxxxx> wrote:
> On Tue, 19 Feb 2013, Alex Bligh wrote:
> > If the call to xc_hvm_track_dirty_vram() fails, then we set dirtybit on
> > all the video ram. This case happens during migration.
This was the patch I had most difficulty with, frankly because I had
little idea what it was doing. Education welcome!

> > Backport of 8aba7dc02d5660df7e7d8651304b3079908358be
> >
> > Signed-off-by: Alex Bligh <alex@xxxxxxxxxxx>
> > ---
> >  xen-all.c |   20 ++++++++++++++++++--
> >  1 files changed, 18 insertions(+), 2 deletions(-)
> >
> > diff --git a/xen-all.c b/xen-all.c
> > index 121289d..dbd759c 100644
> > --- a/xen-all.c
> > +++ b/xen-all.c
> > @@ -470,7 +470,21 @@ static int xen_sync_dirty_bitmap(XenIOState *state,
> >      rc = xc_hvm_track_dirty_vram(xen_xc, xen_domid,
> >                                   start_addr >> TARGET_PAGE_BITS, npages,
> >                                   bitmap);
> > -    if (rc) {
> > +    if (rc < 0) {
> > +        if (rc != -ENODATA) {
> > +            ram_addr_t addr, end;
> > +
> > +            xen_modified_memory(start_addr, size);
> > +
> > +            end = TARGET_PAGE_ALIGN(start_addr + size);
> > +            for (addr = start_addr & TARGET_PAGE_MASK; addr < end; addr
> > += TARGET_PAGE_SIZE) { +
> > cpu_physical_memory_set_dirty(addr);
> > +            }
> > +
> > +            DPRINTF("xen: track_dirty_vram failed (0x" TARGET_FMT_plx
> > +                    ", 0x" TARGET_FMT_plx "): %s\n",
> > +                    start_addr, start_addr + size, strerror(-rc));
> > +        }
> >          return rc;
> >      }
> 8aba7dc02d5660df7e7d8651304b3079908358be only adds a simple call to
> xen_modified_memory if rc != ENODATA.
> Where does the rest of the code you are adding comes from?
Applying the patch unchanged is not easy as there are a pile of
conflicting items. I was taking a conservative approach partly
as I didn't fully understand what types of addresses could be
safely used where. My approach was to make this function as similar
as possible to xen 4.3 immediately after the patch, which is here:

http://git.qemu.org/?p=qemu.git;a=blob;f=xen-all.c;h=e6308be23adb7198c144883eb886fb6edb5fe09f;hb=8aba7dc02d5660df7e7d8651304b3079908358be

There were some oddnesses there, such as:

508     if (rc < 0) {
509         if (rc != -ENODATA) {
510             memory_region_set_dirty(framebuffer, 0, size);
511             DPRINTF("xen: track_dirty_vram failed (0x" TARGET_FMT_plx
512                     ", 0x" TARGET_FMT_plx "): %s\n",
513                     start_addr, start_addr + size, strerror(-rc));
514         }
515         return;
516     }

What is the point of the outer check on rc?

I think you are asking why I didn't just call memory_region_set_dirty.
memory_region_set_dirty in 4.2 (as opposed to 4.3) takes:
void memory_region_set_dirty(MemoryRegion *mr, target_phys_addr_t addr);
and the 3 parameter version is unavailable, so what I did was (I hope)
the equivalent of what the 3 parameter version would have done, going
through each page.

I could have modified memory_region_set_dirty to cope with multiple pages
but that seemed like a more intrusive change.

> > @@ -479,7 +493,9 @@ static int xen_sync_dirty_bitmap(XenIOState *state,
> >          while (map != 0) {
> >              j = ffsl(map) - 1;
> >              map &= ~(1ul << j);
> > -            cpu_physical_memory_set_dirty(vram_offset + (i * width + j)
> > * TARGET_PAGE_SIZE); +            target_phys_addr_t todirty =
> > vram_offset + (i * width + j) * TARGET_PAGE_SIZE; +
> > xen_modified_memory(todirty, TARGET_PAGE_SIZE);
> > +            cpu_physical_memory_set_dirty(todirty);
> >          };
> >      }
> where does this chuck come from?
> Wouldn't it make more sense to add a call to xen_modified_memory from
> cpu_physical_memory_set_dirty?
Again, I was trying to emulate what the 3 parameter
memory_region_set_dirty() does. I believe cpu_physical_memory_set_dirty has
other callers, and was unsure whether adding a call to xen_modified_memory
would be safe in there.

--
Alex Bligh

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel