Andrew Cooper
2013-Nov-25 20:40 UTC
[PATCH] tools/many: Fix Generation ID restore interface.
The original Generation ID code was submitted before the specification had
been finalised, and changed completely between the final draft and formal
release.
Most notably, the size of the Generation ID has doubled to 128 bits, and
changing it now involves writing a new cryptographically random number in the
appropriate location, rather than simply incrementing it.
The xc_domain_save() side of the code is fine, but the xc_domain_restore()
needs substantial changes to be usable.
This patch replaces the old xc_domain_restore() parameters with a new optional
restore_callback. If the callback is provided, and an appropriate hunk is
found in the migration stream, the appropriate piece of guest memory is mapped
and provided to the callback.
This patch also fixes all the in-tree callers of xc_domain_restore(). All
callers had the functionality disabled, and never exposed in a public way.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Ian Campbell <Ian.Campbell@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: George Dunlap <george.dunlap@eu.citrix.com>
---
George:
Despite this being a feature, I am requesting a freeze exemption. It is
functionality which was not used (or indeed useful) before, and remains that
way as far as in-tree consumers are concerned.
However, as there has been once recent xc_domain_restore() API change, it is
less disruptive to other users to do another API change before the 4.4
release rather than afterwards.
---
tools/libxc/xc_domain_restore.c | 68 +++++++++++++++++++-------------------
tools/libxc/xc_nomigrate.c | 3 +-
tools/libxc/xenguest.h | 24 +++++++++++---
tools/libxl/libxl_create.c | 2 +-
tools/libxl/libxl_internal.h | 3 +-
tools/libxl/libxl_save_callout.c | 5 ++-
tools/libxl/libxl_save_helper.c | 4 +--
tools/xcutils/xc_restore.c | 2 +-
8 files changed, 61 insertions(+), 50 deletions(-)
diff --git a/tools/libxc/xc_domain_restore.c b/tools/libxc/xc_domain_restore.c
index 80769a7..bb1f7fd 100644
--- a/tools/libxc/xc_domain_restore.c
+++ b/tools/libxc/xc_domain_restore.c
@@ -1414,8 +1414,7 @@ int xc_domain_restore(xc_interface *xch, int io_fd,
uint32_t dom,
domid_t store_domid, unsigned int console_evtchn,
unsigned long *console_mfn, domid_t console_domid,
unsigned int hvm, unsigned int pae, int superpages,
- int no_incr_generationid, int checkpointed_stream,
- unsigned long *vm_generationid_addr,
+ int checkpointed_stream,
struct restore_callbacks *callbacks)
{
DECLARE_DOMCTL;
@@ -1641,43 +1640,44 @@ int xc_domain_restore(xc_interface *xch, int io_fd,
uint32_t dom,
xc_set_hvm_param(xch, dom, HVM_PARAM_VM86_TSS,
pagebuf.vm86_tss);
if ( pagebuf.console_pfn )
console_pfn = pagebuf.console_pfn;
- if ( pagebuf.vm_generationid_addr ) {
- if ( !no_incr_generationid ) {
- unsigned int offset;
- unsigned char *buf;
- unsigned long long generationid;
+ if ( pagebuf.vm_generationid_addr &&
callbacks->generation_id ) {
+ unsigned int offset;
+ unsigned char *buf;
- /*
- * Map the VM generation id buffer and inject the new
value.
- */
-
- pfn = pagebuf.vm_generationid_addr >> PAGE_SHIFT;
- offset = pagebuf.vm_generationid_addr & (PAGE_SIZE -
1);
-
- if ( (pfn >= dinfo->p2m_size) ||
- (pfn_type[pfn] != XEN_DOMCTL_PFINFO_NOTAB) )
- {
- ERROR("generation id buffer frame is bad");
- goto out;
- }
-
- mfn = ctx->p2m[pfn];
- buf = xc_map_foreign_range(xch, dom, PAGE_SIZE,
- PROT_READ | PROT_WRITE, mfn);
- if ( buf == NULL )
- {
- ERROR("xc_map_foreign_range for generation
id"
- " buffer failed");
- goto out;
- }
+ pfn = pagebuf.vm_generationid_addr >> PAGE_SHIFT;
+ offset = pagebuf.vm_generationid_addr & (PAGE_SIZE - 1);
- generationid = *(unsigned long long *)(buf + offset);
- *(unsigned long long *)(buf + offset) = generationid + 1;
+ if ( pfn >= dinfo->p2m_size )
+ {
+ ERROR("generation id frame (pfn %#lx) outside p2m
(size %#lx)\n",
+ pfn, dinfo->p2m_size);
+ rc = -1;
+ goto out;
+ }
+ else if ( pfn_type[pfn] != XEN_DOMCTL_PFINFO_NOTAB )
+ {
+ ERROR("generation id frame (pfn %#lx) bad type
%#x",
+ pfn, pfn_type[pfn]);
+ rc = -1;
+ goto out;
+ }
- munmap(buf, PAGE_SIZE);
+ mfn = ctx->p2m[pfn];
+ buf = xc_map_foreign_range(xch, dom, PAGE_SIZE,
+ PROT_READ | PROT_WRITE, mfn);
+ if ( !buf )
+ {
+ PERROR("Failed to map generation id frame: (mfn
%#lx)", mfn);
+ rc = -1;
+ goto out;
}
- *vm_generationid_addr = pagebuf.vm_generationid_addr;
+ rc = callbacks->generation_id(dom,
pagebuf.vm_generationid_addr,
+ buf + offset,
callbacks->data);
+ munmap(buf, PAGE_SIZE);
+
+ if ( rc )
+ goto out;
}
break; /* our work here is done */
diff --git a/tools/libxc/xc_nomigrate.c b/tools/libxc/xc_nomigrate.c
index fb6d53e..8b4c5c8 100644
--- a/tools/libxc/xc_nomigrate.c
+++ b/tools/libxc/xc_nomigrate.c
@@ -35,8 +35,7 @@ int xc_domain_restore(xc_interface *xch, int io_fd, uint32_t
dom,
domid_t store_domid, unsigned int console_evtchn,
unsigned long *console_mfn, domid_t console_domid,
unsigned int hvm, unsigned int pae, int superpages,
- int no_incr_generationid, int checkpointed_stream,
- unsigned long *vm_generationid_addr,
+ int checkpointed_stream,
struct restore_callbacks *callbacks)
{
errno = ENOSYS;
diff --git a/tools/libxc/xenguest.h b/tools/libxc/xenguest.h
index a0e30e1..507bf65 100644
--- a/tools/libxc/xenguest.h
+++ b/tools/libxc/xenguest.h
@@ -96,6 +96,25 @@ struct restore_callbacks {
int (*toolstack_restore)(uint32_t domid, const uint8_t *buf,
uint32_t size, void* data);
+ /**
+ * Callback to allow the toolstack to manage a domain''s generation
ID.
+ * This function is called if a generation ID chunk is found in the
+ * migration stream, and the function pointer is provided.
+ *
+ * The generation ID location is a special chhunk in the migration stream.
+ * The toolstack must take a record of the generation ID address, to
+ * provide it back to xc_domain_save() in the future. It must also update
+ * the value of the generation id when appropriate.
+ *
+ * @param domid The domain id.
+ * @param genid_gpa Guest physical address of the generation id.
+ * @param mapped_genid Pointer to the generation id, mapped from the
domain.
+ * @param data General restore_callbacks data pointer.
+ * @returns 0 for success, -1 for error.
+ */
+ int (*generation_id)(uint32_t domid, uint64_t genid_gpa,
+ void *mapped_genid, void *data);
+
/* to be provided as the last argument to each callback function */
void* data;
};
@@ -113,9 +132,7 @@ struct restore_callbacks {
* @parm hvm non-zero if this is a HVM restore
* @parm pae non-zero if this HVM domain has PAE support enabled
* @parm superpages non-zero to allocate guest memory with superpages
- * @parm no_incr_generationid non-zero if generation id is NOT to be
incremented
* @parm checkpointed_stream non-zero if the far end of the stream is using
checkpointing
- * @parm vm_generationid_addr returned with the address of the generation id
buffer
* @parm callbacks non-NULL to receive a callback to restore toolstack
* specific data
* @return 0 on success, -1 on failure
@@ -125,8 +142,7 @@ int xc_domain_restore(xc_interface *xch, int io_fd, uint32_t
dom,
domid_t store_domid, unsigned int console_evtchn,
unsigned long *console_mfn, domid_t console_domid,
unsigned int hvm, unsigned int pae, int superpages,
- int no_incr_generationid, int checkpointed_stream,
- unsigned long *vm_generationid_addr,
+ int checkpointed_stream,
struct restore_callbacks *callbacks);
/**
* xc_domain_restore writes a file to disk that contains the device
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index fe7ba0d..4cd4205 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -838,7 +838,7 @@ static void domcreate_bootloader_done(libxl__egc *egc,
goto out;
}
libxl__xc_domain_restore(egc, dcs,
- hvm, pae, superpages, 1);
+ hvm, pae, superpages);
return;
out:
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index a2d8247..d84890b 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -2614,8 +2614,7 @@ _hidden int libxl__toolstack_save(uint32_t domid, uint8_t
**buf,
/* calls libxl__xc_domain_restore_done when done */
_hidden void libxl__xc_domain_restore(libxl__egc *egc,
libxl__domain_create_state *dcs,
- int hvm, int pae, int superpages,
- int no_incr_generationid);
+ int hvm, int pae, int superpages);
/* If rc==0 then retval is the return value from xc_domain_save
* and errnoval is the errno value it provided.
* If rc!=0, retval and errnoval are undefined. */
diff --git a/tools/libxl/libxl_save_callout.c b/tools/libxl/libxl_save_callout.c
index 6e45b2f..0121cc6 100644
--- a/tools/libxl/libxl_save_callout.c
+++ b/tools/libxl/libxl_save_callout.c
@@ -41,8 +41,7 @@ static void helper_done(libxl__egc *egc,
libxl__save_helper_state *shs);
/*----- entrypoints -----*/
void libxl__xc_domain_restore(libxl__egc *egc, libxl__domain_create_state *dcs,
- int hvm, int pae, int superpages,
- int no_incr_generationid)
+ int hvm, int pae, int superpages)
{
STATE_AO_GC(dcs->ao);
@@ -59,7 +58,7 @@ void libxl__xc_domain_restore(libxl__egc *egc,
libxl__domain_create_state *dcs,
state->store_port,
state->store_domid, state->console_port,
state->console_domid,
- hvm, pae, superpages, no_incr_generationid,
+ hvm, pae, superpages,
cbflags, dcs->checkpointed_stream,
};
diff --git a/tools/libxl/libxl_save_helper.c b/tools/libxl/libxl_save_helper.c
index 880565e..0df97dc 100644
--- a/tools/libxl/libxl_save_helper.c
+++ b/tools/libxl/libxl_save_helper.c
@@ -250,7 +250,6 @@ int main(int argc, char **argv)
unsigned int hvm = strtoul(NEXTARG,0,10);
unsigned int pae = strtoul(NEXTARG,0,10);
int superpages = strtoul(NEXTARG,0,10);
- int no_incr_genidad = strtoul(NEXTARG,0,10);
unsigned cbflags = strtoul(NEXTARG,0,10);
int checkpointed = strtoul(NEXTARG,0,10);
assert(!*++argv);
@@ -265,8 +264,7 @@ int main(int argc, char **argv)
r = xc_domain_restore(xch, io_fd, dom, store_evtchn, &store_mfn,
store_domid, console_evtchn, &console_mfn,
console_domid, hvm, pae, superpages,
- no_incr_genidad, checkpointed, &genidad,
- &helper_restore_callbacks);
+ checkpointed, &helper_restore_callbacks);
helper_stub_restore_results(store_mfn,console_mfn,genidad,0);
complete(r);
diff --git a/tools/xcutils/xc_restore.c b/tools/xcutils/xc_restore.c
index 4ea261b..39d9efb 100644
--- a/tools/xcutils/xc_restore.c
+++ b/tools/xcutils/xc_restore.c
@@ -56,7 +56,7 @@ main(int argc, char **argv)
ret = xc_domain_restore(xch, io_fd, domid, store_evtchn, &store_mfn, 0,
console_evtchn, &console_mfn, 0, hvm, pae,
superpages,
- 0, checkpointed, NULL, NULL);
+ checkpointed, NULL);
if ( ret == 0 )
{
--
1.7.10.4
Ian Campbell
2013-Nov-26 10:11 UTC
Re: [PATCH] tools/many: Fix Generation ID restore interface.
On Mon, 2013-11-25 at 20:40 +0000, Andrew Cooper wrote:> The original Generation ID code was submitted before the specification had > been finalised, and changed completely between the final draft and formal > release. > > Most notably, the size of the Generation ID has doubled to 128 bits, and > changing it now involves writing a new cryptographically random number in the > appropriate location, rather than simply incrementing it. > > The xc_domain_save() side of the code is fine, but the xc_domain_restore() > needs substantial changes to be usable. > > This patch replaces the old xc_domain_restore() parameters with a new optional > restore_callback. If the callback is provided, and an appropriate hunk is > found in the migration stream, the appropriate piece of guest memory is mapped > and provided to the callback. > > This patch also fixes all the in-tree callers of xc_domain_restore(). All > callers had the functionality disabled, and never exposed in a public way. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > CC: Ian Campbell <Ian.Campbell@citrix.com> > CC: Ian Jackson <Ian.Jackson@eu.citrix.com> > CC: George Dunlap <george.dunlap@eu.citrix.com> > > --- > > George: > > Despite this being a feature, I am requesting a freeze exemption. It is > functionality which was not used (or indeed useful) before, and remains that > way as far as in-tree consumers are concerned.If it is both wrong and neither used nor useful why aren''t we ripping it out?> However, as there has been once recent xc_domain_restore() API change, it is > less disruptive to other users to do another API change before the 4.4 > release rather than afterwards.I don''t buy this argument, history suggests that there will almost certainly be an API change in 4.5 too and in any case we don''t make any guarantees about the libxc API.> --- > tools/libxc/xc_domain_restore.c | 68 +++++++++++++++++++------------------- > tools/libxc/xc_nomigrate.c | 3 +- > tools/libxc/xenguest.h | 24 +++++++++++--- > tools/libxl/libxl_create.c | 2 +- > tools/libxl/libxl_internal.h | 3 +- > tools/libxl/libxl_save_callout.c | 5 ++- > tools/libxl/libxl_save_helper.c | 4 +-- > tools/xcutils/xc_restore.c | 2 +- > 8 files changed, 61 insertions(+), 50 deletions(-) > > diff --git a/tools/libxc/xc_domain_restore.c b/tools/libxc/xc_domain_restore.c > index 80769a7..bb1f7fd 100644 > --- a/tools/libxc/xc_domain_restore.c > +++ b/tools/libxc/xc_domain_restore.c > @@ -1414,8 +1414,7 @@ int xc_domain_restore(xc_interface *xch, int io_fd, uint32_t dom, > domid_t store_domid, unsigned int console_evtchn, > unsigned long *console_mfn, domid_t console_domid, > unsigned int hvm, unsigned int pae, int superpages, > - int no_incr_generationid, int checkpointed_stream, > - unsigned long *vm_generationid_addr, > + int checkpointed_stream, > struct restore_callbacks *callbacks) > { > DECLARE_DOMCTL; > @@ -1641,43 +1640,44 @@ int xc_domain_restore(xc_interface *xch, int io_fd, uint32_t dom, > xc_set_hvm_param(xch, dom, HVM_PARAM_VM86_TSS, pagebuf.vm86_tss); > if ( pagebuf.console_pfn ) > console_pfn = pagebuf.console_pfn; > - if ( pagebuf.vm_generationid_addr ) { > - if ( !no_incr_generationid ) { > - unsigned int offset; > - unsigned char *buf; > - unsigned long long generationid; > + if ( pagebuf.vm_generationid_addr && callbacks->generation_id ) { > + unsigned int offset; > + unsigned char *buf; > > - /* > - * Map the VM generation id buffer and inject the new value. > - */ > - > - pfn = pagebuf.vm_generationid_addr >> PAGE_SHIFT; > - offset = pagebuf.vm_generationid_addr & (PAGE_SIZE - 1); > - > - if ( (pfn >= dinfo->p2m_size) || > - (pfn_type[pfn] != XEN_DOMCTL_PFINFO_NOTAB) ) > - { > - ERROR("generation id buffer frame is bad"); > - goto out; > - } > - > - mfn = ctx->p2m[pfn]; > - buf = xc_map_foreign_range(xch, dom, PAGE_SIZE, > - PROT_READ | PROT_WRITE, mfn); > - if ( buf == NULL ) > - { > - ERROR("xc_map_foreign_range for generation id" > - " buffer failed"); > - goto out; > - } > + pfn = pagebuf.vm_generationid_addr >> PAGE_SHIFT; > + offset = pagebuf.vm_generationid_addr & (PAGE_SIZE - 1); > > - generationid = *(unsigned long long *)(buf + offset); > - *(unsigned long long *)(buf + offset) = generationid + 1; > + if ( pfn >= dinfo->p2m_size ) > + { > + ERROR("generation id frame (pfn %#lx) outside p2m (size %#lx)\n", > + pfn, dinfo->p2m_size); > + rc = -1; > + goto out; > + } > + else if ( pfn_type[pfn] != XEN_DOMCTL_PFINFO_NOTAB ) > + { > + ERROR("generation id frame (pfn %#lx) bad type %#x", > + pfn, pfn_type[pfn]); > + rc = -1; > + goto out; > + } > > - munmap(buf, PAGE_SIZE); > + mfn = ctx->p2m[pfn]; > + buf = xc_map_foreign_range(xch, dom, PAGE_SIZE, > + PROT_READ | PROT_WRITE, mfn); > + if ( !buf ) > + { > + PERROR("Failed to map generation id frame: (mfn %#lx)", mfn); > + rc = -1; > + goto out; > } > > - *vm_generationid_addr = pagebuf.vm_generationid_addr; > + rc = callbacks->generation_id(dom, pagebuf.vm_generationid_addr, > + buf + offset, callbacks->data); > + munmap(buf, PAGE_SIZE); > + > + if ( rc ) > + goto out; > } > > break; /* our work here is done */ > diff --git a/tools/libxc/xc_nomigrate.c b/tools/libxc/xc_nomigrate.c > index fb6d53e..8b4c5c8 100644 > --- a/tools/libxc/xc_nomigrate.c > +++ b/tools/libxc/xc_nomigrate.c > @@ -35,8 +35,7 @@ int xc_domain_restore(xc_interface *xch, int io_fd, uint32_t dom, > domid_t store_domid, unsigned int console_evtchn, > unsigned long *console_mfn, domid_t console_domid, > unsigned int hvm, unsigned int pae, int superpages, > - int no_incr_generationid, int checkpointed_stream, > - unsigned long *vm_generationid_addr, > + int checkpointed_stream, > struct restore_callbacks *callbacks) > { > errno = ENOSYS; > diff --git a/tools/libxc/xenguest.h b/tools/libxc/xenguest.h > index a0e30e1..507bf65 100644 > --- a/tools/libxc/xenguest.h > +++ b/tools/libxc/xenguest.h > @@ -96,6 +96,25 @@ struct restore_callbacks { > int (*toolstack_restore)(uint32_t domid, const uint8_t *buf, > uint32_t size, void* data); > > + /** > + * Callback to allow the toolstack to manage a domain''s generation ID. > + * This function is called if a generation ID chunk is found in the > + * migration stream, and the function pointer is provided. > + * > + * The generation ID location is a special chhunk in the migration stream. > + * The toolstack must take a record of the generation ID address, to > + * provide it back to xc_domain_save() in the future. It must also update > + * the value of the generation id when appropriate. > + * > + * @param domid The domain id. > + * @param genid_gpa Guest physical address of the generation id. > + * @param mapped_genid Pointer to the generation id, mapped from the domain. > + * @param data General restore_callbacks data pointer. > + * @returns 0 for success, -1 for error. > + */ > + int (*generation_id)(uint32_t domid, uint64_t genid_gpa, > + void *mapped_genid, void *data); > + > /* to be provided as the last argument to each callback function */ > void* data; > }; > @@ -113,9 +132,7 @@ struct restore_callbacks { > * @parm hvm non-zero if this is a HVM restore > * @parm pae non-zero if this HVM domain has PAE support enabled > * @parm superpages non-zero to allocate guest memory with superpages > - * @parm no_incr_generationid non-zero if generation id is NOT to be incremented > * @parm checkpointed_stream non-zero if the far end of the stream is using checkpointing > - * @parm vm_generationid_addr returned with the address of the generation id buffer > * @parm callbacks non-NULL to receive a callback to restore toolstack > * specific data > * @return 0 on success, -1 on failure > @@ -125,8 +142,7 @@ int xc_domain_restore(xc_interface *xch, int io_fd, uint32_t dom, > domid_t store_domid, unsigned int console_evtchn, > unsigned long *console_mfn, domid_t console_domid, > unsigned int hvm, unsigned int pae, int superpages, > - int no_incr_generationid, int checkpointed_stream, > - unsigned long *vm_generationid_addr, > + int checkpointed_stream, > struct restore_callbacks *callbacks); > /** > * xc_domain_restore writes a file to disk that contains the device > diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c > index fe7ba0d..4cd4205 100644 > --- a/tools/libxl/libxl_create.c > +++ b/tools/libxl/libxl_create.c > @@ -838,7 +838,7 @@ static void domcreate_bootloader_done(libxl__egc *egc, > goto out; > } > libxl__xc_domain_restore(egc, dcs, > - hvm, pae, superpages, 1); > + hvm, pae, superpages); > return; > > out: > diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h > index a2d8247..d84890b 100644 > --- a/tools/libxl/libxl_internal.h > +++ b/tools/libxl/libxl_internal.h > @@ -2614,8 +2614,7 @@ _hidden int libxl__toolstack_save(uint32_t domid, uint8_t **buf, > /* calls libxl__xc_domain_restore_done when done */ > _hidden void libxl__xc_domain_restore(libxl__egc *egc, > libxl__domain_create_state *dcs, > - int hvm, int pae, int superpages, > - int no_incr_generationid); > + int hvm, int pae, int superpages); > /* If rc==0 then retval is the return value from xc_domain_save > * and errnoval is the errno value it provided. > * If rc!=0, retval and errnoval are undefined. */ > diff --git a/tools/libxl/libxl_save_callout.c b/tools/libxl/libxl_save_callout.c > index 6e45b2f..0121cc6 100644 > --- a/tools/libxl/libxl_save_callout.c > +++ b/tools/libxl/libxl_save_callout.c > @@ -41,8 +41,7 @@ static void helper_done(libxl__egc *egc, libxl__save_helper_state *shs); > /*----- entrypoints -----*/ > > void libxl__xc_domain_restore(libxl__egc *egc, libxl__domain_create_state *dcs, > - int hvm, int pae, int superpages, > - int no_incr_generationid) > + int hvm, int pae, int superpages) > { > STATE_AO_GC(dcs->ao); > > @@ -59,7 +58,7 @@ void libxl__xc_domain_restore(libxl__egc *egc, libxl__domain_create_state *dcs, > state->store_port, > state->store_domid, state->console_port, > state->console_domid, > - hvm, pae, superpages, no_incr_generationid, > + hvm, pae, superpages, > cbflags, dcs->checkpointed_stream, > }; > > diff --git a/tools/libxl/libxl_save_helper.c b/tools/libxl/libxl_save_helper.c > index 880565e..0df97dc 100644 > --- a/tools/libxl/libxl_save_helper.c > +++ b/tools/libxl/libxl_save_helper.c > @@ -250,7 +250,6 @@ int main(int argc, char **argv) > unsigned int hvm = strtoul(NEXTARG,0,10); > unsigned int pae = strtoul(NEXTARG,0,10); > int superpages = strtoul(NEXTARG,0,10); > - int no_incr_genidad = strtoul(NEXTARG,0,10); > unsigned cbflags = strtoul(NEXTARG,0,10); > int checkpointed = strtoul(NEXTARG,0,10); > assert(!*++argv); > @@ -265,8 +264,7 @@ int main(int argc, char **argv) > r = xc_domain_restore(xch, io_fd, dom, store_evtchn, &store_mfn, > store_domid, console_evtchn, &console_mfn, > console_domid, hvm, pae, superpages, > - no_incr_genidad, checkpointed, &genidad, > - &helper_restore_callbacks); > + checkpointed, &helper_restore_callbacks); > helper_stub_restore_results(store_mfn,console_mfn,genidad,0); > complete(r); > > diff --git a/tools/xcutils/xc_restore.c b/tools/xcutils/xc_restore.c > index 4ea261b..39d9efb 100644 > --- a/tools/xcutils/xc_restore.c > +++ b/tools/xcutils/xc_restore.c > @@ -56,7 +56,7 @@ main(int argc, char **argv) > > ret = xc_domain_restore(xch, io_fd, domid, store_evtchn, &store_mfn, 0, > console_evtchn, &console_mfn, 0, hvm, pae, superpages, > - 0, checkpointed, NULL, NULL); > + checkpointed, NULL); > > if ( ret == 0 ) > {
Andrew Cooper
2013-Nov-26 10:41 UTC
Re: [PATCH] tools/many: Fix Generation ID restore interface.
On 26/11/13 10:11, Ian Campbell wrote:> On Mon, 2013-11-25 at 20:40 +0000, Andrew Cooper wrote: >> The original Generation ID code was submitted before the specification had >> been finalised, and changed completely between the final draft and formal >> release. >> >> Most notably, the size of the Generation ID has doubled to 128 bits, and >> changing it now involves writing a new cryptographically random number in the >> appropriate location, rather than simply incrementing it. >> >> The xc_domain_save() side of the code is fine, but the xc_domain_restore() >> needs substantial changes to be usable. >> >> This patch replaces the old xc_domain_restore() parameters with a new optional >> restore_callback. If the callback is provided, and an appropriate hunk is >> found in the migration stream, the appropriate piece of guest memory is mapped >> and provided to the callback. >> >> This patch also fixes all the in-tree callers of xc_domain_restore(). All >> callers had the functionality disabled, and never exposed in a public way. >> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >> CC: Ian Campbell <Ian.Campbell@citrix.com> >> CC: Ian Jackson <Ian.Jackson@eu.citrix.com> >> CC: George Dunlap <george.dunlap@eu.citrix.com> >> >> --- >> >> George: >> >> Despite this being a feature, I am requesting a freeze exemption. It is >> functionality which was not used (or indeed useful) before, and remains that >> way as far as in-tree consumers are concerned. > If it is both wrong and neither used nor useful why aren''t we ripping > it out?The implementation in the tree currently is neither useful nor used. It is hardwired to disabled for each intree caller, which is why this change wont affect any current functionality in tree. ~Andrew> >> However, as there has been once recent xc_domain_restore() API change, it is >> less disruptive to other users to do another API change before the 4.4 >> release rather than afterwards. > I don''t buy this argument, history suggests that there will almost > certainly be an API change in 4.5 too and in any case we don''t make any > guarantees about the libxc API. > >> --- >> tools/libxc/xc_domain_restore.c | 68 +++++++++++++++++++------------------- >> tools/libxc/xc_nomigrate.c | 3 +- >> tools/libxc/xenguest.h | 24 +++++++++++--- >> tools/libxl/libxl_create.c | 2 +- >> tools/libxl/libxl_internal.h | 3 +- >> tools/libxl/libxl_save_callout.c | 5 ++- >> tools/libxl/libxl_save_helper.c | 4 +-- >> tools/xcutils/xc_restore.c | 2 +- >> 8 files changed, 61 insertions(+), 50 deletions(-) >> >> diff --git a/tools/libxc/xc_domain_restore.c b/tools/libxc/xc_domain_restore.c >> index 80769a7..bb1f7fd 100644 >> --- a/tools/libxc/xc_domain_restore.c >> +++ b/tools/libxc/xc_domain_restore.c >> @@ -1414,8 +1414,7 @@ int xc_domain_restore(xc_interface *xch, int io_fd, uint32_t dom, >> domid_t store_domid, unsigned int console_evtchn, >> unsigned long *console_mfn, domid_t console_domid, >> unsigned int hvm, unsigned int pae, int superpages, >> - int no_incr_generationid, int checkpointed_stream, >> - unsigned long *vm_generationid_addr, >> + int checkpointed_stream, >> struct restore_callbacks *callbacks) >> { >> DECLARE_DOMCTL; >> @@ -1641,43 +1640,44 @@ int xc_domain_restore(xc_interface *xch, int io_fd, uint32_t dom, >> xc_set_hvm_param(xch, dom, HVM_PARAM_VM86_TSS, pagebuf.vm86_tss); >> if ( pagebuf.console_pfn ) >> console_pfn = pagebuf.console_pfn; >> - if ( pagebuf.vm_generationid_addr ) { >> - if ( !no_incr_generationid ) { >> - unsigned int offset; >> - unsigned char *buf; >> - unsigned long long generationid; >> + if ( pagebuf.vm_generationid_addr && callbacks->generation_id ) { >> + unsigned int offset; >> + unsigned char *buf; >> >> - /* >> - * Map the VM generation id buffer and inject the new value. >> - */ >> - >> - pfn = pagebuf.vm_generationid_addr >> PAGE_SHIFT; >> - offset = pagebuf.vm_generationid_addr & (PAGE_SIZE - 1); >> - >> - if ( (pfn >= dinfo->p2m_size) || >> - (pfn_type[pfn] != XEN_DOMCTL_PFINFO_NOTAB) ) >> - { >> - ERROR("generation id buffer frame is bad"); >> - goto out; >> - } >> - >> - mfn = ctx->p2m[pfn]; >> - buf = xc_map_foreign_range(xch, dom, PAGE_SIZE, >> - PROT_READ | PROT_WRITE, mfn); >> - if ( buf == NULL ) >> - { >> - ERROR("xc_map_foreign_range for generation id" >> - " buffer failed"); >> - goto out; >> - } >> + pfn = pagebuf.vm_generationid_addr >> PAGE_SHIFT; >> + offset = pagebuf.vm_generationid_addr & (PAGE_SIZE - 1); >> >> - generationid = *(unsigned long long *)(buf + offset); >> - *(unsigned long long *)(buf + offset) = generationid + 1; >> + if ( pfn >= dinfo->p2m_size ) >> + { >> + ERROR("generation id frame (pfn %#lx) outside p2m (size %#lx)\n", >> + pfn, dinfo->p2m_size); >> + rc = -1; >> + goto out; >> + } >> + else if ( pfn_type[pfn] != XEN_DOMCTL_PFINFO_NOTAB ) >> + { >> + ERROR("generation id frame (pfn %#lx) bad type %#x", >> + pfn, pfn_type[pfn]); >> + rc = -1; >> + goto out; >> + } >> >> - munmap(buf, PAGE_SIZE); >> + mfn = ctx->p2m[pfn]; >> + buf = xc_map_foreign_range(xch, dom, PAGE_SIZE, >> + PROT_READ | PROT_WRITE, mfn); >> + if ( !buf ) >> + { >> + PERROR("Failed to map generation id frame: (mfn %#lx)", mfn); >> + rc = -1; >> + goto out; >> } >> >> - *vm_generationid_addr = pagebuf.vm_generationid_addr; >> + rc = callbacks->generation_id(dom, pagebuf.vm_generationid_addr, >> + buf + offset, callbacks->data); >> + munmap(buf, PAGE_SIZE); >> + >> + if ( rc ) >> + goto out; >> } >> >> break; /* our work here is done */ >> diff --git a/tools/libxc/xc_nomigrate.c b/tools/libxc/xc_nomigrate.c >> index fb6d53e..8b4c5c8 100644 >> --- a/tools/libxc/xc_nomigrate.c >> +++ b/tools/libxc/xc_nomigrate.c >> @@ -35,8 +35,7 @@ int xc_domain_restore(xc_interface *xch, int io_fd, uint32_t dom, >> domid_t store_domid, unsigned int console_evtchn, >> unsigned long *console_mfn, domid_t console_domid, >> unsigned int hvm, unsigned int pae, int superpages, >> - int no_incr_generationid, int checkpointed_stream, >> - unsigned long *vm_generationid_addr, >> + int checkpointed_stream, >> struct restore_callbacks *callbacks) >> { >> errno = ENOSYS; >> diff --git a/tools/libxc/xenguest.h b/tools/libxc/xenguest.h >> index a0e30e1..507bf65 100644 >> --- a/tools/libxc/xenguest.h >> +++ b/tools/libxc/xenguest.h >> @@ -96,6 +96,25 @@ struct restore_callbacks { >> int (*toolstack_restore)(uint32_t domid, const uint8_t *buf, >> uint32_t size, void* data); >> >> + /** >> + * Callback to allow the toolstack to manage a domain''s generation ID. >> + * This function is called if a generation ID chunk is found in the >> + * migration stream, and the function pointer is provided. >> + * >> + * The generation ID location is a special chhunk in the migration stream. >> + * The toolstack must take a record of the generation ID address, to >> + * provide it back to xc_domain_save() in the future. It must also update >> + * the value of the generation id when appropriate. >> + * >> + * @param domid The domain id. >> + * @param genid_gpa Guest physical address of the generation id. >> + * @param mapped_genid Pointer to the generation id, mapped from the domain. >> + * @param data General restore_callbacks data pointer. >> + * @returns 0 for success, -1 for error. >> + */ >> + int (*generation_id)(uint32_t domid, uint64_t genid_gpa, >> + void *mapped_genid, void *data); >> + >> /* to be provided as the last argument to each callback function */ >> void* data; >> }; >> @@ -113,9 +132,7 @@ struct restore_callbacks { >> * @parm hvm non-zero if this is a HVM restore >> * @parm pae non-zero if this HVM domain has PAE support enabled >> * @parm superpages non-zero to allocate guest memory with superpages >> - * @parm no_incr_generationid non-zero if generation id is NOT to be incremented >> * @parm checkpointed_stream non-zero if the far end of the stream is using checkpointing >> - * @parm vm_generationid_addr returned with the address of the generation id buffer >> * @parm callbacks non-NULL to receive a callback to restore toolstack >> * specific data >> * @return 0 on success, -1 on failure >> @@ -125,8 +142,7 @@ int xc_domain_restore(xc_interface *xch, int io_fd, uint32_t dom, >> domid_t store_domid, unsigned int console_evtchn, >> unsigned long *console_mfn, domid_t console_domid, >> unsigned int hvm, unsigned int pae, int superpages, >> - int no_incr_generationid, int checkpointed_stream, >> - unsigned long *vm_generationid_addr, >> + int checkpointed_stream, >> struct restore_callbacks *callbacks); >> /** >> * xc_domain_restore writes a file to disk that contains the device >> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c >> index fe7ba0d..4cd4205 100644 >> --- a/tools/libxl/libxl_create.c >> +++ b/tools/libxl/libxl_create.c >> @@ -838,7 +838,7 @@ static void domcreate_bootloader_done(libxl__egc *egc, >> goto out; >> } >> libxl__xc_domain_restore(egc, dcs, >> - hvm, pae, superpages, 1); >> + hvm, pae, superpages); >> return; >> >> out: >> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h >> index a2d8247..d84890b 100644 >> --- a/tools/libxl/libxl_internal.h >> +++ b/tools/libxl/libxl_internal.h >> @@ -2614,8 +2614,7 @@ _hidden int libxl__toolstack_save(uint32_t domid, uint8_t **buf, >> /* calls libxl__xc_domain_restore_done when done */ >> _hidden void libxl__xc_domain_restore(libxl__egc *egc, >> libxl__domain_create_state *dcs, >> - int hvm, int pae, int superpages, >> - int no_incr_generationid); >> + int hvm, int pae, int superpages); >> /* If rc==0 then retval is the return value from xc_domain_save >> * and errnoval is the errno value it provided. >> * If rc!=0, retval and errnoval are undefined. */ >> diff --git a/tools/libxl/libxl_save_callout.c b/tools/libxl/libxl_save_callout.c >> index 6e45b2f..0121cc6 100644 >> --- a/tools/libxl/libxl_save_callout.c >> +++ b/tools/libxl/libxl_save_callout.c >> @@ -41,8 +41,7 @@ static void helper_done(libxl__egc *egc, libxl__save_helper_state *shs); >> /*----- entrypoints -----*/ >> >> void libxl__xc_domain_restore(libxl__egc *egc, libxl__domain_create_state *dcs, >> - int hvm, int pae, int superpages, >> - int no_incr_generationid) >> + int hvm, int pae, int superpages) >> { >> STATE_AO_GC(dcs->ao); >> >> @@ -59,7 +58,7 @@ void libxl__xc_domain_restore(libxl__egc *egc, libxl__domain_create_state *dcs, >> state->store_port, >> state->store_domid, state->console_port, >> state->console_domid, >> - hvm, pae, superpages, no_incr_generationid, >> + hvm, pae, superpages, >> cbflags, dcs->checkpointed_stream, >> }; >> >> diff --git a/tools/libxl/libxl_save_helper.c b/tools/libxl/libxl_save_helper.c >> index 880565e..0df97dc 100644 >> --- a/tools/libxl/libxl_save_helper.c >> +++ b/tools/libxl/libxl_save_helper.c >> @@ -250,7 +250,6 @@ int main(int argc, char **argv) >> unsigned int hvm = strtoul(NEXTARG,0,10); >> unsigned int pae = strtoul(NEXTARG,0,10); >> int superpages = strtoul(NEXTARG,0,10); >> - int no_incr_genidad = strtoul(NEXTARG,0,10); >> unsigned cbflags = strtoul(NEXTARG,0,10); >> int checkpointed = strtoul(NEXTARG,0,10); >> assert(!*++argv); >> @@ -265,8 +264,7 @@ int main(int argc, char **argv) >> r = xc_domain_restore(xch, io_fd, dom, store_evtchn, &store_mfn, >> store_domid, console_evtchn, &console_mfn, >> console_domid, hvm, pae, superpages, >> - no_incr_genidad, checkpointed, &genidad, >> - &helper_restore_callbacks); >> + checkpointed, &helper_restore_callbacks); >> helper_stub_restore_results(store_mfn,console_mfn,genidad,0); >> complete(r); >> >> diff --git a/tools/xcutils/xc_restore.c b/tools/xcutils/xc_restore.c >> index 4ea261b..39d9efb 100644 >> --- a/tools/xcutils/xc_restore.c >> +++ b/tools/xcutils/xc_restore.c >> @@ -56,7 +56,7 @@ main(int argc, char **argv) >> >> ret = xc_domain_restore(xch, io_fd, domid, store_evtchn, &store_mfn, 0, >> console_evtchn, &console_mfn, 0, hvm, pae, superpages, >> - 0, checkpointed, NULL, NULL); >> + checkpointed, NULL); >> >> if ( ret == 0 ) >> { >
Ian Campbell
2013-Nov-26 10:45 UTC
Re: [PATCH] tools/many: Fix Generation ID restore interface.
On Tue, 2013-11-26 at 10:41 +0000, Andrew Cooper wrote:> On 26/11/13 10:11, Ian Campbell wrote: > > On Mon, 2013-11-25 at 20:40 +0000, Andrew Cooper wrote: > >> The original Generation ID code was submitted before the specification had > >> been finalised, and changed completely between the final draft and formal > >> release. > >> > >> Most notably, the size of the Generation ID has doubled to 128 bits, and > >> changing it now involves writing a new cryptographically random number in the > >> appropriate location, rather than simply incrementing it. > >> > >> The xc_domain_save() side of the code is fine, but the xc_domain_restore() > >> needs substantial changes to be usable. > >> > >> This patch replaces the old xc_domain_restore() parameters with a new optional > >> restore_callback. If the callback is provided, and an appropriate hunk is > >> found in the migration stream, the appropriate piece of guest memory is mapped > >> and provided to the callback. > >> > >> This patch also fixes all the in-tree callers of xc_domain_restore(). All > >> callers had the functionality disabled, and never exposed in a public way. > >> > >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > >> CC: Ian Campbell <Ian.Campbell@citrix.com> > >> CC: Ian Jackson <Ian.Jackson@eu.citrix.com> > >> CC: George Dunlap <george.dunlap@eu.citrix.com> > >> > >> --- > >> > >> George: > >> > >> Despite this being a feature, I am requesting a freeze exemption. It is > >> functionality which was not used (or indeed useful) before, and remains that > >> way as far as in-tree consumers are concerned. > > If it is both wrong and neither used nor useful why aren''t we ripping > > it out? > > The implementation in the tree currently is neither useful nor used. It > is hardwired to disabled for each intree caller, which is why this > change wont affect any current functionality in tree.That doesn''t answer my question.> > ~Andrew > > > > >> However, as there has been once recent xc_domain_restore() API change, it is > >> less disruptive to other users to do another API change before the 4.4 > >> release rather than afterwards. > > I don''t buy this argument, history suggests that there will almost > > certainly be an API change in 4.5 too and in any case we don''t make any > > guarantees about the libxc API. > > > >> --- > >> tools/libxc/xc_domain_restore.c | 68 +++++++++++++++++++------------------- > >> tools/libxc/xc_nomigrate.c | 3 +- > >> tools/libxc/xenguest.h | 24 +++++++++++--- > >> tools/libxl/libxl_create.c | 2 +- > >> tools/libxl/libxl_internal.h | 3 +- > >> tools/libxl/libxl_save_callout.c | 5 ++- > >> tools/libxl/libxl_save_helper.c | 4 +-- > >> tools/xcutils/xc_restore.c | 2 +- > >> 8 files changed, 61 insertions(+), 50 deletions(-) > >> > >> diff --git a/tools/libxc/xc_domain_restore.c b/tools/libxc/xc_domain_restore.c > >> index 80769a7..bb1f7fd 100644 > >> --- a/tools/libxc/xc_domain_restore.c > >> +++ b/tools/libxc/xc_domain_restore.c > >> @@ -1414,8 +1414,7 @@ int xc_domain_restore(xc_interface *xch, int io_fd, uint32_t dom, > >> domid_t store_domid, unsigned int console_evtchn, > >> unsigned long *console_mfn, domid_t console_domid, > >> unsigned int hvm, unsigned int pae, int superpages, > >> - int no_incr_generationid, int checkpointed_stream, > >> - unsigned long *vm_generationid_addr, > >> + int checkpointed_stream, > >> struct restore_callbacks *callbacks) > >> { > >> DECLARE_DOMCTL; > >> @@ -1641,43 +1640,44 @@ int xc_domain_restore(xc_interface *xch, int io_fd, uint32_t dom, > >> xc_set_hvm_param(xch, dom, HVM_PARAM_VM86_TSS, pagebuf.vm86_tss); > >> if ( pagebuf.console_pfn ) > >> console_pfn = pagebuf.console_pfn; > >> - if ( pagebuf.vm_generationid_addr ) { > >> - if ( !no_incr_generationid ) { > >> - unsigned int offset; > >> - unsigned char *buf; > >> - unsigned long long generationid; > >> + if ( pagebuf.vm_generationid_addr && callbacks->generation_id ) { > >> + unsigned int offset; > >> + unsigned char *buf; > >> > >> - /* > >> - * Map the VM generation id buffer and inject the new value. > >> - */ > >> - > >> - pfn = pagebuf.vm_generationid_addr >> PAGE_SHIFT; > >> - offset = pagebuf.vm_generationid_addr & (PAGE_SIZE - 1); > >> - > >> - if ( (pfn >= dinfo->p2m_size) || > >> - (pfn_type[pfn] != XEN_DOMCTL_PFINFO_NOTAB) ) > >> - { > >> - ERROR("generation id buffer frame is bad"); > >> - goto out; > >> - } > >> - > >> - mfn = ctx->p2m[pfn]; > >> - buf = xc_map_foreign_range(xch, dom, PAGE_SIZE, > >> - PROT_READ | PROT_WRITE, mfn); > >> - if ( buf == NULL ) > >> - { > >> - ERROR("xc_map_foreign_range for generation id" > >> - " buffer failed"); > >> - goto out; > >> - } > >> + pfn = pagebuf.vm_generationid_addr >> PAGE_SHIFT; > >> + offset = pagebuf.vm_generationid_addr & (PAGE_SIZE - 1); > >> > >> - generationid = *(unsigned long long *)(buf + offset); > >> - *(unsigned long long *)(buf + offset) = generationid + 1; > >> + if ( pfn >= dinfo->p2m_size ) > >> + { > >> + ERROR("generation id frame (pfn %#lx) outside p2m (size %#lx)\n", > >> + pfn, dinfo->p2m_size); > >> + rc = -1; > >> + goto out; > >> + } > >> + else if ( pfn_type[pfn] != XEN_DOMCTL_PFINFO_NOTAB ) > >> + { > >> + ERROR("generation id frame (pfn %#lx) bad type %#x", > >> + pfn, pfn_type[pfn]); > >> + rc = -1; > >> + goto out; > >> + } > >> > >> - munmap(buf, PAGE_SIZE); > >> + mfn = ctx->p2m[pfn]; > >> + buf = xc_map_foreign_range(xch, dom, PAGE_SIZE, > >> + PROT_READ | PROT_WRITE, mfn); > >> + if ( !buf ) > >> + { > >> + PERROR("Failed to map generation id frame: (mfn %#lx)", mfn); > >> + rc = -1; > >> + goto out; > >> } > >> > >> - *vm_generationid_addr = pagebuf.vm_generationid_addr; > >> + rc = callbacks->generation_id(dom, pagebuf.vm_generationid_addr, > >> + buf + offset, callbacks->data); > >> + munmap(buf, PAGE_SIZE); > >> + > >> + if ( rc ) > >> + goto out; > >> } > >> > >> break; /* our work here is done */ > >> diff --git a/tools/libxc/xc_nomigrate.c b/tools/libxc/xc_nomigrate.c > >> index fb6d53e..8b4c5c8 100644 > >> --- a/tools/libxc/xc_nomigrate.c > >> +++ b/tools/libxc/xc_nomigrate.c > >> @@ -35,8 +35,7 @@ int xc_domain_restore(xc_interface *xch, int io_fd, uint32_t dom, > >> domid_t store_domid, unsigned int console_evtchn, > >> unsigned long *console_mfn, domid_t console_domid, > >> unsigned int hvm, unsigned int pae, int superpages, > >> - int no_incr_generationid, int checkpointed_stream, > >> - unsigned long *vm_generationid_addr, > >> + int checkpointed_stream, > >> struct restore_callbacks *callbacks) > >> { > >> errno = ENOSYS; > >> diff --git a/tools/libxc/xenguest.h b/tools/libxc/xenguest.h > >> index a0e30e1..507bf65 100644 > >> --- a/tools/libxc/xenguest.h > >> +++ b/tools/libxc/xenguest.h > >> @@ -96,6 +96,25 @@ struct restore_callbacks { > >> int (*toolstack_restore)(uint32_t domid, const uint8_t *buf, > >> uint32_t size, void* data); > >> > >> + /** > >> + * Callback to allow the toolstack to manage a domain''s generation ID. > >> + * This function is called if a generation ID chunk is found in the > >> + * migration stream, and the function pointer is provided. > >> + * > >> + * The generation ID location is a special chhunk in the migration stream. > >> + * The toolstack must take a record of the generation ID address, to > >> + * provide it back to xc_domain_save() in the future. It must also update > >> + * the value of the generation id when appropriate. > >> + * > >> + * @param domid The domain id. > >> + * @param genid_gpa Guest physical address of the generation id. > >> + * @param mapped_genid Pointer to the generation id, mapped from the domain. > >> + * @param data General restore_callbacks data pointer. > >> + * @returns 0 for success, -1 for error. > >> + */ > >> + int (*generation_id)(uint32_t domid, uint64_t genid_gpa, > >> + void *mapped_genid, void *data); > >> + > >> /* to be provided as the last argument to each callback function */ > >> void* data; > >> }; > >> @@ -113,9 +132,7 @@ struct restore_callbacks { > >> * @parm hvm non-zero if this is a HVM restore > >> * @parm pae non-zero if this HVM domain has PAE support enabled > >> * @parm superpages non-zero to allocate guest memory with superpages > >> - * @parm no_incr_generationid non-zero if generation id is NOT to be incremented > >> * @parm checkpointed_stream non-zero if the far end of the stream is using checkpointing > >> - * @parm vm_generationid_addr returned with the address of the generation id buffer > >> * @parm callbacks non-NULL to receive a callback to restore toolstack > >> * specific data > >> * @return 0 on success, -1 on failure > >> @@ -125,8 +142,7 @@ int xc_domain_restore(xc_interface *xch, int io_fd, uint32_t dom, > >> domid_t store_domid, unsigned int console_evtchn, > >> unsigned long *console_mfn, domid_t console_domid, > >> unsigned int hvm, unsigned int pae, int superpages, > >> - int no_incr_generationid, int checkpointed_stream, > >> - unsigned long *vm_generationid_addr, > >> + int checkpointed_stream, > >> struct restore_callbacks *callbacks); > >> /** > >> * xc_domain_restore writes a file to disk that contains the device > >> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c > >> index fe7ba0d..4cd4205 100644 > >> --- a/tools/libxl/libxl_create.c > >> +++ b/tools/libxl/libxl_create.c > >> @@ -838,7 +838,7 @@ static void domcreate_bootloader_done(libxl__egc *egc, > >> goto out; > >> } > >> libxl__xc_domain_restore(egc, dcs, > >> - hvm, pae, superpages, 1); > >> + hvm, pae, superpages); > >> return; > >> > >> out: > >> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h > >> index a2d8247..d84890b 100644 > >> --- a/tools/libxl/libxl_internal.h > >> +++ b/tools/libxl/libxl_internal.h > >> @@ -2614,8 +2614,7 @@ _hidden int libxl__toolstack_save(uint32_t domid, uint8_t **buf, > >> /* calls libxl__xc_domain_restore_done when done */ > >> _hidden void libxl__xc_domain_restore(libxl__egc *egc, > >> libxl__domain_create_state *dcs, > >> - int hvm, int pae, int superpages, > >> - int no_incr_generationid); > >> + int hvm, int pae, int superpages); > >> /* If rc==0 then retval is the return value from xc_domain_save > >> * and errnoval is the errno value it provided. > >> * If rc!=0, retval and errnoval are undefined. */ > >> diff --git a/tools/libxl/libxl_save_callout.c b/tools/libxl/libxl_save_callout.c > >> index 6e45b2f..0121cc6 100644 > >> --- a/tools/libxl/libxl_save_callout.c > >> +++ b/tools/libxl/libxl_save_callout.c > >> @@ -41,8 +41,7 @@ static void helper_done(libxl__egc *egc, libxl__save_helper_state *shs); > >> /*----- entrypoints -----*/ > >> > >> void libxl__xc_domain_restore(libxl__egc *egc, libxl__domain_create_state *dcs, > >> - int hvm, int pae, int superpages, > >> - int no_incr_generationid) > >> + int hvm, int pae, int superpages) > >> { > >> STATE_AO_GC(dcs->ao); > >> > >> @@ -59,7 +58,7 @@ void libxl__xc_domain_restore(libxl__egc *egc, libxl__domain_create_state *dcs, > >> state->store_port, > >> state->store_domid, state->console_port, > >> state->console_domid, > >> - hvm, pae, superpages, no_incr_generationid, > >> + hvm, pae, superpages, > >> cbflags, dcs->checkpointed_stream, > >> }; > >> > >> diff --git a/tools/libxl/libxl_save_helper.c b/tools/libxl/libxl_save_helper.c > >> index 880565e..0df97dc 100644 > >> --- a/tools/libxl/libxl_save_helper.c > >> +++ b/tools/libxl/libxl_save_helper.c > >> @@ -250,7 +250,6 @@ int main(int argc, char **argv) > >> unsigned int hvm = strtoul(NEXTARG,0,10); > >> unsigned int pae = strtoul(NEXTARG,0,10); > >> int superpages = strtoul(NEXTARG,0,10); > >> - int no_incr_genidad = strtoul(NEXTARG,0,10); > >> unsigned cbflags = strtoul(NEXTARG,0,10); > >> int checkpointed = strtoul(NEXTARG,0,10); > >> assert(!*++argv); > >> @@ -265,8 +264,7 @@ int main(int argc, char **argv) > >> r = xc_domain_restore(xch, io_fd, dom, store_evtchn, &store_mfn, > >> store_domid, console_evtchn, &console_mfn, > >> console_domid, hvm, pae, superpages, > >> - no_incr_genidad, checkpointed, &genidad, > >> - &helper_restore_callbacks); > >> + checkpointed, &helper_restore_callbacks); > >> helper_stub_restore_results(store_mfn,console_mfn,genidad,0); > >> complete(r); > >> > >> diff --git a/tools/xcutils/xc_restore.c b/tools/xcutils/xc_restore.c > >> index 4ea261b..39d9efb 100644 > >> --- a/tools/xcutils/xc_restore.c > >> +++ b/tools/xcutils/xc_restore.c > >> @@ -56,7 +56,7 @@ main(int argc, char **argv) > >> > >> ret = xc_domain_restore(xch, io_fd, domid, store_evtchn, &store_mfn, 0, > >> console_evtchn, &console_mfn, 0, hvm, pae, superpages, > >> - 0, checkpointed, NULL, NULL); > >> + checkpointed, NULL); > >> > >> if ( ret == 0 ) > >> { > > >
Andrew Cooper
2013-Nov-26 10:51 UTC
Re: [PATCH] tools/many: Fix Generation ID restore interface.
On 26/11/13 10:45, Ian Campbell wrote:> On Tue, 2013-11-26 at 10:41 +0000, Andrew Cooper wrote: >> On 26/11/13 10:11, Ian Campbell wrote: >>> On Mon, 2013-11-25 at 20:40 +0000, Andrew Cooper wrote: >>>> The original Generation ID code was submitted before the specification had >>>> been finalised, and changed completely between the final draft and formal >>>> release. >>>> >>>> Most notably, the size of the Generation ID has doubled to 128 bits, and >>>> changing it now involves writing a new cryptographically random number in the >>>> appropriate location, rather than simply incrementing it. >>>> >>>> The xc_domain_save() side of the code is fine, but the xc_domain_restore() >>>> needs substantial changes to be usable. >>>> >>>> This patch replaces the old xc_domain_restore() parameters with a new optional >>>> restore_callback. If the callback is provided, and an appropriate hunk is >>>> found in the migration stream, the appropriate piece of guest memory is mapped >>>> and provided to the callback. >>>> >>>> This patch also fixes all the in-tree callers of xc_domain_restore(). All >>>> callers had the functionality disabled, and never exposed in a public way. >>>> >>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >>>> CC: Ian Campbell <Ian.Campbell@citrix.com> >>>> CC: Ian Jackson <Ian.Jackson@eu.citrix.com> >>>> CC: George Dunlap <george.dunlap@eu.citrix.com> >>>> >>>> --- >>>> >>>> George: >>>> >>>> Despite this being a feature, I am requesting a freeze exemption. It is >>>> functionality which was not used (or indeed useful) before, and remains that >>>> way as far as in-tree consumers are concerned. >>> If it is both wrong and neither used nor useful why aren''t we ripping >>> it out? >> The implementation in the tree currently is neither useful nor used. It >> is hardwired to disabled for each intree caller, which is why this >> change wont affect any current functionality in tree. > That doesn''t answer my question.Well I am ripping out the old interface. It is at the same time as providing a new sane interface to be used. Just because there are no intree users doesn''t mean there are no users. Part of moving Xapi to libxl will involve plumbing all this stuff into libxl. ~Andrew
George Dunlap
2013-Nov-26 10:57 UTC
Re: [PATCH] tools/many: Fix Generation ID restore interface.
On 11/25/2013 08:40 PM, Andrew Cooper wrote:> The original Generation ID code was submitted before the specification had > been finalised, and changed completely between the final draft and formal > release. > > Most notably, the size of the Generation ID has doubled to 128 bits, and > changing it now involves writing a new cryptographically random number in the > appropriate location, rather than simply incrementing it. > > The xc_domain_save() side of the code is fine, but the xc_domain_restore() > needs substantial changes to be usable. > > This patch replaces the old xc_domain_restore() parameters with a new optional > restore_callback. If the callback is provided, and an appropriate hunk is > found in the migration stream, the appropriate piece of guest memory is mapped > and provided to the callback. > > This patch also fixes all the in-tree callers of xc_domain_restore(). All > callers had the functionality disabled, and never exposed in a public way. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > CC: Ian Campbell <Ian.Campbell@citrix.com> > CC: Ian Jackson <Ian.Jackson@eu.citrix.com> > CC: George Dunlap <george.dunlap@eu.citrix.com> > > --- > > George: > > Despite this being a feature, I am requesting a freeze exemption. It is > functionality which was not used (or indeed useful) before, and remains that > way as far as in-tree consumers are concerned. > > However, as there has been once recent xc_domain_restore() API change, it is > less disruptive to other users to do another API change before the 4.4 > release rather than afterwards.1. What is the value of this feature? It''s not used and not useful, so as far as I can tell, the value is 0. xc_domain_restore() is not a stable interface, so changing it isn''t a consideration. 2. What is the risk of bugs that may slip the release? That may not be found and ship in the release? It looks fairly straightforward, but the code it''s modifying is incredibly fragile, and has a lot of potential combinations which are hard to test. 0 value + non-zero risk => I don''t see a reason to accept it. -George