Xen project Mailing List

On Sat, Nov 17, 2012 at 2:04 AM, Liu Jinsong <jinsong.liu@xxxxxxxxx> wrote:

Generally, there are 2 cases:
1. broken page occurs before migration
2. broken page occurs during migration

I think we probably need to include the "last iteration" subcases here as well -- although, probably it would be better to put them in the comments. Also make sure to note that now even suspend may do a second iteration if pages are broken during the suspend operation.

Also, what you should do is have the main description always say what the patch does, and then at the bottom have a summary of the changes; for instance:

v4:
- Adjust variables and patch description based on feedback
v3:
- Handle pages broken on the last iteration
&c

We had submitted 2 versions of patches. Their difference is how to handle case 2:
V1 patch aborted migration for the case 'broken page occurs during migration';
V2 patch marked broken page to dirty bitmap then got handled next round and no abort;

This is V3 patch, adding handle for vMCE occur at last iteration of migration:
At the sender
For case 1, the broken page will be mapped but not copied to target

(otherwise it may trigger more serious error, say, SRAR error).

While its pfn_type and pfn number will be transferred to target

so that target take appropriate action.

For case 2, mce handler marks the broken page to dirty bitmap, so that

at copypages stage of migration, its pfn_type and pfn number will be

transferred to target and then take appropriate action.

At the target

When migration target populates pages for guest. As for the case
of broken page wrt migration, we prefer to keep the type of the page,

for the sake of seamless migration.

At target it will set p2m as p2m_ram_broken for broken page. Guest MCE

may have good chance to handle its broken page, while if guest access

the broken page again it will kill itself as expected.

If vMCE occur at the last memory copy iteration of migration, we do one more
iteration so that the broken page's pfn_type and pfn number has chance to
be transferred to target.

Suggested-by: George Dunlap <george.dunlap@xxxxxxxxxxxxx>
Signed-off-by: Liu Jinsong <jinsong.liu@xxxxxxxxx>
Acked-by: George Dunlap <george.dunlap@xxxxxxxxxxxxx>

- for tools part of the patch
Acked-by: Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>

You should remove these until you get another ack (since the patch has changed materially)

- for hypervisor part of the patch
Acked-by: Jan Beulich <JBeulich@xxxxxxxx>

I think if you haven't changed the HV part since his ack, you can probably leave this one.

diff -r 8b93ac0c93f3 -r 446f6b9bfc89 tools/libxc/xc_domain.c

--- a/tools/libxc/xc_domain.c Tue Nov 13 11:19:17 2012 +0000

+++ b/tools/libxc/xc_domain.c Sat Nov 17 09:46:05 2012 +0800

@@ -283,6 +283,22 @@
return ret;
}

