* [PATCH] tools/many: Fix Generation ID restore interface.
@ 2013-11-25 20:40 Andrew Cooper
2013-11-26 10:11 ` Ian Campbell
2013-11-26 10:57 ` George Dunlap
0 siblings, 2 replies; 6+ messages in thread
From: Andrew Cooper @ 2013-11-25 20:40 UTC (permalink / raw)
To: Xen-devel; +Cc: George Dunlap, Andrew Cooper, Ian Jackson, Ian Campbell
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] tools/many: Fix Generation ID restore interface.
2013-11-25 20:40 [PATCH] tools/many: Fix Generation ID restore interface Andrew Cooper
@ 2013-11-26 10:11 ` Ian Campbell
2013-11-26 10:41 ` Andrew Cooper
2013-11-26 10:57 ` George Dunlap
1 sibling, 1 reply; 6+ messages in thread
From: Ian Campbell @ 2013-11-26 10:11 UTC (permalink / raw)
To: Andrew Cooper; +Cc: George Dunlap, Ian Jackson, Xen-devel
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 )
> {
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] tools/many: Fix Generation ID restore interface.
2013-11-26 10:11 ` Ian Campbell
@ 2013-11-26 10:41 ` Andrew Cooper
2013-11-26 10:45 ` Ian Campbell
0 siblings, 1 reply; 6+ messages in thread
From: Andrew Cooper @ 2013-11-26 10:41 UTC (permalink / raw)
To: Ian Campbell; +Cc: George Dunlap, Ian Jackson, Xen-devel
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 )
>> {
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] tools/many: Fix Generation ID restore interface.
2013-11-26 10:41 ` Andrew Cooper
@ 2013-11-26 10:45 ` Ian Campbell
2013-11-26 10:51 ` Andrew Cooper
0 siblings, 1 reply; 6+ messages in thread
From: Ian Campbell @ 2013-11-26 10:45 UTC (permalink / raw)
To: Andrew Cooper; +Cc: George Dunlap, Ian Jackson, Xen-devel
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 )
> >> {
> >
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] tools/many: Fix Generation ID restore interface.
2013-11-26 10:45 ` Ian Campbell
@ 2013-11-26 10:51 ` Andrew Cooper
0 siblings, 0 replies; 6+ messages in thread
From: Andrew Cooper @ 2013-11-26 10:51 UTC (permalink / raw)
To: Ian Campbell; +Cc: George Dunlap, Ian Jackson, Xen-devel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] tools/many: Fix Generation ID restore interface.
2013-11-25 20:40 [PATCH] tools/many: Fix Generation ID restore interface Andrew Cooper
2013-11-26 10:11 ` Ian Campbell
@ 2013-11-26 10:57 ` George Dunlap
1 sibling, 0 replies; 6+ messages in thread
From: George Dunlap @ 2013-11-26 10:57 UTC (permalink / raw)
To: Andrew Cooper, Xen-devel; +Cc: Ian Jackson, Ian Campbell
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-11-26 10:57 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-11-25 20:40 [PATCH] tools/many: Fix Generation ID restore interface Andrew Cooper
2013-11-26 10:11 ` Ian Campbell
2013-11-26 10:41 ` Andrew Cooper
2013-11-26 10:45 ` Ian Campbell
2013-11-26 10:51 ` Andrew Cooper
2013-11-26 10:57 ` George Dunlap
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.