Alex Bligh
2013-Feb-19 13:17 UTC
[PATCHv4 0/5] QEMU: Enabling live-migrate on HVM on qemu-xen device model in 4.2 - qemu patchset
This patch series consists of 2 parts: * 11 patches to libxl * 5 patches to QEMU The 11 patches to libxl are unchanged since version 3 of the patch, but have Acked-By lines reintroduced. The 5 patches to QEMU are unchanged since version 2 of the patch. These patches enable live-migrate on HVM using the upstream qemu-xen device model under Xen 4.2. Currently this is unimplemented. In the main they are backports of patches in xen-unstable, thought the QEMU side in particular needed some fiddling. I would suggest these patches should be included in 4.2.2. You can find these on Github at: https://github.com/abligh/qemu-upstream-4.2-testing-livemigrate Or on the master branch of this repository: https://github.com/abligh/qemu-upstream-4.2-testing-livemigrate.git Alex Bligh (5): QMP, Introduce xen-set-global-dirty-log command. xen: Introduce xen_modified_memory. exec: Introduce helper to set dirty flags. exec, memory: Call to xen_modified_memory. xen: Set the vram dirty when an error occur. exec.c | 49 +++++++++++++++++++++---------------------------- hw/xen.h | 1 + memory.c | 4 ++++ qapi-schema.json | 13 +++++++++++++ qmp-commands.hx | 24 ++++++++++++++++++++++++ xen-all.c | 47 +++++++++++++++++++++++++++++++++++++++++++++-- xen-stub.c | 9 +++++++++ 7 files changed, 117 insertions(+), 30 deletions(-) -- 1.7.4.1
Alex Bligh
2013-Feb-19 13:17 UTC
[PATCHv4 1/5] QMP, Introduce xen-set-global-dirty-log command.
This command is used during a migration of a guest under Xen. It calls cpu_physical_memory_set_dirty_tracking. Backport of 39f42439d0629d3921629dc4b38e68df8f2f7b83 Signed-off-by: Alex Bligh <alex@alex.org.uk> --- qapi-schema.json | 13 +++++++++++++ qmp-commands.hx | 24 ++++++++++++++++++++++++ xen-all.c | 6 ++++++ xen-stub.c | 5 +++++ 4 files changed, 48 insertions(+), 0 deletions(-) diff --git a/qapi-schema.json b/qapi-schema.json index a669e98..bb0d7c5 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -905,3 +905,16 @@ # Since: 1.1 ## { ''command'': ''xen-save-devices-state'', ''data'': {''filename'': ''str''} } + +## +# @xen-set-global-dirty-log +# +# Enable or disable the global dirty log mode. +# +# @enable: true to enable, false to disable. +# +# Returns: nothing +# +# Since: 1.3 +## +{ ''command'': ''xen-set-global-dirty-log'', ''data'': { ''enable'': ''bool'' } } diff --git a/qmp-commands.hx b/qmp-commands.hx index bf1df49..0de68df 100644 --- a/qmp-commands.hx +++ b/qmp-commands.hx @@ -472,6 +472,30 @@ Example: EQMP { + .name = "xen-set-global-dirty-log", + .args_type = "enable:b", + .mhandler.cmd_new = qmp_marshal_input_xen_set_global_dirty_log, + }, + +SQMP +xen-set-global-dirty-log +------- + +Enable or disable the global dirty log mode. + +Arguments: + +- "enable": Enable it or disable it. + +Example: + +-> { "execute": "xen-set-global-dirty-log", + "arguments": { "enable": true } } +<- { "return": {} } + +EQMP + + { .name = "migrate", .args_type = "detach:-d,blk:-b,inc:-i,uri:s", .params = "[-d] [-b] [-i] uri", diff --git a/xen-all.c b/xen-all.c index 3256509..6b4e511 100644 --- a/xen-all.c +++ b/xen-all.c @@ -12,6 +12,7 @@ #include "hw/pc.h" #include "hw/xen_common.h" #include "hw/xen_backend.h" +#include "qmp-commands.h" #include "range.h" #include "xen-mapcache.h" @@ -524,6 +525,11 @@ static CPUPhysMemoryClient xen_cpu_phys_memory_client = { .log_stop = xen_log_stop, }; +void qmp_xen_set_global_dirty_log(bool enable, Error **errp) +{ + cpu_physical_memory_set_dirty_tracking(!!enable); +} + /* VCPU Operations, MMIO, IO ring ... */ static void xen_reset_vcpu(void *opaque) diff --git a/xen-stub.c b/xen-stub.c index efe2ab5..25317ec 100644 --- a/xen-stub.c +++ b/xen-stub.c @@ -8,6 +8,7 @@ #include "qemu-common.h" #include "hw/xen.h" +#include "qmp-commands.h" void xenstore_store_pv_console_info(int i, CharDriverState *chr) { @@ -43,3 +44,7 @@ int xen_init(void) { return -ENOSYS; } + +void qmp_xen_set_global_dirty_log(bool enable, Error **errp) +{ +} -- 1.7.4.1
This function is to be used during live migration. Every write access to the guest memory should call this funcion so the Xen tools knows which pages are dirty. Backport of 910b38e4dc4c37683c8b821e75a7f4cf095e4b21 Signed-off-by: Alex Bligh <alex@alex.org.uk> --- hw/xen.h | 1 + xen-all.c | 21 +++++++++++++++++++++ xen-stub.c | 4 ++++ 3 files changed, 26 insertions(+), 0 deletions(-) diff --git a/hw/xen.h b/hw/xen.h index 2162111..359a275 100644 --- a/hw/xen.h +++ b/hw/xen.h @@ -45,6 +45,7 @@ void xenstore_store_pv_console_info(int i, struct CharDriverState *chr); #if defined(NEED_CPU_H) && !defined(CONFIG_USER_ONLY) void xen_ram_alloc(ram_addr_t ram_addr, ram_addr_t size); +void xen_modified_memory(ram_addr_t start, ram_addr_t length); #endif #if defined(CONFIG_XEN) && CONFIG_XEN_CTRL_INTERFACE_VERSION < 400 diff --git a/xen-all.c b/xen-all.c index 6b4e511..121289d 100644 --- a/xen-all.c +++ b/xen-all.c @@ -1135,3 +1135,24 @@ void destroy_hvm_domain(bool reboot) xc_interface_close(xc_handle); } } + +void xen_modified_memory(ram_addr_t start, ram_addr_t length) +{ + if (unlikely(cpu_physical_memory_get_dirty_tracking())) { + int rc; + ram_addr_t start_pfn, nb_pages; + + if (length == 0) { + length = TARGET_PAGE_SIZE; + } + start_pfn = start >> TARGET_PAGE_BITS; + nb_pages = ((start + length + TARGET_PAGE_SIZE - 1) >> TARGET_PAGE_BITS) + - start_pfn; + rc = xc_hvm_modified_memory(xen_xc, xen_domid, start_pfn, nb_pages); + if (rc) { + fprintf(stderr, + "%s failed for "RAM_ADDR_FMT" ("RAM_ADDR_FMT"): %i, %s\n", + __func__, start, nb_pages, rc, strerror(-rc)); + } + } +} diff --git a/xen-stub.c b/xen-stub.c index 25317ec..7b54477 100644 --- a/xen-stub.c +++ b/xen-stub.c @@ -48,3 +48,7 @@ int xen_init(void) void qmp_xen_set_global_dirty_log(bool enable, Error **errp) { } + +void xen_modified_memory(ram_addr_t start, ram_addr_t length) +{ +} -- 1.7.4.1
This new helper/hook is used in the next patch to add an extra call in a single place. Backport of 51d7a9eb2b64e787c90bea1027308087eac22065 Signed-off-by: Alex Bligh <alex@alex.org.uk> --- exec.c | 45 +++++++++++++++++---------------------------- 1 files changed, 17 insertions(+), 28 deletions(-) diff --git a/exec.c b/exec.c index 6c206ff..511777b 100644 --- a/exec.c +++ b/exec.c @@ -3951,6 +3951,18 @@ int cpu_memory_rw_debug(CPUState *env, target_ulong addr, } #else + +static void invalidate_and_set_dirty(target_phys_addr_t addr, + target_phys_addr_t length) +{ + if (!cpu_physical_memory_is_dirty(addr)) { + /* invalidate code */ + tb_invalidate_phys_page_range(addr, addr + length, 0); + /* set dirty bit */ + cpu_physical_memory_set_dirty_flags(addr, (0xff & ~CODE_DIRTY_FLAG)); + } +} + void cpu_physical_memory_rw(target_phys_addr_t addr, uint8_t *buf, int len, int is_write) { @@ -4003,13 +4015,7 @@ void cpu_physical_memory_rw(target_phys_addr_t addr, uint8_t *buf, /* RAM case */ ptr = qemu_get_ram_ptr(addr1); memcpy(ptr, buf, l); - if (!cpu_physical_memory_is_dirty(addr1)) { - /* invalidate code */ - tb_invalidate_phys_page_range(addr1, addr1 + l, 0); - /* set dirty bit */ - cpu_physical_memory_set_dirty_flags( - addr1, (0xff & ~CODE_DIRTY_FLAG)); - } + invalidate_and_set_dirty(addr1, l); qemu_put_ram_ptr(ptr); } } else { @@ -4081,6 +4087,7 @@ void cpu_physical_memory_write_rom(target_phys_addr_t addr, /* ROM/RAM case */ ptr = qemu_get_ram_ptr(addr1); memcpy(ptr, buf, l); + invalidate_and_set_dirty(addr1, l); qemu_put_ram_ptr(ptr); } len -= l; @@ -4211,13 +4218,7 @@ void cpu_physical_memory_unmap(void *buffer, target_phys_addr_t len, l = TARGET_PAGE_SIZE; if (l > access_len) l = access_len; - if (!cpu_physical_memory_is_dirty(addr1)) { - /* invalidate code */ - tb_invalidate_phys_page_range(addr1, addr1 + l, 0); - /* set dirty bit */ - cpu_physical_memory_set_dirty_flags( - addr1, (0xff & ~CODE_DIRTY_FLAG)); - } + invalidate_and_set_dirty(addr1, l); addr1 += l; access_len -= l; } @@ -4561,13 +4562,7 @@ static inline void stl_phys_internal(target_phys_addr_t addr, uint32_t val, stl_p(ptr, val); break; } - if (!cpu_physical_memory_is_dirty(addr1)) { - /* invalidate code */ - tb_invalidate_phys_page_range(addr1, addr1 + 4, 0); - /* set dirty bit */ - cpu_physical_memory_set_dirty_flags(addr1, - (0xff & ~CODE_DIRTY_FLAG)); - } + invalidate_and_set_dirty(addr1, 4); } } @@ -4639,13 +4634,7 @@ static inline void stw_phys_internal(target_phys_addr_t addr, uint32_t val, stw_p(ptr, val); break; } - if (!cpu_physical_memory_is_dirty(addr1)) { - /* invalidate code */ - tb_invalidate_phys_page_range(addr1, addr1 + 2, 0); - /* set dirty bit */ - cpu_physical_memory_set_dirty_flags(addr1, - (0xff & ~CODE_DIRTY_FLAG)); - } + invalidate_and_set_dirty(addr1, 2); } } -- 1.7.4.1
This patch add some calls to xen_modified_memory to notify Xen about dirtybits during migration. Backport of e226939de5814527a21396903b08c3d0ed989558 Signed-off-by: Alex Bligh <alex@alex.org.uk> --- exec.c | 4 ++++ memory.c | 4 ++++ 2 files changed, 8 insertions(+), 0 deletions(-) diff --git a/exec.c b/exec.c index 511777b..401f9bc 100644 --- a/exec.c +++ b/exec.c @@ -2988,6 +2988,9 @@ ram_addr_t qemu_ram_alloc_from_ptr(DeviceState *dev, const char *name, memset(ram_list.phys_dirty + (new_block->offset >> TARGET_PAGE_BITS), 0xff, size >> TARGET_PAGE_BITS); + if (xen_enabled()) + xen_modified_memory(new_block->offset, size); + if (kvm_enabled()) kvm_setup_guest_memory(new_block->host, size); @@ -3961,6 +3964,7 @@ static void invalidate_and_set_dirty(target_phys_addr_t addr, /* set dirty bit */ cpu_physical_memory_set_dirty_flags(addr, (0xff & ~CODE_DIRTY_FLAG)); } + xen_modified_memory(addr, length); } void cpu_physical_memory_rw(target_phys_addr_t addr, uint8_t *buf, diff --git a/memory.c b/memory.c index 7c20a07..6e0c596 100644 --- a/memory.c +++ b/memory.c @@ -16,6 +16,7 @@ #include "ioport.h" #include "bitops.h" #include "kvm.h" +#include "hw/xen.h" #include <assert.h> unsigned memory_region_transaction_depth = 0; @@ -1065,6 +1066,9 @@ bool memory_region_get_dirty(MemoryRegion *mr, target_phys_addr_t addr, void memory_region_set_dirty(MemoryRegion *mr, target_phys_addr_t addr) { assert(mr->terminates); + if (xen_enabled()) + xen_modified_memory((mr->ram_addr + addr) & TARGET_PAGE_MASK, + TARGET_PAGE_SIZE); return cpu_physical_memory_set_dirty(mr->ram_addr + addr); } -- 1.7.4.1
Alex Bligh
2013-Feb-19 13:17 UTC
[PATCHv4 5/5] xen: Set the vram dirty when an error occur.
If the call to xc_hvm_track_dirty_vram() fails, then we set dirtybit on all the video ram. This case happens during migration. Backport of 8aba7dc02d5660df7e7d8651304b3079908358be Signed-off-by: Alex Bligh <alex@alex.org.uk> --- 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; } @@ -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); }; } -- 1.7.4.1
Stefano Stabellini
2013-Feb-19 17:42 UTC
Re: [PATCHv4 3/5] exec: Introduce helper to set dirty flags.
On Tue, 19 Feb 2013, Alex Bligh wrote:> This new helper/hook is used in the next patch to add an extra call in a single > place. > > Backport of 51d7a9eb2b64e787c90bea1027308087eac22065 > > Signed-off-by: Alex Bligh <alex@alex.org.uk> > --- > exec.c | 45 +++++++++++++++++---------------------------- > 1 files changed, 17 insertions(+), 28 deletions(-) > > diff --git a/exec.c b/exec.c > index 6c206ff..511777b 100644 > --- a/exec.c > +++ b/exec.c > @@ -3951,6 +3951,18 @@ int cpu_memory_rw_debug(CPUState *env, target_ulong addr, > } > > #else > + > +static void invalidate_and_set_dirty(target_phys_addr_t addr, > + target_phys_addr_t length) > +{ > + if (!cpu_physical_memory_is_dirty(addr)) { > + /* invalidate code */ > + tb_invalidate_phys_page_range(addr, addr + length, 0); > + /* set dirty bit */ > + cpu_physical_memory_set_dirty_flags(addr, (0xff & ~CODE_DIRTY_FLAG)); > + } > +} > + > void cpu_physical_memory_rw(target_phys_addr_t addr, uint8_t *buf, > int len, int is_write) > { > @@ -4003,13 +4015,7 @@ void cpu_physical_memory_rw(target_phys_addr_t addr, uint8_t *buf, > /* RAM case */ > ptr = qemu_get_ram_ptr(addr1); > memcpy(ptr, buf, l); > - if (!cpu_physical_memory_is_dirty(addr1)) { > - /* invalidate code */ > - tb_invalidate_phys_page_range(addr1, addr1 + l, 0); > - /* set dirty bit */ > - cpu_physical_memory_set_dirty_flags( > - addr1, (0xff & ~CODE_DIRTY_FLAG)); > - } > + invalidate_and_set_dirty(addr1, l); > qemu_put_ram_ptr(ptr); > } > } else { > @@ -4081,6 +4087,7 @@ void cpu_physical_memory_write_rom(target_phys_addr_t addr, > /* ROM/RAM case */ > ptr = qemu_get_ram_ptr(addr1); > memcpy(ptr, buf, l); > + invalidate_and_set_dirty(addr1, l); > qemu_put_ram_ptr(ptr); > } > len -= l;In QEMU the code style is spaces for indentation. Moreover the right way of doing the backport here would be firstly to backport 0b57e287, then 51d7a9eb2b64e787c90bea1027308087eac22065 should just apply.
Stefano Stabellini
2013-Feb-19 18:03 UTC
Re: [PATCHv4 4/5] exec, memory: Call to xen_modified_memory.
On Tue, 19 Feb 2013, Alex Bligh wrote:> This patch add some calls to xen_modified_memory to notify Xen about dirtybits > during migration. > > Backport of e226939de5814527a21396903b08c3d0ed989558 > > Signed-off-by: Alex Bligh <alex@alex.org.uk> > > exec.c | 4 ++++ > memory.c | 4 ++++ > 2 files changed, 8 insertions(+), 0 deletions(-) > > diff --git a/exec.c b/exec.c > index 511777b..401f9bc 100644 > --- a/exec.c > +++ b/exec.c > @@ -2988,6 +2988,9 @@ ram_addr_t qemu_ram_alloc_from_ptr(DeviceState *dev, const char *name, > memset(ram_list.phys_dirty + (new_block->offset >> TARGET_PAGE_BITS), > 0xff, size >> TARGET_PAGE_BITS); > > + if (xen_enabled()) > + xen_modified_memory(new_block->offset, size); > +e226939de5814527a21396903b08c3d0ed989558 adds two calls to xen_modified_memory, one in cpu_physical_memory_set_dirty_range, the other in invalidate_and_set_dirty. Where does this one come from? If something is missing you need to backport that patch too, rather than trying to adapt this patch. In this case I believe you need to backport 1720aeee72888f80b974c33b6eb39922a0bea992.> if (kvm_enabled()) > kvm_setup_guest_memory(new_block->host, size); > > @@ -3961,6 +3964,7 @@ static void invalidate_and_set_dirty(target_phys_addr_t addr, > /* set dirty bit */ > cpu_physical_memory_set_dirty_flags(addr, (0xff & ~CODE_DIRTY_FLAG)); > } > + xen_modified_memory(addr, length); > } > > void cpu_physical_memory_rw(target_phys_addr_t addr, uint8_t *buf, > diff --git a/memory.c b/memory.c > index 7c20a07..6e0c596 100644 > --- a/memory.c > +++ b/memory.c > @@ -16,6 +16,7 @@ > #include "ioport.h" > #include "bitops.h" > #include "kvm.h" > +#include "hw/xen.h" > #include <assert.h> > > unsigned memory_region_transaction_depth = 0; > @@ -1065,6 +1066,9 @@ bool memory_region_get_dirty(MemoryRegion *mr, target_phys_addr_t addr, > void memory_region_set_dirty(MemoryRegion *mr, target_phys_addr_t addr) > { > assert(mr->terminates); > + if (xen_enabled()) > + xen_modified_memory((mr->ram_addr + addr) & TARGET_PAGE_MASK, > + TARGET_PAGE_SIZE); > return cpu_physical_memory_set_dirty(mr->ram_addr + addr); > } > > -- > 1.7.4.1 >
Stefano Stabellini
2013-Feb-19 18:05 UTC
Re: [PATCHv4 5/5] xen: Set the vram dirty when an error occur.
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. > > Backport of 8aba7dc02d5660df7e7d8651304b3079908358be > > Signed-off-by: Alex Bligh <alex@alex.org.uk> > --- > 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?> @@ -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?
Alex Bligh
2013-Feb-19 21:31 UTC
Re: [PATCHv4 5/5] xen: Set the vram dirty when an error occur.
Stefano, --On 19 February 2013 18:05:46 +0000 Stefano Stabellini <stefano.stabellini@eu.citrix.com> 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@alex.org.uk> >> --- >> 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
Alex Bligh
2013-Feb-19 21:42 UTC
Re: [PATCHv4 3/5] exec: Introduce helper to set dirty flags.
Stefano, --On 19 February 2013 17:42:50 +0000 Stefano Stabellini <stefano.stabellini@eu.citrix.com> wrote:> In QEMU the code style is spaces for indentation.OK no idea how that happened - sorry. I''m using qemu''s published emacs c mode (or thought I was).> Moreover the right way of doing the backport here would be firstly to > backport 0b57e287, then 51d7a9eb2b64e787c90bea1027308087eac22065 should > just apply.I just backported the 5 patches you pointed me at, and any code changes they required. So the change I posted brings in invalidate_and_set_dirty, pretty much per the original patch. Is the issue here I combined it with the other patch you mentioned? Guidance welcome. -- Alex Bligh
Alex Bligh
2013-Feb-19 21:47 UTC
Re: [PATCHv4 4/5] exec, memory: Call to xen_modified_memory.
--On 19 February 2013 18:03:04 +0000 Stefano Stabellini <stefano.stabellini@eu.citrix.com> wrote:> e226939de5814527a21396903b08c3d0ed989558 adds two calls to > xen_modified_memory, one in cpu_physical_memory_set_dirty_range, the > other in invalidate_and_set_dirty. > > Where does this one come from? > > If something is missing you need to backport that patch too, rather than > trying to adapt this patch. > In this case I believe you need to backport > 1720aeee72888f80b974c33b6eb39922a0bea992.Again, I was trying to make the fewest changes possible, being aware I have little understanding of the code. Some assistance would be appreciated here particularly as the substitute patch you mention would appear to have have wider implications. -- Alex Bligh
Stefano Stabellini
2013-Feb-20 12:06 UTC
Re: [PATCHv4 3/5] exec: Introduce helper to set dirty flags.
On Tue, 19 Feb 2013, Alex Bligh wrote:> Stefano, > > --On 19 February 2013 17:42:50 +0000 Stefano Stabellini > <stefano.stabellini@eu.citrix.com> wrote: > > > In QEMU the code style is spaces for indentation. > > OK no idea how that happened - sorry. I''m using qemu''s published emacs > c mode (or thought I was). > > > Moreover the right way of doing the backport here would be firstly to > > backport 0b57e287, then 51d7a9eb2b64e787c90bea1027308087eac22065 should > > just apply. > > I just backported the 5 patches you pointed me at, and any code changes > they required. > > So the change I posted brings in invalidate_and_set_dirty, pretty much > per the original patch. Is the issue here I combined it with the other > patch you mentioned? Guidance welcome.In general is preferable to backport a patch as-is. If the patch in question depends on another patch, which is small and can also be easily backported, like in this case, the best course of action is to backport them both as separate patches.
Stefano Stabellini
2013-Feb-20 12:21 UTC
Re: [PATCHv4 5/5] xen: Set the vram dirty when an error occur.
On Tue, 19 Feb 2013, Alex Bligh wrote:> Stefano, > > --On 19 February 2013 18:05:46 +0000 Stefano Stabellini > <stefano.stabellini@eu.citrix.com> 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@alex.org.uk> > >> --- > >> 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.
Stefano Stabellini
2013-Feb-20 12:30 UTC
Re: [PATCHv4 4/5] exec, memory: Call to xen_modified_memory.
On Tue, 19 Feb 2013, Alex Bligh wrote:> --On 19 February 2013 18:03:04 +0000 Stefano Stabellini > <stefano.stabellini@eu.citrix.com> wrote: > > > e226939de5814527a21396903b08c3d0ed989558 adds two calls to > > xen_modified_memory, one in cpu_physical_memory_set_dirty_range, the > > other in invalidate_and_set_dirty. > > > > Where does this one come from? > > > > If something is missing you need to backport that patch too, rather than > > trying to adapt this patch. > > In this case I believe you need to backport > > 1720aeee72888f80b974c33b6eb39922a0bea992. > > Again, I was trying to make the fewest changes possible, being aware I > have little understanding of the code. Some assistance would be appreciated > here particularly as the substitute patch you mention would appear to have > have wider implications.OK. Keep the patch as you wrote it and add a comment in the description saying why you needed to make the additional changes that are not part of e226939de5814527a21396903b08c3d0ed989558. Also add a call to xen_modified_memory from cpu_physical_memory_set_dirty, if I am not mistaken that should solve the problem with the dirty vram.
Alex Bligh
2013-Feb-20 20:35 UTC
Re: [PATCHv4 4/5] exec, memory: Call to xen_modified_memory.
Stefano, --On 20 February 2013 12:30:45 +0000 Stefano Stabellini <stefano.stabellini@eu.citrix.com> wrote:> Also add a call to xen_modified_memory from > cpu_physical_memory_set_dirty, if I am not mistaken that should solve > the problem with the dirty vram.I think I now recall why I did not do this. Firstly, cpu_physical_memory_set_dirty is inline, and I''d have to check (presumably) xen_enabled() then call a non-inline function. This seemed a bit much to satisfy only the needs of the VRAM check as (as far as I could tell - I may well be wrong) xen_modifed_memory was being called appropriately by all the other callers anyway. Secondly, if I do need to modify this one, it seemed odd that I''d need not need to modify cpu_physical_set_dirty_flags. It seemed cleaner just to fix up the VRAM stuff. What do you think? I''ll send another patch series with everything else in. -- Alex Bligh