|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [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@xxxxxxxxxx>
> CC: Ian Campbell <Ian.Campbell@xxxxxxxxxx>
> CC: Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>
> CC: George Dunlap <george.dunlap@xxxxxxxxxxxxx>
>
> ---
>
> 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 )
> {
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |