Xen project Mailing List

On Mon, Jan 30, 2012 at 8:54 AM, Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx> wrote:

Introduce a new save_id to save/restore toolstack specific extra
information.

Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
---

tools/libxc/xc_domain_restore.c | 32 +++++++++++++++++++++++++++++++-
tools/libxc/xc_domain_save.c | 17 +++++++++++++++++
tools/libxc/xenguest.h | 23 ++++++++++++++++++++++-

tools/libxc/xg_save_restore.h | 1 +
tools/libxl/libxl_dom.c | 2 +-

tools/xcutils/xc_restore.c | 2 +-
6 files changed, 73 insertions(+), 4 deletions(-)

diff --git a/tools/libxc/xc_domain_restore.c b/tools/libxc/xc_domain_restore.c
index 3fda6f8..ead3df4 100644
--- a/tools/libxc/xc_domain_restore.c
+++ b/tools/libxc/xc_domain_restore.c
@@ -45,6 +45,8 @@ struct restore_ctx {
int last_checkpoint; /* Set when we should commit to the current checkpoint when it completes. */
int compressing; /* Set when sender signals that pages would be sent compressed (for Remus) */
struct domain_info_context dinfo;
+ uint8_t *toolstack_data;
+ uint32_t toolstack_data_len;
};

This is still leaking speculative state. You have only moved the restore code but
you are still reading the (potentially discardable) state into a global ctx structure.

Take a look at the tailbuf code right below the pagebuf_get() call in xc_domain_restore().
It reads the tailbuf onto a temporary buffer and then copies it onto the main one.
This way, if an error occurs while receiving data, one can completely discard the current
checkpoint (pagebuf & tmptailbuf) and restore from the previous one (tailbuf).

You could have a similar *consistent buffer* in the xc_domain_restore function itself, and copy the toolstack
stuff into it before starting to buffer a new checkpoint. Perhaps something like the snippet below?

+ toolstack_data = realloc(pagebuf.toolstack_data_len)
+ memcpy(toolstack_data, pagebuf.toolstack_data, pagebuf.toolstack_data_len);
if ( ctx->last_checkpoint )

Though, this would incur two reallocs (once in pagebuf_get and once more in the main loop).

If there is a maximum size for this buffer, I would suggest mallocing it once and for all, and just
memcpy it.

@@ -827,6 +829,20 @@ static int pagebuf_get_one(xc_interface *xch, struct restore_ctx *ctx,

}
return pagebuf_get_one(xch, ctx, buf, fd, dom);

+ case XC_SAVE_ID_TOOLSTACK:
+ {
+ RDEXACT(fd, &ctx->toolstack_data_len,
+ sizeof(ctx->toolstack_data_len));
+ ctx->toolstack_data = (uint8_t*) malloc(ctx->toolstack_data_len);
+ if ( ctx->toolstack_data == NULL )

+ {
+ PERROR("error memory allocation");
+ return -1;
+ }

+ RDEXACT(fd, ctx->toolstack_data, ctx->toolstack_data_len);

Basically this becomes,
RDEXACT(fd, &buf->toolstack_data_len,...)
buf->toolstack_data = (uint8_t *)realloc(buf->toolstack_data_len, 0)
RDEXACT(fd, buf->toolstack_data, buf->toolstack_data_len)

And please do realloc. Since the pagebuf_get_one code is called repeatedly
by the main loop, using malloc would result in loads of memory leaks.

+ return pagebuf_get_one(xch, ctx, buf, fd, dom);
+ }

+
case XC_SAVE_ID_ENABLE_COMPRESSION:
/* We cannot set compression flag directly in pagebuf structure,
* since this pagebuf still has uncompressed pages that are yet to

@@ -1262,7 +1278,8 @@ int xc_domain_restore(xc_interface *xch, int io_fd, uint32_t dom,

unsigned int console_evtchn, unsigned long *console_mfn,

unsigned int hvm, unsigned int pae, int superpages,

int no_incr_generationid,
- unsigned long *vm_generationid_addr)
+ unsigned long *vm_generationid_addr,

+ struct restore_callbacks *callbacks)
{
DECLARE_DOMCTL;
int rc = 1, frc, i, j, n, m, pae_extended_cr3 = 0, ext_vcpucontext = 0;

@@ -2023,6 +2040,19 @@ int xc_domain_restore(xc_interface *xch, int io_fd, uint32_t dom,
goto out;

finish_hvm:

+ if ( callbacks != NULL && callbacks->toolstack_restore != NULL )
+ {
+ if ( callbacks->toolstack_restore(dom,

+ ctx->toolstack_data, ctx->toolstack_data_len,

+ callbacks->data) < 0 )
+ {

+ PERROR("error calling toolstack_restore");

+ free(ctx->toolstack_data);

+ goto out;
+ }
+ }

+ free(ctx->toolstack_data);
+
/* Dump the QEMU state to a state file for QEMU to load */
if ( dump_qemu(xch, dom, &tailbuf.u.hvm) ) {
PERROR("Error dumping QEMU state to file");
diff --git a/tools/libxc/xc_domain_save.c b/tools/libxc/xc_domain_save.c
index f473dd7..318c433 100644
--- a/tools/libxc/xc_domain_save.c
+++ b/tools/libxc/xc_domain_save.c
@@ -1687,6 +1687,23 @@ int xc_domain_save(xc_interface *xch, int io_fd, uint32_t dom, uint32_t max_iter
}
}