+/* set broken page p2m */
+int xc_set_broken_page_p2m(xc_interface *xch,
+ uint32_t domid,
+ unsigned long pfn)
+{
+ int ret;
+ DECLARE_DOMCTL;
+
+ domctl.cmd = XEN_DOMCTL_set_broken_page_p2m;
+ domctl.domain = (domid_t)domid;
+ domctl.u.set_broken_page_p2m.pfn = pfn;
+ ret = do_domctl(xch, &domctl);
+
+ return ret ? -1 : 0;
+}
+
/* get info from hvm guest for save */
int xc_domain_hvm_getcontext(xc_interface *xch,
uint32_t domid,

diff -r 8b93ac0c93f3 -r 446f6b9bfc89 tools/libxc/xc_domain_restore.c

--- a/tools/libxc/xc_domain_restore.c Tue Nov 13 11:19:17 2012 +0000

+++ b/tools/libxc/xc_domain_restore.c Sat Nov 17 09:46:05 2012 +0800

@@ -1023,9 +1023,15 @@

countpages = count;
for (i = oldcount; i < buf->nr_pages; ++i)
- if ((buf->pfn_types[i] & XEN_DOMCTL_PFINFO_LTAB_MASK) == XEN_DOMCTL_PFINFO_XTAB
- ||(buf->pfn_types[i] & XEN_DOMCTL_PFINFO_LTAB_MASK) == XEN_DOMCTL_PFINFO_XALLOC)
+ {
+ unsigned long pagetype;
+
+ pagetype = buf->pfn_types[i] & XEN_DOMCTL_PFINFO_LTAB_MASK;
+ if ( pagetype == XEN_DOMCTL_PFINFO_XTAB ||
+ pagetype == XEN_DOMCTL_PFINFO_BROKEN ||
+ pagetype == XEN_DOMCTL_PFINFO_XALLOC )
--countpages;
+ }

if (!countpages)
return count;
@@ -1267,6 +1273,17 @@
/* a bogus/unmapped/allocate-only page: skip it */
continue;

+ if ( pagetype == XEN_DOMCTL_PFINFO_BROKEN )
+ {
+ if ( xc_set_broken_page_p2m(xch, dom, pfn) )
+ {
+ ERROR("Set p2m for broken page failed, "
+ "dom=%d, pfn=%lx\n", dom, pfn);
+ goto err_mapped;
+ }
+ continue;
+ }
+
if (pfn_err[i])
{
ERROR("unexpected PFN mapping failure pfn %lx map_mfn %lx p2m_mfn %lx",

diff -r 8b93ac0c93f3 -r 446f6b9bfc89 tools/libxc/xc_domain_save.c

--- a/tools/libxc/xc_domain_save.c Tue Nov 13 11:19:17 2012 +0000

+++ b/tools/libxc/xc_domain_save.c Sat Nov 17 09:46:05 2012 +0800
@@ -1118,7 +1118,7 @@
/* Now write out each data page, canonicalising page tables as we go... */
for ( ; ; )
{
- unsigned int N, batch, run;
+ unsigned int N, batch, run, broken_page_num1, broken_page_num2;

You need more descriptive variables here. Maybe "broken_before_send" and "broken_after_send"?

char reportbuf[80];

snprintf(reportbuf, sizeof(reportbuf),
@@ -1270,6 +1270,7 @@
goto out;
}

+ broken_page_num1 = 0;
for ( run = j = 0; j < batch; j++ )
{
unsigned long gmfn = pfn_batch[j];
@@ -1277,6 +1278,14 @@

if ( !hvm )
gmfn = pfn_to_mfn(gmfn);

+ if ( pfn_type[j] == XEN_DOMCTL_PFINFO_BROKEN )
+ {
+ pfn_type[j] |= pfn_batch[j];

+ ++broken_page_num1;

+ ++run;
+ continue;
+ }
+
if ( pfn_err[j] )
{
if ( pfn_type[j] == XEN_DOMCTL_PFINFO_XTAB )

@@ -1371,8 +1380,12 @@

}
}

- /* skip pages that aren't present or are alloc-only */
+ /*
+ * skip pages that aren't present,
+ * or are broken, or are alloc-only
+ */
if ( pagetype == XEN_DOMCTL_PFINFO_XTAB
+ || pagetype == XEN_DOMCTL_PFINFO_BROKEN
|| pagetype == XEN_DOMCTL_PFINFO_XALLOC )
continue;

@@ -1484,6 +1497,35 @@

munmap(region_base, batch*PAGE_SIZE);

+ /*
+ * if vMCE occur at last iter, do one more iter so that it get
+ * chance to transfer broken page's pfn_type and pfn number to
+ * target and then take appropriate action
+ */

Maybe, "Pages may have broken between mapping them and sending them. Count the number of broken pages after sending, and if there are more than before sending, do another iteration to make sure the pages are marked broken on the receiving side"?

+ if ( last_iter )
+ {

/* Read the p2m table again */

+ for ( j = 0; j < batch; j++ )
+ {
+ if ( hvm )

+ pfn_type[j] = pfn_batch[j];

+ else
+ pfn_type[j] = pfn_to_mfn(pfn_batch[j]);
+ }
+
+ if ( xc_get_pfn_type_batch(xch, dom, batch, pfn_type) )
+ {
+ PERROR("get_pfn_type_batch failed");
+ goto out;
+ }
+

/* Count the number of broken pages */

+ broken_page_num2 = 0;
+ for ( j = 0; j < batch; j++ )

+ if ( pfn_type[j] == XEN_DOMCTL_PFINFO_BROKEN )

+ broken_page_num2++;
+

/* If there are more than before sending, do one more iteration */

+ if ( broken_page_num1 < broken_page_num2 )
+ last_iter = 0;
+ }
} /* end of this while loop for this iteration */

I think that's all I have.

-George

Re: [Xen-devel] [PATCH V3] X86/vMCE: handle broken page with regard to migration