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