+ if ( callbacks != NULL && callbacks->toolstack_save != NULL )
+ {

+ int id = XC_SAVE_ID_TOOLSTACK;
+ uint8_t *buf;
+ uint32_t len;

+
+ if ( callbacks->toolstack_save(dom, &buf, &len, callbacks->data) < 0 )
+ {

+ PERROR("Error calling toolstack_save");
+ goto out;
+ }
+ wrexact(io_fd, &id, sizeof(id));
+ wrexact(io_fd, &len, sizeof(len));
+ wrexact(io_fd, buf, len);
+ free(buf);
+ }
+
if ( !callbacks->checkpoint )
{
/*
diff --git a/tools/libxc/xenguest.h b/tools/libxc/xenguest.h

index 6026370..76aa475 100644

--- a/tools/libxc/xenguest.h
+++ b/tools/libxc/xenguest.h
@@ -44,6 +44,14 @@ struct save_callbacks {
/* Enable qemu-dm logging dirty pages to xen */
int (*switch_qemu_logdirty)(int domid, unsigned enable, void *data); /* HVM only */

+ /* Save toolstack specific data
+ * @param buf the buffer with the data to be saved
+ * @param len the length of the buffer
+ * The callee allocates the buffer, the caller frees it (buffer must
+ * be free'able).
+ */
+ int (*toolstack_save)(uint32_t domid, uint8_t **buf, uint32_t *len, void *data);
+
/* to be provided as the last argument to each callback function */
void* data;
};

@@ -62,6 +70,16 @@ int xc_domain_save(xc_interface *xch, int io_fd, uint32_t dom, uint32_t max_iter
unsigned long vm_generationid_addr);

+/* callbacks provided by xc_domain_restore */
+struct restore_callbacks {
+ /* callback to restore toolstack specific data */
+ int (*toolstack_restore)(uint32_t domid, uint8_t *buf,
+ uint32_t size, void* data);
+
+ /* to be provided as the last argument to each callback function */
+ void* data;
+};
+
/**
* This function will restore a saved domain.
*

@@ -75,6 +93,8 @@ int xc_domain_save(xc_interface *xch, int io_fd, uint32_t dom, uint32_t max_iter

* @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 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
*/

int xc_domain_restore(xc_interface *xch, int io_fd, uint32_t dom,

@@ -82,7 +102,8 @@ int xc_domain_restore(xc_interface *xch, int io_fd, uint32_t dom,

unsigned int console_evtchn, unsigned long *console_mfn,

unsigned int hvm, unsigned int pae, int superpages,

int no_incr_generationid,
- unsigned long *vm_generationid_addr);
+ unsigned long *vm_generationid_addr,

+ struct restore_callbacks *callbacks);
/**
* xc_domain_restore writes a file to disk that contains the device
* model saved state.
diff --git a/tools/libxc/xg_save_restore.h b/tools/libxc/xg_save_restore.h

index 6286b68..46fdeaa 100644
--- a/tools/libxc/xg_save_restore.h
+++ b/tools/libxc/xg_save_restore.h
@@ -254,6 +254,7 @@

#define XC_SAVE_ID_COMPRESSED_DATA -12 /* Marker to indicate arrival of compressed data */
#define XC_SAVE_ID_ENABLE_COMPRESSION -13 /* Marker to enable compression logic at receiver side */

#define XC_SAVE_ID_HVM_GENERATION_ID_ADDR -14
+#define XC_SAVE_ID_TOOLSTACK -15 /* Optional toolstack specific info */

/*
** We process save/restore/migrate in batches of pages; the below
diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c

index 91643a2..2c5eec5 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -379,7 +379,7 @@ int libxl__domain_restore_common(libxl__gc *gc, uint32_t domid,

state->store_port, &state->store_mfn,
state->console_port, &state->console_mfn,

hvm, pae, superpages, no_incr_generationid,
- &state->vm_generationid_addr);
+ &state->vm_generationid_addr, NULL);

if ( rc ) {
LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "restoring domain");
return ERROR_FAIL;
diff --git a/tools/xcutils/xc_restore.c b/tools/xcutils/xc_restore.c

index 63d53a8..306a10e 100644
--- a/tools/xcutils/xc_restore.c
+++ b/tools/xcutils/xc_restore.c
@@ -47,7 +47,7 @@ main(int argc, char **argv)

ret = xc_domain_restore(xch, io_fd, domid, store_evtchn, &store_mfn,

console_evtchn, &console_mfn, hvm, pae, superpages,

- 0, NULL);
+ 0, NULL, NULL);

if ( ret == 0 )
{
--
1.7.2.5

Re: [Xen-devel] [PATCH v3 1/2] libxc: introduce XC_SAVE_ID_TOOLSTACK