* save/restore race
@ 2007-01-23 22:01 John Levon
2007-01-23 22:15 ` Ian Pratt
0 siblings, 1 reply; 10+ messages in thread
From: John Levon @ 2007-01-23 22:01 UTC (permalink / raw)
To: xen-devel
save requires a valid arch.pfn_to_mfn_frame_list_list MFN. However,
there is no guarantee that this is up to date, since a previous
restore is considered complete as soon as the domain is unpaused:
if not paused:
dominfo.unpause()
dominfo.completeRestore(handler.store_mfn, handler.console_mfn)
It seems that Linux is being lucky here, in that rebuilding the MFNs is
the first thing it does after suspend(). On Solaris, it occurs somewhat
later in the resume process due to constraints on locking within our MMU
code.
This doesn't seem specific to migration either, a save just after a
restore has completed can hit this race as far as I can see. I'm short
on ideas that don't involve a new interface (like the domain writing
back a xenstore value when it's done resuming). Suggestions?
regards
john
^ permalink raw reply [flat|nested] 10+ messages in thread* RE: save/restore race 2007-01-23 22:01 save/restore race John Levon @ 2007-01-23 22:15 ` Ian Pratt 2007-01-23 22:48 ` John Levon 0 siblings, 1 reply; 10+ messages in thread From: Ian Pratt @ 2007-01-23 22:15 UTC (permalink / raw) To: John Levon, xen-devel > save requires a valid arch.pfn_to_mfn_frame_list_list MFN. However, > there is no guarantee that this is up to date, since a previous > restore is considered complete as soon as the domain is unpaused: > > if not paused: > dominfo.unpause() > > dominfo.completeRestore(handler.store_mfn, handler.console_mfn) > > It seems that Linux is being lucky here, in that rebuilding the MFNs is > the first thing it does after suspend(). On Solaris, it occurs somewhat > later in the resume process due to constraints on locking within our MMU > code. > > This doesn't seem specific to migration either, a save just after a > restore has completed can hit this race as far as I can see. I'm short > on ideas that don't involve a new interface (like the domain writing > back a xenstore value when it's done resuming). Suggestions? Just ignore any new suspend request until you've got the frame list list rebuilt. In the Linux code this is implicit as xenbus and even interrupts aren't re-enabled until after this. Ian ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: save/restore race 2007-01-23 22:15 ` Ian Pratt @ 2007-01-23 22:48 ` John Levon 2007-01-23 23:38 ` Ian Pratt 0 siblings, 1 reply; 10+ messages in thread From: John Levon @ 2007-01-23 22:48 UTC (permalink / raw) To: Ian Pratt; +Cc: xen-devel On Tue, Jan 23, 2007 at 10:15:29PM -0000, Ian Pratt wrote: > Just ignore any new suspend request until you've got the frame list list > rebuilt. see xc_linux_save.c: 667 live_p2m_frame_list_list = 668 xc_map_foreign_range(xc_handle, dom, PAGE_SIZE, PROT_READ, 669 live_shinfo->arch.pfn_to_mfn_frame_list_list); ... 1087 if (suspend_and_state(suspend, xc_handle, io_fd, dom, &info, 1088 &ctxt)) { 1089 ERROR("Domain appears not to have suspended"); 1090 goto out; 1091 } We use the value long before the domain is told to suspend. regards john ^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: save/restore race 2007-01-23 22:48 ` John Levon @ 2007-01-23 23:38 ` Ian Pratt 2007-01-24 2:10 ` John Levon 0 siblings, 1 reply; 10+ messages in thread From: Ian Pratt @ 2007-01-23 23:38 UTC (permalink / raw) To: John Levon; +Cc: xen-devel > > Just ignore any new suspend request until you've got the frame list list > > rebuilt. > > see xc_linux_save.c: > > 667 live_p2m_frame_list_list = > 668 xc_map_foreign_range(xc_handle, dom, PAGE_SIZE, PROT_READ, > 669 live_shinfo->arch.pfn_to_mfn_frame_list_list); > > ... > > 1087 if (suspend_and_state(suspend, xc_handle, io_fd, dom, > &info, > 1088 &ctxt)) { > 1089 ERROR("Domain appears not to have suspended"); > 1090 goto out; > 1091 } > > We use the value long before the domain is told to suspend. Ah, I see what you're getting at. We should make xc_linux_save cope more gracefully with arch.pfn_to_mfn_frame_list_list being NULL, have xc_linux_save zero the field in shared_info before writing it out (its an MFN and not valid anyhow), and then move the field's re-initialisation a little later in the post_suspend function to close the race. Best, Ian ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: save/restore race 2007-01-23 23:38 ` Ian Pratt @ 2007-01-24 2:10 ` John Levon 2007-01-24 2:19 ` Ian Pratt 0 siblings, 1 reply; 10+ messages in thread From: John Levon @ 2007-01-24 2:10 UTC (permalink / raw) To: Ian Pratt; +Cc: xen-devel On Tue, Jan 23, 2007 at 11:38:29PM -0000, Ian Pratt wrote: > We should make xc_linux_save cope more gracefully with > arch.pfn_to_mfn_frame_list_list being NULL, have xc_linux_save zero the > field in shared_info before writing it out (its an MFN and not valid > anyhow), and then move the field's re-initialisation a little later in > the post_suspend function to close the race. I've done some testing with the below, and it seems to do the trick. regards john # HG changeset patch # User john.levon@sun.com # Date 1169600022 28800 # Node ID 3121e0cb7be68fa46c59f61422f05adc03ec5a5e # Parent a75413c0072fa5b892bd8b6a05c1f1d3435bb093 Close save-after-restore race. Make xc_linux_save() wait for the frame_list_list MFN to be updated by the domain before trying to use it. Signed-off-by: John Levon <john.levon@sun.com> diff --git a/tools/libxc/xc_linux_save.c b/tools/libxc/xc_linux_save.c --- a/tools/libxc/xc_linux_save.c +++ b/tools/libxc/xc_linux_save.c @@ -403,6 +403,33 @@ static int suspend_and_state(int (*suspe return -1; } +/* +** Map the top-level page of MFNs from the guest. The guest might not have +** finished resuming from a previous restore operation, so we wait a while for +** it to update the MFN to a reasonable value. +*/ +static void *map_frame_list_list(int xc_handle, uint32_t dom, + shared_info_t *shinfo) +{ + int count = 3000; + void *p; + + while (count-- && shinfo->arch.pfn_to_mfn_frame_list_list == 0) + usleep(10000); + + if (shinfo->arch.pfn_to_mfn_frame_list_list == 0) { + ERROR("Timed out waiting for frame list updated."); + return NULL; + } + + p = xc_map_foreign_range(xc_handle, dom, PAGE_SIZE, PROT_READ, + shinfo->arch.pfn_to_mfn_frame_list_list); + + if (p == NULL) + ERROR("Couldn't map p2m_frame_list_list (errno %d)", errno); + + return p; +} /* ** During transfer (or in the state file), all page-table pages must be @@ -663,14 +690,11 @@ int xc_linux_save(int xc_handle, int io_ max_pfn = live_shinfo->arch.max_pfn; - live_p2m_frame_list_list = - xc_map_foreign_range(xc_handle, dom, PAGE_SIZE, PROT_READ, - live_shinfo->arch.pfn_to_mfn_frame_list_list); - - if (!live_p2m_frame_list_list) { - ERROR("Couldn't map p2m_frame_list_list (errno %d)", errno); - goto out; - } + live_p2m_frame_list_list = map_frame_list_list(xc_handle, dom, + live_shinfo); + + if (!live_p2m_frame_list_list) + goto out; live_p2m_frame_list = xc_map_foreign_batch(xc_handle, dom, PROT_READ, @@ -1169,8 +1193,14 @@ int xc_linux_save(int xc_handle, int io_ ctxt.ctrlreg[3] = xen_pfn_to_cr3(mfn_to_pfn(xen_cr3_to_pfn(ctxt.ctrlreg[3]))); + /* + * Reset the MFN to be a known-invalid value. See map_frame_list_list(). + */ + memcpy(page, live_shinfo, PAGE_SIZE); + ((shared_info_t *)page)->arch.pfn_to_mfn_frame_list_list = 0; + if (!write_exact(io_fd, &ctxt, sizeof(ctxt)) || - !write_exact(io_fd, live_shinfo, PAGE_SIZE)) { + !write_exact(io_fd, page, PAGE_SIZE)) { ERROR("Error when writing to state file (1) (errno %d)", errno); goto out; } ^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: save/restore race 2007-01-24 2:10 ` John Levon @ 2007-01-24 2:19 ` Ian Pratt 2007-01-24 3:13 ` [PATCH] " John Levon 0 siblings, 1 reply; 10+ messages in thread From: Ian Pratt @ 2007-01-24 2:19 UTC (permalink / raw) To: John Levon; +Cc: xen-devel > > We should make xc_linux_save cope more gracefully with > > arch.pfn_to_mfn_frame_list_list being NULL, have xc_linux_save zero the > > field in shared_info before writing it out (its an MFN and not valid > > anyhow), and then move the field's re-initialisation a little later in > > the post_suspend function to close the race. > > I've done some testing with the below, and it seems to do the trick. 30 seconds is quite a time to wait... Wouldn't a second be more appropriate? While you're at it, I'd be grateful if you could move the shared_info assignment in linux's post_suspend() and setup_arch() [i386 and x86_64] below the list building code. Thanks, Ian > regards > john > > > # HG changeset patch > # User john.levon@sun.com > # Date 1169600022 28800 > # Node ID 3121e0cb7be68fa46c59f61422f05adc03ec5a5e > # Parent a75413c0072fa5b892bd8b6a05c1f1d3435bb093 > Close save-after-restore race. Make xc_linux_save() wait for the > frame_list_list MFN to be updated by the domain before trying to use it. > > Signed-off-by: John Levon <john.levon@sun.com> > > diff --git a/tools/libxc/xc_linux_save.c b/tools/libxc/xc_linux_save.c > --- a/tools/libxc/xc_linux_save.c > +++ b/tools/libxc/xc_linux_save.c > @@ -403,6 +403,33 @@ static int suspend_and_state(int (*suspe > return -1; > } > > +/* > +** Map the top-level page of MFNs from the guest. The guest might not have > +** finished resuming from a previous restore operation, so we wait a while > for > +** it to update the MFN to a reasonable value. > +*/ > +static void *map_frame_list_list(int xc_handle, uint32_t dom, > + shared_info_t *shinfo) > +{ > + int count = 3000; > + void *p; > + > + while (count-- && shinfo->arch.pfn_to_mfn_frame_list_list == 0) > + usleep(10000); > + > + if (shinfo->arch.pfn_to_mfn_frame_list_list == 0) { > + ERROR("Timed out waiting for frame list updated."); > + return NULL; > + } > + > + p = xc_map_foreign_range(xc_handle, dom, PAGE_SIZE, PROT_READ, > + shinfo->arch.pfn_to_mfn_frame_list_list); > + > + if (p == NULL) > + ERROR("Couldn't map p2m_frame_list_list (errno %d)", errno); > + > + return p; > +} > > /* > ** During transfer (or in the state file), all page-table pages must be > @@ -663,14 +690,11 @@ int xc_linux_save(int xc_handle, int io_ > > max_pfn = live_shinfo->arch.max_pfn; > > - live_p2m_frame_list_list = > - xc_map_foreign_range(xc_handle, dom, PAGE_SIZE, PROT_READ, > - live_shinfo- > >arch.pfn_to_mfn_frame_list_list); > - > - if (!live_p2m_frame_list_list) { > - ERROR("Couldn't map p2m_frame_list_list (errno %d)", errno); > - goto out; > - } > + live_p2m_frame_list_list = map_frame_list_list(xc_handle, dom, > + live_shinfo); > + > + if (!live_p2m_frame_list_list) > + goto out; > > live_p2m_frame_list = > xc_map_foreign_batch(xc_handle, dom, PROT_READ, > @@ -1169,8 +1193,14 @@ int xc_linux_save(int xc_handle, int io_ > ctxt.ctrlreg[3] = > xen_pfn_to_cr3(mfn_to_pfn(xen_cr3_to_pfn(ctxt.ctrlreg[3]))); > > + /* > + * Reset the MFN to be a known-invalid value. See > map_frame_list_list(). > + */ > + memcpy(page, live_shinfo, PAGE_SIZE); > + ((shared_info_t *)page)->arch.pfn_to_mfn_frame_list_list = 0; > + > if (!write_exact(io_fd, &ctxt, sizeof(ctxt)) || > - !write_exact(io_fd, live_shinfo, PAGE_SIZE)) { > + !write_exact(io_fd, page, PAGE_SIZE)) { > ERROR("Error when writing to state file (1) (errno %d)", errno); > goto out; > } ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] save/restore race 2007-01-24 2:19 ` Ian Pratt @ 2007-01-24 3:13 ` John Levon 2007-01-24 10:07 ` Steven Hand 2007-02-06 15:04 ` Srikanth SM 0 siblings, 2 replies; 10+ messages in thread From: John Levon @ 2007-01-24 3:13 UTC (permalink / raw) To: Ian Pratt; +Cc: xen-devel On Wed, Jan 24, 2007 at 02:19:42AM -0000, Ian Pratt wrote: > 30 seconds is quite a time to wait... Wouldn't a second be more > appropriate? > > While you're at it, I'd be grateful if you could move the shared_info > assignment in linux's post_suspend() and setup_arch() [i386 and x86_64] > below the list building code. Sure. Patch below is for 3.0.4 but should essentially apply to xen-unstable too. thanks john # HG changeset patch # User john.levon@sun.com # Date 1169607991 28800 # Node ID b5ac8fda95bc5f9b789df80095d59e23e7f40205 # Parent a75413c0072fa5b892bd8b6a05c1f1d3435bb093 Close save-after-restore race. Make xc_linux_save() wait for the frame_list_list MFN to be updated by the domain before trying to use it. Make Linux set the top-level MFN /after/ updating the other MFN lists. Signed-off-by: John Levon <john.levon@sun.com> diff --git a/linux-2.6-xen-sparse/arch/i386/kernel/setup-xen.c b/linux-2.6-xen-sparse/arch/i386/kernel/setup-xen.c --- a/linux-2.6-xen-sparse/arch/i386/kernel/setup-xen.c +++ b/linux-2.6-xen-sparse/arch/i386/kernel/setup-xen.c @@ -1777,8 +1777,6 @@ void __init setup_arch(char **cmdline_p) * frames that make up the p2m table. Used by save/restore */ pfn_to_mfn_frame_list_list = alloc_bootmem_low_pages(PAGE_SIZE); - HYPERVISOR_shared_info->arch.pfn_to_mfn_frame_list_list = - virt_to_mfn(pfn_to_mfn_frame_list_list); fpp = PAGE_SIZE/sizeof(unsigned long); for (i=0, j=0, k=-1; i< max_pfn; i+=fpp, j++) { @@ -1795,6 +1793,8 @@ void __init setup_arch(char **cmdline_p) virt_to_mfn(&phys_to_machine_mapping[i]); } HYPERVISOR_shared_info->arch.max_pfn = max_pfn; + HYPERVISOR_shared_info->arch.pfn_to_mfn_frame_list_list = + virt_to_mfn(pfn_to_mfn_frame_list_list); } /* diff --git a/linux-2.6-xen-sparse/arch/x86_64/kernel/setup-xen.c b/linux-2.6-xen-sparse/arch/x86_64/kernel/setup-xen.c --- a/linux-2.6-xen-sparse/arch/x86_64/kernel/setup-xen.c +++ b/linux-2.6-xen-sparse/arch/x86_64/kernel/setup-xen.c @@ -862,8 +862,6 @@ void __init setup_arch(char **cmdline_p) * save/restore. */ pfn_to_mfn_frame_list_list = alloc_bootmem_pages(PAGE_SIZE); - HYPERVISOR_shared_info->arch.pfn_to_mfn_frame_list_list = - virt_to_mfn(pfn_to_mfn_frame_list_list); fpp = PAGE_SIZE/sizeof(unsigned long); for (i=0, j=0, k=-1; i< end_pfn; i+=fpp, j++) { @@ -880,6 +878,8 @@ void __init setup_arch(char **cmdline_p) virt_to_mfn(&phys_to_machine_mapping[i]); } HYPERVISOR_shared_info->arch.max_pfn = end_pfn; + HYPERVISOR_shared_info->arch.pfn_to_mfn_frame_list_list = + virt_to_mfn(pfn_to_mfn_frame_list_list); } } diff --git a/linux-2.6-xen-sparse/drivers/xen/core/machine_reboot.c b/linux-2.6-xen-sparse/drivers/xen/core/machine_reboot.c --- a/linux-2.6-xen-sparse/drivers/xen/core/machine_reboot.c +++ b/linux-2.6-xen-sparse/drivers/xen/core/machine_reboot.c @@ -98,9 +98,6 @@ static void post_suspend(void) memset(empty_zero_page, 0, PAGE_SIZE); - HYPERVISOR_shared_info->arch.pfn_to_mfn_frame_list_list = - virt_to_mfn(pfn_to_mfn_frame_list_list); - fpp = PAGE_SIZE/sizeof(unsigned long); for (i = 0, j = 0, k = -1; i < max_pfn; i += fpp, j++) { if ((j % fpp) == 0) { @@ -113,6 +110,8 @@ static void post_suspend(void) virt_to_mfn(&phys_to_machine_mapping[i]); } HYPERVISOR_shared_info->arch.max_pfn = max_pfn; + HYPERVISOR_shared_info->arch.pfn_to_mfn_frame_list_list = + virt_to_mfn(pfn_to_mfn_frame_list_list); } #else /* !(defined(__i386__) || defined(__x86_64__)) */ diff --git a/tools/libxc/xc_linux_save.c b/tools/libxc/xc_linux_save.c --- a/tools/libxc/xc_linux_save.c +++ b/tools/libxc/xc_linux_save.c @@ -403,6 +403,33 @@ static int suspend_and_state(int (*suspe return -1; } +/* +** Map the top-level page of MFNs from the guest. The guest might not have +** finished resuming from a previous restore operation, so we wait a while for +** it to update the MFN to a reasonable value. +*/ +static void *map_frame_list_list(int xc_handle, uint32_t dom, + shared_info_t *shinfo) +{ + int count = 100; + void *p; + + while (count-- && shinfo->arch.pfn_to_mfn_frame_list_list == 0) + usleep(10000); + + if (shinfo->arch.pfn_to_mfn_frame_list_list == 0) { + ERROR("Timed out waiting for frame list updated."); + return NULL; + } + + p = xc_map_foreign_range(xc_handle, dom, PAGE_SIZE, PROT_READ, + shinfo->arch.pfn_to_mfn_frame_list_list); + + if (p == NULL) + ERROR("Couldn't map p2m_frame_list_list (errno %d)", errno); + + return p; +} /* ** During transfer (or in the state file), all page-table pages must be @@ -663,14 +690,11 @@ int xc_linux_save(int xc_handle, int io_ max_pfn = live_shinfo->arch.max_pfn; - live_p2m_frame_list_list = - xc_map_foreign_range(xc_handle, dom, PAGE_SIZE, PROT_READ, - live_shinfo->arch.pfn_to_mfn_frame_list_list); - - if (!live_p2m_frame_list_list) { - ERROR("Couldn't map p2m_frame_list_list (errno %d)", errno); - goto out; - } + live_p2m_frame_list_list = map_frame_list_list(xc_handle, dom, + live_shinfo); + + if (!live_p2m_frame_list_list) + goto out; live_p2m_frame_list = xc_map_foreign_batch(xc_handle, dom, PROT_READ, @@ -1169,8 +1193,14 @@ int xc_linux_save(int xc_handle, int io_ ctxt.ctrlreg[3] = xen_pfn_to_cr3(mfn_to_pfn(xen_cr3_to_pfn(ctxt.ctrlreg[3]))); + /* + * Reset the MFN to be a known-invalid value. See map_frame_list_list(). + */ + memcpy(page, live_shinfo, PAGE_SIZE); + ((shared_info_t *)page)->arch.pfn_to_mfn_frame_list_list = 0; + if (!write_exact(io_fd, &ctxt, sizeof(ctxt)) || - !write_exact(io_fd, live_shinfo, PAGE_SIZE)) { + !write_exact(io_fd, page, PAGE_SIZE)) { ERROR("Error when writing to state file (1) (errno %d)", errno); goto out; } ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] save/restore race 2007-01-24 3:13 ` [PATCH] " John Levon @ 2007-01-24 10:07 ` Steven Hand 2007-02-06 15:04 ` Srikanth SM 1 sibling, 0 replies; 10+ messages in thread From: Steven Hand @ 2007-01-24 10:07 UTC (permalink / raw) To: John Levon; +Cc: Ian Pratt, xen-devel, Steven.Hand >Sure. Patch below is for 3.0.4 but should essentially apply to >xen-unstable too. Applied to -unstable and 3.0.4 thanks, S. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] save/restore race 2007-01-24 3:13 ` [PATCH] " John Levon 2007-01-24 10:07 ` Steven Hand @ 2007-02-06 15:04 ` Srikanth SM 2007-02-06 15:13 ` John Levon 1 sibling, 1 reply; 10+ messages in thread From: Srikanth SM @ 2007-02-06 15:04 UTC (permalink / raw) To: John Levon, m+Ian.Pratt; +Cc: xen-devel [-- Attachment #1: Type: text/plain, Size: 8721 bytes --] Hi, I Came across this thread trying to understand live migration better. I did some experiments and atleast on linux, I could do away with the waiting for the arch.pfn_to_mfn_frame_list_list to get initialized after a restore. I believe it will work with other OSes as well. I request your comments regarding this. Please see if the below patch (against xen-unstable) can be used. What I did was, simple-minded though: Instead of the guest building p2m frame-list-list and the frame-list _after_ a restore, we build it from dom0 _during_ restore. - In save, canonicalise the pfn_to_mfn_frame_list_list mfn along with the entries in that page and store them in state file. (Note that we are anyway saving canonicalised p2m_frame_list pages) - In restore, uncanonicalise pfn_to_mfn_frame_list_list pfn, and the entries of the page and copy it to appropriate place in guest memory. Also copy p2m_frame_list entries to appropriate place in guest memory. However, what I do not know is the exact reason why this initialization of pfn_to_mfn_frame_list_list and pfn_to_mfn_frame_list is being done by the guest domain after unpausing currently. Could you please clarify this? Thanks and regards Srikanth Patch for inline viewing: # HG changeset patch # User srao@srikanth # Node ID 6443c0465741c05ffef24f5ca7c7fd3907089d04 # Parent 8bc64a3a505413e4d3bd7def06539ef088f2ca0c Avoid waiting for pfn-to-mfn-frame-list-list etc., to get initialized when doing a save after restore. Signed-off-by: Srikanth S. M. <srikanth_sm@symantec.com> diff -r 8bc64a3a5054 -r 6443c0465741 linux-2.6-xen-sparse/drivers/xen/core/machine_reboot.c --- a/linux-2.6-xen-sparse/drivers/xen/core/machine_reboot.c Mon Feb 5 16:40:19 2007 +++ b/linux-2.6-xen-sparse/drivers/xen/core/machine_reboot.c Tue Feb 6 14:42:34 2007 @@ -87,10 +87,7 @@ static void post_suspend(int suspend_cancelled) { - int i, j, k, fpp; extern unsigned long max_pfn; - extern unsigned long *pfn_to_mfn_frame_list_list; - extern unsigned long *pfn_to_mfn_frame_list[]; if (suspend_cancelled) { xen_start_info->store_mfn = @@ -105,20 +102,7 @@ memset(empty_zero_page, 0, PAGE_SIZE); - fpp = PAGE_SIZE/sizeof(unsigned long); - for (i = 0, j = 0, k = -1; i < max_pfn; i += fpp, j++) { - if ((j % fpp) == 0) { - k++; - pfn_to_mfn_frame_list_list[k] = - virt_to_mfn(pfn_to_mfn_frame_list[k]); - j = 0; - } - pfn_to_mfn_frame_list[k][j] = - virt_to_mfn(&phys_to_machine_mapping[i]); - } HYPERVISOR_shared_info->arch.max_pfn = max_pfn; - HYPERVISOR_shared_info->arch.pfn_to_mfn_frame_list_list = - virt_to_mfn(pfn_to_mfn_frame_list_list); } #else /* !(defined(__i386__) || defined(__x86_64__)) */ diff -r 8bc64a3a5054 -r 6443c0465741 tools/libxc/xc_linux_restore.c --- a/tools/libxc/xc_linux_restore.c Mon Feb 5 16:40:19 2007 +++ b/tools/libxc/xc_linux_restore.c Tue Feb 6 14:42:34 2007 @@ -172,6 +172,9 @@ /* A copy of the pfn-to-mfn table frame list. */ xen_pfn_t *p2m_frame_list = NULL; + /* A temporary mapping of pfn-to-mfn table frame list */ + xen_pfn_t *live_p2m_fl = NULL; + /* A temporary mapping of the guest's start_info page. */ start_info_t *start_info; @@ -816,11 +819,41 @@ for ( i = 0; i < MAX_VIRT_CPUS; i++ ) shared_info->vcpu_info[i].evtchn_pending_sel = 0; - /* Copy saved contents of shared-info page. No checking needed. */ + /* + * Uncanonicalise fll pfn. Copy saved contents of shared-info page. + * No checking needed. + */ + shared_info->arch.pfn_to_mfn_frame_list_list = + p2m[shared_info->arch.pfn_to_mfn_frame_list_list]; page = xc_map_foreign_range( xc_handle, dom, PAGE_SIZE, PROT_WRITE, shared_info_frame); memcpy(page, shared_info, PAGE_SIZE); munmap(page, PAGE_SIZE); + + /* Update fll page. Uncanonicalise entries in fll */ + page = xc_map_foreign_range(xc_handle, dom, PAGE_SIZE, PROT_WRITE, + shared_info->arch.pfn_to_mfn_frame_list_list); + if (page == NULL) { + ERROR("Cant map fll page"); + goto out; + } + if (!read_exact(io_fd, page, PAGE_SIZE)) { + ERROR("Cant read fll page"); + goto out; + } + for (i = 0; i < P2M_FLL_ENTRIES; i++) { + xen_pfn_t *p = page; + p[i] = p2m[p[i]]; + } + + /* Map pfn-to-mfn frame list pages */ + live_p2m_fl = xc_map_foreign_batch(xc_handle, dom, PROT_WRITE, + page, P2M_FLL_ENTRIES); + munmap(page, PAGE_SIZE); + if (live_p2m_fl == NULL) { + ERROR("Cant map p2m frame-list pages"); + goto out; + } /* Uncanonicalise the pfn-to-mfn table frame-number list. */ for (i = 0; i < P2M_FL_ENTRIES; i++) { @@ -832,6 +865,13 @@ p2m_frame_list[i] = p2m[pfn]; } + + /* + * Copy the p2m_frame_list (local copy) + * to the live-p2m-frame-list + */ + memcpy(live_p2m_fl, p2m_frame_list, P2M_FL_ENTRIES * sizeof (xen_pfn_t)); + munmap(live_p2m_fl, P2M_FLL_ENTRIES * PAGE_SIZE); /* Copy the P2M we've constructed to the 'live' P2M */ if (!(live_p2m = xc_map_foreign_batch(xc_handle, dom, PROT_WRITE, diff -r 8bc64a3a5054 -r 6443c0465741 tools/libxc/xc_linux_save.c --- a/tools/libxc/xc_linux_save.c Mon Feb 5 16:40:19 2007 +++ b/tools/libxc/xc_linux_save.c Tue Feb 6 14:42:34 2007 @@ -426,34 +426,6 @@ } /* -** Map the top-level page of MFNs from the guest. The guest might not have -** finished resuming from a previous restore operation, so we wait a while for -** it to update the MFN to a reasonable value. -*/ -static void *map_frame_list_list(int xc_handle, uint32_t dom, - shared_info_t *shinfo) -{ - int count = 100; - void *p; - - while (count-- && shinfo->arch.pfn_to_mfn_frame_list_list == 0) - usleep(10000); - - if (shinfo->arch.pfn_to_mfn_frame_list_list == 0) { - ERROR("Timed out waiting for frame list updated."); - return NULL; - } - - p = xc_map_foreign_range(xc_handle, dom, PAGE_SIZE, PROT_READ, - shinfo->arch.pfn_to_mfn_frame_list_list); - - if (p == NULL) - ERROR("Couldn't map p2m_frame_list_list (errno %d)", errno); - - return p; -} - -/* ** During transfer (or in the state file), all page-table pages must be ** converted into a 'canonical' form where references to actual mfns ** are replaced with references to the corresponding pfns. @@ -616,7 +588,6 @@ } - int xc_linux_save(int xc_handle, int io_fd, uint32_t dom, uint32_t max_iters, uint32_t max_factor, uint32_t flags, int (*suspend)(int)) { @@ -714,12 +685,14 @@ } max_pfn = live_shinfo->arch.max_pfn; - - live_p2m_frame_list_list = map_frame_list_list(xc_handle, dom, - live_shinfo); - - if (!live_p2m_frame_list_list) - goto out; + live_p2m_frame_list_list = + xc_map_foreign_range(xc_handle, dom, PAGE_SIZE, PROT_READ, + live_shinfo->arch.pfn_to_mfn_frame_list_list); + + if (!live_p2m_frame_list_list) { + ERROR("Couldn't map p2m_frame_list_list (errno %d)", errno); + goto out; + } live_p2m_frame_list = xc_map_foreign_batch(xc_handle, dom, PROT_READ, @@ -1225,14 +1198,32 @@ xen_pfn_to_cr3(mfn_to_pfn(xen_cr3_to_pfn(ctxt.ctrlreg[3]))); /* - * Reset the MFN to be a known-invalid value. See map_frame_list_list(). + * Canonicalise fll mfn */ memcpy(page, live_shinfo, PAGE_SIZE); - ((shared_info_t *)page)->arch.pfn_to_mfn_frame_list_list = 0; + if (!translate_mfn_to_pfn( + &((shared_info_t *)page)->arch.pfn_to_mfn_frame_list_list)) { + ERROR("Cant canonicalise fll mfn"); + goto out; + } if (!write_exact(io_fd, &ctxt, sizeof(ctxt)) || !write_exact(io_fd, page, PAGE_SIZE)) { ERROR("Error when writing to state file (1) (errno %d)", errno); + goto out; + } + + /* canonicalise fll entries and send fll page */ + memcpy(page, live_p2m_frame_list_list, PAGE_SIZE); + for (i = 0; i < P2M_FLL_ENTRIES; i++) { + if (!translate_mfn_to_pfn(&((xen_pfn_t *)page)[i])) { + ERROR("Cant canonicalise fll entry %d", i); + goto out; + } + } + + if (!write_exact(io_fd, page, PAGE_SIZE)) { + ERROR("Cant write fll page to state file"); goto out; } > > 30 seconds is quite a time to wait... Wouldn't a second be more > > appropriate? > > > > While you're at it, I'd be grateful if you could move the shared_info > > assignment in linux's post_suspend() and setup_arch() [i386 and x86_64] > > below the list building code. > Sure. Patch below is for 3.0.4 but should essentially apply to > xen-unstable too. > > thanks > john [-- Attachment #2: save_restore.patch --] [-- Type: text/x-patch, Size: 7121 bytes --] # HG changeset patch # User srao@srikanth # Node ID 6443c0465741c05ffef24f5ca7c7fd3907089d04 # Parent 8bc64a3a505413e4d3bd7def06539ef088f2ca0c Avoid waiting for pfn-to-mfn-frame-list-list etc., to get initialized when doing a save after restore. Signed-off-by: Srikanth S. M. <srikanth_sm@symantec.com> diff -r 8bc64a3a5054 -r 6443c0465741 linux-2.6-xen-sparse/drivers/xen/core/machine_reboot.c --- a/linux-2.6-xen-sparse/drivers/xen/core/machine_reboot.c Mon Feb 5 16:40:19 2007 +++ b/linux-2.6-xen-sparse/drivers/xen/core/machine_reboot.c Tue Feb 6 14:42:34 2007 @@ -87,10 +87,7 @@ static void post_suspend(int suspend_cancelled) { - int i, j, k, fpp; extern unsigned long max_pfn; - extern unsigned long *pfn_to_mfn_frame_list_list; - extern unsigned long *pfn_to_mfn_frame_list[]; if (suspend_cancelled) { xen_start_info->store_mfn = @@ -105,20 +102,7 @@ memset(empty_zero_page, 0, PAGE_SIZE); - fpp = PAGE_SIZE/sizeof(unsigned long); - for (i = 0, j = 0, k = -1; i < max_pfn; i += fpp, j++) { - if ((j % fpp) == 0) { - k++; - pfn_to_mfn_frame_list_list[k] = - virt_to_mfn(pfn_to_mfn_frame_list[k]); - j = 0; - } - pfn_to_mfn_frame_list[k][j] = - virt_to_mfn(&phys_to_machine_mapping[i]); - } HYPERVISOR_shared_info->arch.max_pfn = max_pfn; - HYPERVISOR_shared_info->arch.pfn_to_mfn_frame_list_list = - virt_to_mfn(pfn_to_mfn_frame_list_list); } #else /* !(defined(__i386__) || defined(__x86_64__)) */ diff -r 8bc64a3a5054 -r 6443c0465741 tools/libxc/xc_linux_restore.c --- a/tools/libxc/xc_linux_restore.c Mon Feb 5 16:40:19 2007 +++ b/tools/libxc/xc_linux_restore.c Tue Feb 6 14:42:34 2007 @@ -172,6 +172,9 @@ /* A copy of the pfn-to-mfn table frame list. */ xen_pfn_t *p2m_frame_list = NULL; + /* A temporary mapping of pfn-to-mfn table frame list */ + xen_pfn_t *live_p2m_fl = NULL; + /* A temporary mapping of the guest's start_info page. */ start_info_t *start_info; @@ -816,11 +819,41 @@ for ( i = 0; i < MAX_VIRT_CPUS; i++ ) shared_info->vcpu_info[i].evtchn_pending_sel = 0; - /* Copy saved contents of shared-info page. No checking needed. */ + /* + * Uncanonicalise fll pfn. Copy saved contents of shared-info page. + * No checking needed. + */ + shared_info->arch.pfn_to_mfn_frame_list_list = + p2m[shared_info->arch.pfn_to_mfn_frame_list_list]; page = xc_map_foreign_range( xc_handle, dom, PAGE_SIZE, PROT_WRITE, shared_info_frame); memcpy(page, shared_info, PAGE_SIZE); munmap(page, PAGE_SIZE); + + /* Update fll page. Uncanonicalise entries in fll */ + page = xc_map_foreign_range(xc_handle, dom, PAGE_SIZE, PROT_WRITE, + shared_info->arch.pfn_to_mfn_frame_list_list); + if (page == NULL) { + ERROR("Cant map fll page"); + goto out; + } + if (!read_exact(io_fd, page, PAGE_SIZE)) { + ERROR("Cant read fll page"); + goto out; + } + for (i = 0; i < P2M_FLL_ENTRIES; i++) { + xen_pfn_t *p = page; + p[i] = p2m[p[i]]; + } + + /* Map pfn-to-mfn frame list pages */ + live_p2m_fl = xc_map_foreign_batch(xc_handle, dom, PROT_WRITE, + page, P2M_FLL_ENTRIES); + munmap(page, PAGE_SIZE); + if (live_p2m_fl == NULL) { + ERROR("Cant map p2m frame-list pages"); + goto out; + } /* Uncanonicalise the pfn-to-mfn table frame-number list. */ for (i = 0; i < P2M_FL_ENTRIES; i++) { @@ -832,6 +865,13 @@ p2m_frame_list[i] = p2m[pfn]; } + + /* + * Copy the p2m_frame_list (local copy) + * to the live-p2m-frame-list + */ + memcpy(live_p2m_fl, p2m_frame_list, P2M_FL_ENTRIES * sizeof (xen_pfn_t)); + munmap(live_p2m_fl, P2M_FLL_ENTRIES * PAGE_SIZE); /* Copy the P2M we've constructed to the 'live' P2M */ if (!(live_p2m = xc_map_foreign_batch(xc_handle, dom, PROT_WRITE, diff -r 8bc64a3a5054 -r 6443c0465741 tools/libxc/xc_linux_save.c --- a/tools/libxc/xc_linux_save.c Mon Feb 5 16:40:19 2007 +++ b/tools/libxc/xc_linux_save.c Tue Feb 6 14:42:34 2007 @@ -426,34 +426,6 @@ } /* -** Map the top-level page of MFNs from the guest. The guest might not have -** finished resuming from a previous restore operation, so we wait a while for -** it to update the MFN to a reasonable value. -*/ -static void *map_frame_list_list(int xc_handle, uint32_t dom, - shared_info_t *shinfo) -{ - int count = 100; - void *p; - - while (count-- && shinfo->arch.pfn_to_mfn_frame_list_list == 0) - usleep(10000); - - if (shinfo->arch.pfn_to_mfn_frame_list_list == 0) { - ERROR("Timed out waiting for frame list updated."); - return NULL; - } - - p = xc_map_foreign_range(xc_handle, dom, PAGE_SIZE, PROT_READ, - shinfo->arch.pfn_to_mfn_frame_list_list); - - if (p == NULL) - ERROR("Couldn't map p2m_frame_list_list (errno %d)", errno); - - return p; -} - -/* ** During transfer (or in the state file), all page-table pages must be ** converted into a 'canonical' form where references to actual mfns ** are replaced with references to the corresponding pfns. @@ -616,7 +588,6 @@ } - int xc_linux_save(int xc_handle, int io_fd, uint32_t dom, uint32_t max_iters, uint32_t max_factor, uint32_t flags, int (*suspend)(int)) { @@ -714,12 +685,14 @@ } max_pfn = live_shinfo->arch.max_pfn; - - live_p2m_frame_list_list = map_frame_list_list(xc_handle, dom, - live_shinfo); - - if (!live_p2m_frame_list_list) - goto out; + live_p2m_frame_list_list = + xc_map_foreign_range(xc_handle, dom, PAGE_SIZE, PROT_READ, + live_shinfo->arch.pfn_to_mfn_frame_list_list); + + if (!live_p2m_frame_list_list) { + ERROR("Couldn't map p2m_frame_list_list (errno %d)", errno); + goto out; + } live_p2m_frame_list = xc_map_foreign_batch(xc_handle, dom, PROT_READ, @@ -1225,14 +1198,32 @@ xen_pfn_to_cr3(mfn_to_pfn(xen_cr3_to_pfn(ctxt.ctrlreg[3]))); /* - * Reset the MFN to be a known-invalid value. See map_frame_list_list(). + * Canonicalise fll mfn */ memcpy(page, live_shinfo, PAGE_SIZE); - ((shared_info_t *)page)->arch.pfn_to_mfn_frame_list_list = 0; + if (!translate_mfn_to_pfn( + &((shared_info_t *)page)->arch.pfn_to_mfn_frame_list_list)) { + ERROR("Cant canonicalise fll mfn"); + goto out; + } if (!write_exact(io_fd, &ctxt, sizeof(ctxt)) || !write_exact(io_fd, page, PAGE_SIZE)) { ERROR("Error when writing to state file (1) (errno %d)", errno); + goto out; + } + + /* canonicalise fll entries and send fll page */ + memcpy(page, live_p2m_frame_list_list, PAGE_SIZE); + for (i = 0; i < P2M_FLL_ENTRIES; i++) { + if (!translate_mfn_to_pfn(&((xen_pfn_t *)page)[i])) { + ERROR("Cant canonicalise fll entry %d", i); + goto out; + } + } + + if (!write_exact(io_fd, page, PAGE_SIZE)) { + ERROR("Cant write fll page to state file"); goto out; } [-- Attachment #3: Type: text/plain, Size: 138 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] save/restore race 2007-02-06 15:04 ` Srikanth SM @ 2007-02-06 15:13 ` John Levon 0 siblings, 0 replies; 10+ messages in thread From: John Levon @ 2007-02-06 15:13 UTC (permalink / raw) To: Srikanth SM; +Cc: m+Ian.Pratt, xen-devel On Tue, Feb 06, 2007 at 08:34:21PM +0530, Srikanth SM wrote: > What I did was, simple-minded though: Instead of the guest building > p2m frame-list-list and the frame-list _after_ a restore, we build it from > dom0 _during_ restore. You broke compatibility... I have no idea why it was the guest's responsibility, but it's here to stay now. regards, john ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2007-02-06 15:13 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2007-01-23 22:01 save/restore race John Levon 2007-01-23 22:15 ` Ian Pratt 2007-01-23 22:48 ` John Levon 2007-01-23 23:38 ` Ian Pratt 2007-01-24 2:10 ` John Levon 2007-01-24 2:19 ` Ian Pratt 2007-01-24 3:13 ` [PATCH] " John Levon 2007-01-24 10:07 ` Steven Hand 2007-02-06 15:04 ` Srikanth SM 2007-02-06 15:13 ` John Levon
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.