|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [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@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.
~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 |