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

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Tue, 19 Feb 2013, Alex Bligh wrote:
> 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?

Yeah, the code could be more readable. The point would be only to act on
errors.


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

I think that the best thing to do in this case is just to add a call to
xen_modified_memory if (rc != -ENODATA).


> >> @@ -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.
 
Considering that this change wasn't part of the original patch, it needs
to go into a different patch. Probably patch 4/5 would be a better fit,
given that this change is needed because
cpu_physical_memory_set_dirty_range doesn't exist.

Finally I think that adding a call to xen_modified_memory from
cpu_physical_memory_set_dirty would simplify things. The size would be
PAGE_SIZE.

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