Xen devel - Jan 2012 - [PATCH v3 0/2] libxl: save/restore qemu physmap

Hi all,
this patch series introduces a new xc_save_id called
XC_SAVE_ID_TOOLSTACK to save/restore toolstack specific information.
Libxl is going to use the new save_id to save and restore qemu''s
physmap.


Changes in v3:

- rebased the series;

- xc_domain_restore: read the toolstack data in pagebuf_get_one, call
the callback at finish_hvm;



Changes in v2:

- xc_domain_save frees the buffer allocated by the callback;

- introduce a version number in the libxl save record;

- define libxl__physmap_info and use it to read/store information to the
buffer.


Stefano Stabellini (2):
      libxc: introduce XC_SAVE_ID_TOOLSTACK
      libxl: save/restore qemu''s physmap

 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         |  132 ++++++++++++++++++++++++++++++++++++++-
 tools/xcutils/xc_restore.c      |    2 +-
 6 files changed, 203 insertions(+), 4 deletions(-)

Cheers,

Stefano

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

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 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;
 };
 
 #define HEARTBEAT_MS 1000
@@ -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);
+            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

Read Qemu''s physmap from xenstore and save it using toolstack_save.
Restore Qemu''s physmap using toolstack_restore.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 tools/libxl/libxl_dom.c |  132 ++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 131 insertions(+), 1 deletions(-)

diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index 2c5eec5..a6eb714 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -349,6 +349,66 @@ out:
     return rc;
 }
 
+struct libxl__physmap_info {
+    uint64_t phys_offset;
+    uint64_t start_addr;
+    uint64_t size;
+    uint32_t namelen;
+    char name[];
+};
+
+#define TOOLSTACK_SAVE_VERSION 1
+
+static int libxl__toolstack_restore(uint32_t domid, uint8_t *buf,
+        uint32_t size, void *data)
+{
+    libxl__gc *gc = (libxl__gc *) data;
+    int i, ret;
+    uint8_t *ptr = buf;
+    uint32_t count = 0, version = 0;
+    struct libxl__physmap_info* pi;
+
+    if (size < sizeof(version) + sizeof(count))
+        return -1;
+
+    memcpy(&version, ptr, sizeof(version));
+    ptr += sizeof(version);
+
+    if (version != TOOLSTACK_SAVE_VERSION)
+        return -1;
+
+    memcpy(&count, ptr, sizeof(count));
+    ptr += sizeof(count);
+ 
+    if (size < sizeof(version) + sizeof(count) +
+            count * (sizeof(struct libxl__physmap_info)))
+        return -1;
+
+    for (i = 0; i < count; i++) {
+        pi = (struct libxl__physmap_info*) ptr;
+        ptr += sizeof(struct libxl__physmap_info) + pi->namelen;
+
+        ret = libxl__xs_write(gc, 0, libxl__sprintf(gc,
+                   
"/local/domain/0/device-model/%d/physmap/%"PRIx64"/start_addr",
+                    domid, pi->phys_offset), "%"PRIx64,
pi->start_addr);
+        if (ret)
+            return -1;
+        ret = libxl__xs_write(gc, 0, libxl__sprintf(gc,
+                   
"/local/domain/0/device-model/%d/physmap/%"PRIx64"/size",
+                    domid, pi->phys_offset), "%"PRIx64,
pi->size);
+        if (ret)
+            return -1;
+        if (pi->namelen > 0) {
+            ret = libxl__xs_write(gc, 0, libxl__sprintf(gc,
+                       
"/local/domain/0/device-model/%d/physmap/%"PRIx64"/name",
+                        domid, pi->phys_offset), "%s",
pi->name);
+            if (ret)
+                return -1;
+        }
+    }
+    return 0;
+}
+
 int libxl__domain_restore_common(libxl__gc *gc, uint32_t domid,
                                  libxl_domain_build_info *info,
                                  libxl__domain_build_state *state,
@@ -358,6 +418,7 @@ int libxl__domain_restore_common(libxl__gc *gc, uint32_t
domid,
     /* read signature */
     int rc;
     int hvm, pae, superpages;
+    struct restore_callbacks callbacks;
     int no_incr_generationid;
     switch (info->type) {
     case LIBXL_DOMAIN_TYPE_HVM:
@@ -365,6 +426,8 @@ int libxl__domain_restore_common(libxl__gc *gc, uint32_t
domid,
         superpages = 1;
         pae = info->u.hvm.pae;
         no_incr_generationid = info->u.hvm.no_incr_generationid;
+        callbacks.toolstack_restore = libxl__toolstack_restore;
+        callbacks.data = gc;
         break;
     case LIBXL_DOMAIN_TYPE_PV:
         hvm = 0;
@@ -379,7 +442,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, NULL);
+                           &state->vm_generationid_addr,
&callbacks);
     if ( rc ) {
         LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "restoring domain");
         return ERROR_FAIL;
@@ -534,6 +597,72 @@ static int libxl__domain_suspend_common_callback(void
*data)
     return 0;
 }
 
+static int libxl__toolstack_save(uint32_t domid, uint8_t **buf,
+        uint32_t *len, void *data)
+{
+    struct suspendinfo *si = (struct suspendinfo *) data;
+    libxl__gc *gc = (libxl__gc *) si->gc;
+    int i = 0;
+    char *start_addr = NULL, *size = NULL, *phys_offset = NULL, *name = NULL;
+    unsigned int num = 0;
+    uint32_t count = 0, version = TOOLSTACK_SAVE_VERSION, namelen = 0;
+    uint8_t *ptr = NULL;
+    char **entries = NULL;
+    struct libxl__physmap_info *pi;
+
+    entries = libxl__xs_directory(gc, 0, libxl__sprintf(gc,
+                "/local/domain/0/device-model/%d/physmap", domid),
&num);
+    count = num;
+
+    *len = sizeof(version) + sizeof(count);
+    *buf = calloc(1, *len);
+    ptr = *buf;
+    if (*buf == NULL)
+        return -1;
+
+    memcpy(ptr, &version, sizeof(version));
+    ptr += sizeof(version);
+    memcpy(ptr, &count, sizeof(count));
+    ptr += sizeof(count);
+
+    for (i = 0; i < count; i++) {
+        unsigned long offset;
+        phys_offset = entries[i];
+        start_addr = libxl__xs_read(gc, 0, libxl__sprintf(gc,
+                   
"/local/domain/0/device-model/%d/physmap/%s/start_addr",
+                    domid, phys_offset));
+        size = libxl__xs_read(gc, 0, libxl__sprintf(gc,
+                   
"/local/domain/0/device-model/%d/physmap/%s/size",
+                    domid, phys_offset));
+        name = libxl__xs_read(gc, 0, libxl__sprintf(gc,
+                   
"/local/domain/0/device-model/%d/physmap/%s/name",
+                    domid, phys_offset));
+
+        if (start_addr == NULL || size == NULL || phys_offset == NULL)
+            return -1;
+
+        if (name == NULL)
+            namelen = 0;
+        else
+            namelen = strlen(name) + 1;
+        *len += namelen + sizeof(struct libxl__physmap_info);
+        offset = ptr - (*buf);
+        *buf = realloc(*buf, *len);
+        if (*buf == NULL)
+            return -1;
+        ptr = (*buf) + offset;
+        pi = (struct libxl__physmap_info *) ptr;
+        pi->phys_offset = strtoll(phys_offset, NULL, 16);
+        pi->start_addr = strtoll(start_addr, NULL, 16);
+        pi->size = strtoll(size, NULL, 16);
+        pi->namelen = namelen;
+        memcpy(pi->name, name, namelen);
+        ptr += sizeof(struct libxl__physmap_info) + namelen;
+    }
+
+    return 0;
+}
+
 int libxl__domain_suspend_common(libxl__gc *gc, uint32_t domid, int fd,
                                  libxl_domain_type type,
                                  int live, int debug)
@@ -596,6 +725,7 @@ int libxl__domain_suspend_common(libxl__gc *gc, uint32_t
domid, int fd,
     memset(&callbacks, 0, sizeof(callbacks));
     callbacks.suspend = libxl__domain_suspend_common_callback;
     callbacks.switch_qemu_logdirty =
libxl__domain_suspend_common_switch_qemu_logdirty;
+    callbacks.toolstack_save = libxl__toolstack_save;
     callbacks.data = &si;
 
     rc = xc_domain_save(ctx->xch, fd, domid, 0, 0, flags, &callbacks,
-- 
1.7.2.5

On Mon, Jan 30, 2012 at 8:54 AM, Stefano Stabellini <
stefano.stabellini@eu.citrix.com> wrote:
> +
> +static int libxl__toolstack_restore(uint32_t domid, uint8_t *buf,
> +        uint32_t size, void *data)
> +{
> +    libxl__gc *gc = (libxl__gc *) data;
> +    int i, ret;
> +    uint8_t *ptr = buf;
> +    uint32_t count = 0, version = 0;
> +    struct libxl__physmap_info* pi;
> +
> +    if (size < sizeof(version) + sizeof(count))
> +        return -1;
> +
> +    memcpy(&version, ptr, sizeof(version));
> +    ptr += sizeof(version);
> +
> +    if (version != TOOLSTACK_SAVE_VERSION)
> +        return -1;
> +
> +    memcpy(&count, ptr, sizeof(count));
> +    ptr += sizeof(count);
> +
> +    if (size < sizeof(version) + sizeof(count) +
> +            count * (sizeof(struct libxl__physmap_info)))
> +        return -1;
> +
> +    for (i = 0; i < count; i++) {
> +        pi = (struct libxl__physmap_info*) ptr;
> +        ptr += sizeof(struct libxl__physmap_info) + pi->namelen;
> +
> +        ret = libxl__xs_write(gc, 0, libxl__sprintf(gc,
> +
> 
"/local/domain/0/device-model/%d/physmap/%"PRIx64"/start_addr",
> +                    domid, pi->phys_offset), "%"PRIx64,
pi->start_addr);
> +        if (ret)
> +            return -1;
> +        ret = libxl__xs_write(gc, 0, libxl__sprintf(gc,
> +
> 
"/local/domain/0/device-model/%d/physmap/%"PRIx64"/size",
> +                    domid, pi->phys_offset), "%"PRIx64,
pi->size);
> +        if (ret)
> +            return -1;
> +        if (pi->namelen > 0) {
> +            ret = libxl__xs_write(gc, 0, libxl__sprintf(gc,
> +
> 
"/local/domain/0/device-model/%d/physmap/%"PRIx64"/name",
> +                        domid, pi->phys_offset), "%s",
pi->name);
> +            if (ret)
> +                return -1;
> +        }
> +    }
> +    return 0;
> +}
> +
>

Sorry if this sounds silly. Is the save/restore of Qemu Physmap equivalent
to
save/restore of the Qemu Device Model ?


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On Mon, 30 Jan 2012, Shriram Rajagopalan wrote:
> On Mon, Jan 30, 2012 at 8:54 AM, Stefano Stabellini
<stefano.stabellini@eu.citrix.com> wrote:
>       +
>       +static int libxl__toolstack_restore(uint32_t domid, uint8_t *buf,
>       +        uint32_t size, void *data)
>       +{
>       +    libxl__gc *gc = (libxl__gc *) data;
>       +    int i, ret;
>       +    uint8_t *ptr = buf;
>       +    uint32_t count = 0, version = 0;
>       +    struct libxl__physmap_info* pi;
>       +
>       +    if (size < sizeof(version) + sizeof(count))
>       +        return -1;
>       +
>       +    memcpy(&version, ptr, sizeof(version));
>       +    ptr += sizeof(version);
>       +
>       +    if (version != TOOLSTACK_SAVE_VERSION)
>       +        return -1;
>       +
>       +    memcpy(&count, ptr, sizeof(count));
>       +    ptr += sizeof(count);
>       +
>       +    if (size < sizeof(version) + sizeof(count) +
>       +            count * (sizeof(struct libxl__physmap_info)))
>       +        return -1;
>       +
>       +    for (i = 0; i < count; i++) {
>       +        pi = (struct libxl__physmap_info*) ptr;
>       +        ptr += sizeof(struct libxl__physmap_info) +
pi->namelen;
>       +
>       +        ret = libxl__xs_write(gc, 0, libxl__sprintf(gc,
>       +                  
 "/local/domain/0/device-model/%d/physmap/%"PRIx64"/start_addr",
>       +                    domid, pi->phys_offset),
"%"PRIx64, pi->start_addr);
>       +        if (ret)
>       +            return -1;
>       +        ret = libxl__xs_write(gc, 0, libxl__sprintf(gc,
>       +                  
 "/local/domain/0/device-model/%d/physmap/%"PRIx64"/size",
>       +                    domid, pi->phys_offset),
"%"PRIx64, pi->size);
>       +        if (ret)
>       +            return -1;
>       +        if (pi->namelen > 0) {
>       +            ret = libxl__xs_write(gc, 0, libxl__sprintf(gc,
>       +                      
 "/local/domain/0/device-model/%d/physmap/%"PRIx64"/name",
>       +                        domid, pi->phys_offset),
"%s", pi->name);
>       +            if (ret)
>       +                return -1;
>       +        }
>       +    }
>       +    return 0;
>       +}
>       +
> 
> 
> 
> Sorry if this sounds silly. Is the save/restore of Qemu Physmap equivalent
to
> save/restore of the Qemu Device Model ?
Nope, it is in addition to the save/restore of the QEMU device state.
--8323329-1202080671-1327946825=:3196
Content-Type: text/plain; charset="us-ascii"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Content-Disposition: inline

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

--8323329-1202080671-1327946825=:3196--

On Mon, Jan 30, 2012 at 8:54 AM, Stefano Stabellini <
stefano.stabellini@eu.citrix.com> wrote:
> Introduce a new save_id to save/restore toolstack specific extra
> information.
>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> ---
>  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
>
>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On Mon, 30 Jan 2012, Shriram Rajagopalan wrote:
> On Mon, Jan 30, 2012 at 8:54 AM, Stefano Stabellini
<stefano.stabellini@eu.citrix.com> wrote:
>       Introduce a new save_id to save/restore toolstack specific extra
>       information.
> 
>       Signed-off-by: Stefano Stabellini
<stefano.stabellini@eu.citrix.com>
>       ---
>  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.
> 
Even though the current toolstack data is tiny, I don''t want to realloc
a potentially big buffer twice or memcpy''ing more than necessary.
I don''t want to add a maximum size either.
What if I add two new arguments to pagebuf_get_one: a pointer to the
buffer to be malloc''ed and the length? Like it was done with the
callbacks argument in the previous version of this patch?


>       @@ -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.
> 
I presume that this buf pointer would be an argument passed to
pagebuf_get_one.
In this case, I don''t suppose I need to memcpy anything anywhere, do I?
Later on, in finish_hvm, I''ll just do:



   if ( callbacks != NULL && callbacks->toolstack_restore != NULL )
   {
       if ( callbacks->toolstack_restore(dom,
                   buf->toolstack_data, buf->toolstack_data_len,
                   callbacks->data) < 0 )
       {
           PERROR("error calling toolstack_restore");
           free(buf->toolstack_data);
           buf->toolstack_data = NULL;
           buf->toolstack_data_len = 0;
           goto out;
       }
   }
   free(buf->toolstack_data);
   buf->toolstack_data = NULL;
   buf->toolstack_data_len = 0;


--8323329-2089809799-1328022236=:3196
Content-Type: text/plain; charset="us-ascii"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Content-Disposition: inline

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

--8323329-2089809799-1328022236=:3196--

On Tue, 31 Jan 2012, Stefano Stabellini wrote:
> On Mon, 30 Jan 2012, Shriram Rajagopalan wrote:
> > On Mon, Jan 30, 2012 at 8:54 AM, Stefano Stabellini
<stefano.stabellini@eu.citrix.com> wrote:
> >       Introduce a new save_id to save/restore toolstack specific extra
> >       information.
> > 
> >       Signed-off-by: Stefano Stabellini
<stefano.stabellini@eu.citrix.com>
> >       ---
> >  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.
> > 
> 
> Even though the current toolstack data is tiny, I don''t want to
realloc
> a potentially big buffer twice or memcpy''ing more than necessary.
> I don''t want to add a maximum size either.
> What if I add two new arguments to pagebuf_get_one: a pointer to the
> buffer to be malloc''ed and the length? Like it was done with the
> callbacks argument in the previous version of this patch?
> 
> 
> 
> >       @@ -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.
> > 
> 
> I presume that this buf pointer would be an argument passed to
> pagebuf_get_one.
> In this case, I don''t suppose I need to memcpy anything anywhere,
do I?
> Later on, in finish_hvm, I''ll just do:
> 
> 
> 
>    if ( callbacks != NULL && callbacks->toolstack_restore
!= NULL )
>    {
>        if ( callbacks->toolstack_restore(dom,
>                    buf->toolstack_data,
buf->toolstack_data_len,
>                    callbacks->data) < 0
)
>        {
>            PERROR("error calling
toolstack_restore");
>            free(buf->toolstack_data);
>            buf->toolstack_data = NULL;
>            buf->toolstack_data_len = 0;
>            goto out;
>        }
>    }
>    free(buf->toolstack_data);
>    buf->toolstack_data = NULL;
>    buf->toolstack_data_len = 0;

Sorry about this, I configured my mailer not to send emails in UTF-8
because they can be the cause of nasty python bugs with spaces and tabs.
Let me write this code snippet again:

    if ( callbacks != NULL && callbacks->toolstack_restore != NULL )
    {
        if ( callbacks->toolstack_restore(dom,
                    buf->toolstack_data, buf->toolstack_data_len,
                    callbacks->data) < 0 )
        {
            PERROR("error calling toolstack_restore");
            free(buf->toolstack_data);
            buf->toolstack_data = NULL;
            buf->toolstack_data_len = 0;
            goto out;
        }
    }
    free(buf->toolstack_data);
    buf->toolstack_data = NULL;
    buf->toolstack_data_len = 0;

--8323329-133505091-1328022521=:3196
Content-Type: text/plain; charset="us-ascii"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Content-Disposition: inline

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

--8323329-133505091-1328022521=:3196--

On 2012-01-31, at 7:08 AM, Stefano Stabellini <
stefano.stabellini@eu.citrix.com> wrote:
> On Tue, 31 Jan 2012, Stefano Stabellini wrote:
>> On Mon, 30 Jan 2012, Shriram Rajagopalan wrote:
>>> On Mon, Jan 30, 2012 at 8:54 AM, Stefano Stabellini <
stefano.stabellini@eu.citrix.com> wrote:
>>>     Introduce a new save_id to save/restore toolstack specific
extra
>>>     information.
>>>
>>>     Signed-off-by: Stefano Stabellini
<stefano.stabellini@eu.citrix.com>
>>>     ---
>>>  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.
>>>
>>
>> Even though the current toolstack data is tiny, I don''t want
to realloc
>> a potentially big buffer twice or memcpy''ing more than
necessary.
>> I don''t want to add a maximum size either.
>> What if I add two new arguments to pagebuf_get_one: a pointer to the
>> buffer to be malloc''ed and the length? Like it was done with
the
>> callbacks argument in the previous version of this patch?
>>
>>
>>
>>>     @@ -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.
>>>
>>
>> I presume that this buf pointer would be an argument passed to
>> pagebuf_get_one.
>> In this case, I don''t suppose I need to memcpy anything
anywhere, do I?
>> Later on, in finish_hvm, I''ll just do:
>>
>>
>>
>>    if ( callbacks != NULL &&
callbacks->toolstack_restore != NULL )
>>    {
>>        if ( callbacks->toolstack_restore(dom,
>>                   ÂÂ
buf->toolstack_data,
buf->toolstack_data_len,
>>                    callbacks->data)
< 0 )
>>        {
>>            PERROR("error calling
toolstack_restore");
>>            free(buf->toolstack_data);
>>          buf->toolstack_data = NULL;
>>          buf->toolstack_data_len = 0;
>>            goto out;
>>        }
>>    }
>>    free(buf->toolstack_data);
>>  buf->toolstack_data = NULL;
>>  buf->toolstack_data_len = 0;
>
>
> Sorry about this, I configured my mailer not to send emails in UTF-8
> because they can be the cause of nasty python bugs with spaces and tabs.
> Let me write this code snippet again:
>
>   if ( callbacks != NULL && callbacks->toolstack_restore != NULL
)
>   {
>       if ( callbacks->toolstack_restore(dom,
>                   buf->toolstack_data, buf->toolstack_data_len,
>                   callbacks->data) < 0 )
>       {
>           PERROR("error calling toolstack_restore");
>           free(buf->toolstack_data);
>           buf->toolstack_data = NULL;
>           buf->toolstack_data_len = 0;
>           goto out;
>       }
>   }
>   free(buf->toolstack_data);
>   buf->toolstack_data = NULL;
>   buf->toolstack_data_len = 0;
Let me rephrase the issue to make sure I am on the same page with you.
Please feel free to refute my assumptions or anything obvious that I might
have
missed.

Your code as-is (first version of the patch, where restore_toolstack is
called
in pagebuf_get_one) would work well for live migration only
i.e. num(checkpoints) =1
It wont work for remus (num(checkpoints) >1), since you are committing
speculative state (state from an incomplete checkpoint).

Now , I assume that the toolstack data is going to be sent in every
checkpoint,
just like the qemu device model state.

If that is not the case, the restore side of the code is fine as-is (even
with
previous version) but the save end needs to be modified, for it seems to
send the data with every checkpoint.  With that mod, things would work fine
for both live migration & Remus.


Assuming that the toolstack data is sent repeatedly with every checkpoint,
there are a few things to note:

* Do not commit state (like xenstore write) in pagebuf_get_one
because state received there (ie last half buffered checkpoint) could be
discarded.
-- this was the bug in the first version (calling toolstack_restore in
pagebuf)

* Since pagebuf_get_one gets called for each checkpoint, a malloc in
pagebuf_get_one
needs a free before the next call. Alternative: use realloc (e.g.
buf->pages)
and free in pagebuf_free().

-- this is the bug in your current version of the patch. I see a malloc in
pagebuf_get_one function, but
   no corresponding free. You are freeing in finish_hvm (after the Nth
checkpoint), but have
  created N-1 memory leaks.

* Do not touch anything from *current_version* of pagebuf after finish:,
since it has been marked incomplete.
  Moving the toolstack_data pointer from a discardable pagebuf to
restore_ctx, is not going to solve
  the issue as you are still (indirectly) accessing the data from
current_version of pagebuf.

 State received in pagebuf_get is applied in xc_domain_restore, only in the
following code
 region (simplified):

//We have a full checkpoint[i] and a corresponding tailbuf.
copypages:
  apply checkpoint[i]

if (pagebuf_get(checkpoint[i+1]) || tailbuf_get(tmptailbuf)) {
  printf("Error receiving last checkpoint");
  //So, "discard" it and resume domain, with memory checkpoint[i],
  // and also apply tailbuf
}

memcpy(tailbuf, tmptailbuf);

/* At this point, I have a full checkpoint[i+1] and a tailbuf that is
consistent with
  checkpoint i+1.
*/
goto copypages;

finish:
  if (hvm) goto finish_hvm;
  ... pv stuff
  finish_hvm:
    Apply tailbuf (that corresponds to checkpoint[i])
   ...



But if you wish to receive dynamic state (e.g. device model, toolstack,
etc) in every checkpoint
that should be applied *only once*  in the end, you could use the tailbuf
(as done by the
qemu device model currently).
Or if you have to receive them in the pagebuf, then have two pieces of the
same state,
 consistent_state, curr_state, such that you do

 pagebuf_get(curr_state)
 tailbuf_get(..)..
 if (no error so far), then
  copy curr_state to consistent_state.
  goto copypages;

finish:
 ..
  finish_hvm:
     apply consistent_state.

IMHO, it looks cleaner to push the toolstack state into buffer_tail
(send/receive in tailbuf), so
that you can continue to do malloc in buffer_tail and a corresponding free
in tailbuf_free.

cheers
shriram


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On Tue, 31 Jan 2012, Shriram Rajagopalan wrote:
> Or if you have to receive them in the pagebuf, then have two pieces of the
same state,
>  consistent_state, curr_state, such that you do
> 
>  pagebuf_get(curr_state)
>  tailbuf_get(..)..
>  if (no error so far), then
>   copy curr_state to consistent_state.
>   goto copypages;
> 
> finish:
>  ..
>   finish_hvm:
>      apply consistent_state.
I think I am starting to understand: see appended patch if it makes
things better.

---

diff --git a/tools/libxc/xc_domain_restore.c b/tools/libxc/xc_domain_restore.c
index 3fda6f8..02b523d 100644
--- a/tools/libxc/xc_domain_restore.c
+++ b/tools/libxc/xc_domain_restore.c
@@ -684,6 +684,11 @@ typedef struct {
     uint64_t vm_generationid_addr;
 } pagebuf_t;
 
+struct toolstack_data_t {
+    uint8_t *data;
+    uint32_t len;
+};
+
 static int pagebuf_init(pagebuf_t* buf)
 {
     memset(buf, 0, sizeof(*buf));
@@ -703,7 +708,8 @@ static void pagebuf_free(pagebuf_t* buf)
 }
 
 static int pagebuf_get_one(xc_interface *xch, struct restore_ctx *ctx,
-                           pagebuf_t* buf, int fd, uint32_t dom)
+                           pagebuf_t* buf, int fd, uint32_t dom,
+                           struct toolstack_data_t *tdata)
 {
     int count, countpages, oldcount, i;
     void* ptmp;
@@ -726,7 +732,7 @@ static int pagebuf_get_one(xc_interface *xch, struct
restore_ctx *ctx,
     case XC_SAVE_ID_ENABLE_VERIFY_MODE:
         DPRINTF("Entering page verify mode\n");
         buf->verify = 1;
-        return pagebuf_get_one(xch, ctx, buf, fd, dom);
+        return pagebuf_get_one(xch, ctx, buf, fd, dom, tdata);
 
     case XC_SAVE_ID_VCPU_INFO:
         buf->new_ctxt_format = 1;
@@ -737,7 +743,7 @@ static int pagebuf_get_one(xc_interface *xch, struct
restore_ctx *ctx,
             return -1;
         }
         // DPRINTF("Max VCPU ID: %d, vcpumap: %llx\n",
buf->max_vcpu_id, buf->vcpumap);
-        return pagebuf_get_one(xch, ctx, buf, fd, dom);
+        return pagebuf_get_one(xch, ctx, buf, fd, dom, tdata);
 
     case XC_SAVE_ID_HVM_IDENT_PT:
         /* Skip padding 4 bytes then read the EPT identity PT location. */
@@ -748,7 +754,7 @@ static int pagebuf_get_one(xc_interface *xch, struct
restore_ctx *ctx,
             return -1;
         }
         // DPRINTF("EPT identity map address: %llx\n",
buf->identpt);
-        return pagebuf_get_one(xch, ctx, buf, fd, dom);
+        return pagebuf_get_one(xch, ctx, buf, fd, dom, tdata);
 
     case XC_SAVE_ID_HVM_VM86_TSS:
         /* Skip padding 4 bytes then read the vm86 TSS location. */
@@ -759,7 +765,7 @@ static int pagebuf_get_one(xc_interface *xch, struct
restore_ctx *ctx,
             return -1;
         }
         // DPRINTF("VM86 TSS location: %llx\n", buf->vm86_tss);
-        return pagebuf_get_one(xch, ctx, buf, fd, dom);
+        return pagebuf_get_one(xch, ctx, buf, fd, dom, tdata);
 
     case XC_SAVE_ID_TMEM:
         DPRINTF("xc_domain_restore start tmem\n");
@@ -767,14 +773,14 @@ static int pagebuf_get_one(xc_interface *xch, struct
restore_ctx *ctx,
             PERROR("error reading/restoring tmem");
             return -1;
         }
-        return pagebuf_get_one(xch, ctx, buf, fd, dom);
+        return pagebuf_get_one(xch, ctx, buf, fd, dom, tdata);
 
     case XC_SAVE_ID_TMEM_EXTRA:
         if ( xc_tmem_restore_extra(xch, dom, fd) ) {
             PERROR("error reading/restoring tmem extra");
             return -1;
         }
-        return pagebuf_get_one(xch, ctx, buf, fd, dom);
+        return pagebuf_get_one(xch, ctx, buf, fd, dom, tdata);
 
     case XC_SAVE_ID_TSC_INFO:
     {
@@ -788,7 +794,7 @@ static int pagebuf_get_one(xc_interface *xch, struct
restore_ctx *ctx,
             PERROR("error reading/restoring tsc info");
             return -1;
         }
-        return pagebuf_get_one(xch, ctx, buf, fd, dom);
+        return pagebuf_get_one(xch, ctx, buf, fd, dom, tdata);
     }
 
     case XC_SAVE_ID_HVM_CONSOLE_PFN :
@@ -800,12 +806,12 @@ static int pagebuf_get_one(xc_interface *xch, struct
restore_ctx *ctx,
             return -1;
         }
         // DPRINTF("console pfn location: %llx\n",
buf->console_pfn);
-        return pagebuf_get_one(xch, ctx, buf, fd, dom);
+        return pagebuf_get_one(xch, ctx, buf, fd, dom, tdata);
 
     case XC_SAVE_ID_LAST_CHECKPOINT:
         ctx->last_checkpoint = 1;
         // DPRINTF("last checkpoint indication received");
-        return pagebuf_get_one(xch, ctx, buf, fd, dom);
+        return pagebuf_get_one(xch, ctx, buf, fd, dom, tdata);
 
     case XC_SAVE_ID_HVM_ACPI_IOPORTS_LOCATION:
         /* Skip padding 4 bytes then read the acpi ioport location. */
@@ -815,7 +821,7 @@ static int pagebuf_get_one(xc_interface *xch, struct
restore_ctx *ctx,
             PERROR("error read the acpi ioport location");
             return -1;
         }
-        return pagebuf_get_one(xch, ctx, buf, fd, dom);
+        return pagebuf_get_one(xch, ctx, buf, fd, dom, tdata);
 
     case XC_SAVE_ID_HVM_VIRIDIAN:
         /* Skip padding 4 bytes then read the acpi ioport location. */
@@ -825,7 +831,20 @@ static int pagebuf_get_one(xc_interface *xch, struct
restore_ctx *ctx,
             PERROR("error read the viridian flag");
             return -1;
         }
-        return pagebuf_get_one(xch, ctx, buf, fd, dom);
+        return pagebuf_get_one(xch, ctx, buf, fd, dom, tdata);
+
+    case XC_SAVE_ID_TOOLSTACK:
+        {
+            RDEXACT(fd, &tdata->len, sizeof(tdata->len));
+            tdata->data = (uint8_t*) realloc(tdata->data, tdata->len);
+            if ( tdata->data == NULL )
+            {
+                PERROR("error memory allocation");
+                return -1;
+            }
+            RDEXACT(fd, tdata->data, tdata->len);
+            return pagebuf_get_one(xch, ctx, buf, fd, dom, tdata);
+        }
 
     case XC_SAVE_ID_ENABLE_COMPRESSION:
         /* We cannot set compression flag directly in pagebuf structure,
@@ -835,7 +854,7 @@ static int pagebuf_get_one(xc_interface *xch, struct
restore_ctx *ctx,
          */
         ctx->compressing = 1;
         // DPRINTF("compression flag received");
-        return pagebuf_get_one(xch, ctx, buf, fd, dom);
+        return pagebuf_get_one(xch, ctx, buf, fd, dom, tdata);
 
     case XC_SAVE_ID_COMPRESSED_DATA:
 
@@ -870,7 +889,7 @@ static int pagebuf_get_one(xc_interface *xch, struct
restore_ctx *ctx,
             return -1;
         }
         DPRINTF("read generation id buffer address");
-        return pagebuf_get_one(xch, ctx, buf, fd, dom);
+        return pagebuf_get_one(xch, ctx, buf, fd, dom, tdata);
 
     default:
         if ( (count > MAX_BATCH_SIZE) || (count < 0) ) {
@@ -914,7 +933,7 @@ static int pagebuf_get_one(xc_interface *xch, struct
restore_ctx *ctx,
      * following a <XC_SAVE_ID_COMPRESSED_DATA, compressedChunkSize>
tuple.
      */
     if (buf->compressing)
-        return pagebuf_get_one(xch, ctx, buf, fd, dom);
+        return pagebuf_get_one(xch, ctx, buf, fd, dom, tdata);
 
     oldcount = buf->nr_physpages;
     buf->nr_physpages += countpages;
@@ -939,7 +958,8 @@ static int pagebuf_get_one(xc_interface *xch, struct
restore_ctx *ctx,
 }
 
 static int pagebuf_get(xc_interface *xch, struct restore_ctx *ctx,
-                       pagebuf_t* buf, int fd, uint32_t dom)
+                       pagebuf_t* buf, int fd, uint32_t dom,
+                       struct toolstack_data_t *tdata)
 {
     int rc;
 
@@ -947,7 +967,7 @@ static int pagebuf_get(xc_interface *xch, struct restore_ctx
*ctx,
     buf->compbuf_pos = buf->compbuf_size = 0;
 
     do {
-        rc = pagebuf_get_one(xch, ctx, buf, fd, dom);
+        rc = pagebuf_get_one(xch, ctx, buf, fd, dom, tdata);
     } while (rc > 0);
 
     if (rc < 0)
@@ -1262,7 +1282,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;
@@ -1310,6 +1331,7 @@ int xc_domain_restore(xc_interface *xch, int io_fd,
uint32_t dom,
 
     pagebuf_t pagebuf;
     tailbuf_t tailbuf, tmptail;
+    struct toolstack_data_t tdata, tdata2, tdatatmp;
     void* vcpup;
     uint64_t console_pfn = 0;
 
@@ -1322,6 +1344,8 @@ int xc_domain_restore(xc_interface *xch, int io_fd,
uint32_t dom,
     pagebuf_init(&pagebuf);
     memset(&tailbuf, 0, sizeof(tailbuf));
     tailbuf.ishvm = hvm;
+    memset(&tdata, 0, sizeof(tdata));
+    memset(&tdata2, 0, sizeof(tdata2));
 
     memset(ctx, 0, sizeof(*ctx));
 
@@ -1441,7 +1465,7 @@ int xc_domain_restore(xc_interface *xch, int io_fd,
uint32_t dom,
         if ( !ctx->completed ) {
             pagebuf.nr_physpages = pagebuf.nr_pages = 0;
             pagebuf.compbuf_pos = pagebuf.compbuf_size = 0;
-            if ( pagebuf_get_one(xch, ctx, &pagebuf, io_fd, dom) < 0 ) {
+            if ( pagebuf_get_one(xch, ctx, &pagebuf, io_fd, dom,
&tdata) < 0 ) {
                 PERROR("Error when reading batch");
                 goto out;
             }
@@ -1589,7 +1613,7 @@ int xc_domain_restore(xc_interface *xch, int io_fd,
uint32_t dom,
 
     // DPRINTF("Buffered checkpoint\n");
 
-    if ( pagebuf_get(xch, ctx, &pagebuf, io_fd, dom) ) {
+    if ( pagebuf_get(xch, ctx, &pagebuf, io_fd, dom, &tdata2) ) {
         PERROR("error when buffering batch, finishing");
         goto finish;
     }
@@ -1602,6 +1626,9 @@ int xc_domain_restore(xc_interface *xch, int io_fd,
uint32_t dom,
     }
     tailbuf_free(&tailbuf);
     memcpy(&tailbuf, &tmptail, sizeof(tailbuf));
+    tdatatmp = tdata;
+    tdata = tdata2;
+    tdata2 = tdatatmp;
 
     goto loadpages;
 
@@ -2023,6 +2050,20 @@ 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, tdata.data, tdata.len,
+                    callbacks->data) < 0 )
+        {
+            PERROR("error calling toolstack_restore");
+            free(tdata.data);
+            free(tdata2.data);
+            goto out;
+        }
+    }
+    free(tdata.data);
+    free(tdata2.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 )
     {
--8323329-1278865655-1328036317=:3196
Content-Type: text/plain; charset="us-ascii"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Content-Disposition: inline

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

--8323329-1278865655-1328036317=:3196--

On Tue, Jan 31, 2012 at 10:58 AM, Stefano Stabellini <
stefano.stabellini@eu.citrix.com> wrote:
> On Tue, 31 Jan 2012, Shriram Rajagopalan wrote:
> > Or if you have to receive them in the pagebuf, then have two pieces of
> the same state,
> >  consistent_state, curr_state, such that you do
> >
> >  pagebuf_get(curr_state)
> >  tailbuf_get(..)..
> >  if (no error so far), then
> >   copy curr_state to consistent_state.
> >   goto copypages;
> >
> > finish:
> >  ..
> >   finish_hvm:
> >      apply consistent_state.
>
> I think I am starting to understand: see appended patch if it makes
> things better.
>
>
Instead of adding yet another parameter to pagebuf_get_one, you could
simplify the patch by adding a toolstack_data_t field to the pagebuf struct.
See comments below.


> ---
>
> diff --git a/tools/libxc/xc_domain_restore.c
> b/tools/libxc/xc_domain_restore.c
> index 3fda6f8..02b523d 100644
> --- a/tools/libxc/xc_domain_restore.c
> +++ b/tools/libxc/xc_domain_restore.c
> @@ -684,6 +684,11 @@ typedef struct {
>     uint64_t vm_generationid_addr;
>  } pagebuf_t;
>
> +struct toolstack_data_t {
+    uint8_t *data;
+    uint32_t len;
+};
+

Add an instance of this struct to the pagebuf_t structure
(gets initialized to NULL, by pagebuf_init)

and dont forget to add the appropriate free calls to pagebuf_free.

>   static int pagebuf_init(pagebuf_t* buf)
>  {
>     memset(buf, 0, sizeof(*buf));
> @@ -703,7 +708,8 @@ static void pagebuf_free(pagebuf_t* buf)
>  }
>
>  static int pagebuf_get_one(xc_interface *xch, struct restore_ctx *ctx,
> -                           pagebuf_t* buf, int fd, uint32_t dom)
> +                           pagebuf_t* buf, int fd, uint32_t dom,
> +                           struct toolstack_data_t *tdata)
>
Eliminate the extra arg. and all the fixes to pagebuf_get_one() calls.

 +        {
> +    case XC_SAVE_ID_TOOLSTACK:
> +            RDEXACT(fd, &tdata->len, sizeof(tdata->len));
> +            tdata->data = (uint8_t*) realloc(tdata->data,
tdata->len);
> +            if ( tdata->data == NULL )
> +            {
> +                PERROR("error memory allocation");
> +                return -1;
> +            }
> +            RDEXACT(fd, tdata->data, tdata->len);
> +            return pagebuf_get_one(xch, ctx, buf, fd, dom, tdata);
> +        }
>
>
RDEXACT(fd, &buf->tdata->len, sizeof())
...

@@ -1262,7 +1282,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;
> @@ -1310,6 +1331,7 @@ int xc_domain_restore(xc_interface *xch, int io_fd,
> uint32_t dom,
>
>     pagebuf_t pagebuf;
>     tailbuf_t tailbuf, tmptail;
> +    struct toolstack_data_t tdata, tdata2, tdatatmp;
>
struct toolstack_data_t tdata, tdatatmp;

and the memsets ofcourse.

> @@ -1441,7 +1465,7 @@ int xc_domain_restore(xc_interface *xch, int io_fd,
> uint32_t dom,
>          if ( !ctx->completed ) {
>             pagebuf.nr_physpages = pagebuf.nr_pages = 0;
>             pagebuf.compbuf_pos = pagebuf.compbuf_size = 0;
> -            if ( pagebuf_get_one(xch, ctx, &pagebuf, io_fd, dom) <
0 ) {
> +            if ( pagebuf_get_one(xch, ctx, &pagebuf, io_fd, dom,
&tdata)
> < 0 ) {
>                  PERROR("Error when reading batch");
>                 goto out;
>             }
>
Both live migration and Remus reach this piece of code, while applying
the 1st or Nth checkpoint (respectively)

if (ctx->last_checkpoint)
{
  //  DPRINTF("Last checkpoint, finishing\n");
  goto finish;
}

// DPRINTF("Buffered checkpoint\n");

if ( pagebuf_get(xch, ctx, &pagebuf, io_fd, dom) ) {

So, your code could be plugged right on top of the first "if".

+    tdatatmp = tdata;
+    tdata = pagebuf.tdata;
+    pagebuf.tdata = tdatatmp;

if (ctx->last_checkpoint)
...

@@ -2023,6 +2050,20 @@ int xc_domain_restore(xc_interface *xch, int
io_fd,
> uint32_t dom,
>     goto out;
>
>   finish_hvm:
> +    if ( callbacks != NULL && callbacks->toolstack_restore !=
NULL )
>
how about another (tdata.data != NULL) ? If older versions of xen (with
lets say older libxl toolstack)
were to migrate to xen 4.2, they wouldnt be sending the TOOLSTACK_ID would
they?

> +    {
> +        if ( callbacks->toolstack_restore(dom, tdata.data, tdata.len,
> +                    callbacks->data) < 0 )
> +        {
> +            PERROR("error calling toolstack_restore");
> +            free(tdata.data);
> +            free(tdata2.data);
> +            goto out;
> +        }
> +    }
> +    free(tdata.data);
> +    free(tdata2.data);
> +
>
Add free(buf->tdata.data) to pagebuf_free and
just do free(tdata.data) here.

cheers
shriram


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

All your comments make perfect sense, so I made all the changes you
suggested.

8<---


diff --git a/tools/libxc/xc_domain_restore.c b/tools/libxc/xc_domain_restore.c
index 3fda6f8..958534c 100644
--- a/tools/libxc/xc_domain_restore.c
+++ b/tools/libxc/xc_domain_restore.c
@@ -659,6 +659,11 @@ static void tailbuf_free(tailbuf_t *buf)
         tailbuf_free_pv(&buf->u.pv);
 }
 
+struct toolstack_data_t {
+    uint8_t *data;
+    uint32_t len;
+};
+
 typedef struct {
     void* pages;
     /* pages is of length nr_physpages, pfn_types is of length nr_pages */
@@ -682,6 +687,8 @@ typedef struct {
     uint64_t acpi_ioport_location;
     uint64_t viridian;
     uint64_t vm_generationid_addr;
+
+    struct toolstack_data_t tdata;
 } pagebuf_t;
 
 static int pagebuf_init(pagebuf_t* buf)
@@ -692,6 +699,10 @@ static int pagebuf_init(pagebuf_t* buf)
 
 static void pagebuf_free(pagebuf_t* buf)
 {
+    if (buf->tdata.data != NULL) {
+        free(buf->tdata.data);
+        buf->tdata.data = NULL;
+    }
     if (buf->pages) {
         free(buf->pages);
         buf->pages = NULL;
@@ -827,6 +838,19 @@ 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, &buf->tdata.len, sizeof(buf->tdata.len));
+            buf->tdata.data = (uint8_t*) realloc(buf->tdata.data,
buf->tdata.len);
+            if ( buf->tdata.data == NULL )
+            {
+                PERROR("error memory allocation");
+                return -1;
+            }
+            RDEXACT(fd, buf->tdata.data, buf->tdata.len);
+            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 +1286,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;
@@ -1310,6 +1335,7 @@ int xc_domain_restore(xc_interface *xch, int io_fd,
uint32_t dom,
 
     pagebuf_t pagebuf;
     tailbuf_t tailbuf, tmptail;
+    struct toolstack_data_t tdata, tdatatmp;
     void* vcpup;
     uint64_t console_pfn = 0;
 
@@ -1322,6 +1348,7 @@ int xc_domain_restore(xc_interface *xch, int io_fd,
uint32_t dom,
     pagebuf_init(&pagebuf);
     memset(&tailbuf, 0, sizeof(tailbuf));
     tailbuf.ishvm = hvm;
+    memset(&tdata, 0, sizeof(tdata));
 
     memset(ctx, 0, sizeof(*ctx));
 
@@ -1581,6 +1608,10 @@ int xc_domain_restore(xc_interface *xch, int io_fd,
uint32_t dom,
         ERROR("Error, unknow acpi ioport location (%i)",
pagebuf.acpi_ioport_location);
     }
 
+    tdatatmp = tdata;
+    tdata = pagebuf.tdata;
+    pagebuf.tdata = tdatatmp;
+
     if ( ctx->last_checkpoint )
     {
         // DPRINTF("Last checkpoint, finishing\n");
@@ -2023,6 +2054,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
&&
+            tdata.data != NULL )
+    {
+        if ( callbacks->toolstack_restore(dom, tdata.data, tdata.len,
+                    callbacks->data) < 0 )
+        {
+            PERROR("error calling toolstack_restore");
+            free(tdata.data);
+            goto out;
+        }
+    }
+    free(tdata.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 )
     {

On 2012-02-01, at 5:53 AM, Stefano Stabellini
<stefano.stabellini@eu.citrix.com> wrote:
> All your comments make perfect sense, so I made all the changes you
> suggested.
> 
> 8<---
> 
> 
> diff --git a/tools/libxc/xc_domain_restore.c
b/tools/libxc/xc_domain_restore.c
> index 3fda6f8..958534c 100644
> --- a/tools/libxc/xc_domain_restore.c
> +++ b/tools/libxc/xc_domain_restore.c
> @@ -659,6 +659,11 @@ static void tailbuf_free(tailbuf_t *buf)
>         tailbuf_free_pv(&buf->u.pv);
> }
> 
> +struct toolstack_data_t {
> +    uint8_t *data;
> +    uint32_t len;
> +};
> +
> typedef struct {
>     void* pages;
>     /* pages is of length nr_physpages, pfn_types is of length nr_pages */
> @@ -682,6 +687,8 @@ typedef struct {
>     uint64_t acpi_ioport_location;
>     uint64_t viridian;
>     uint64_t vm_generationid_addr;
> +
> +    struct toolstack_data_t tdata;
> } pagebuf_t;
> 
> static int pagebuf_init(pagebuf_t* buf)
> @@ -692,6 +699,10 @@ static int pagebuf_init(pagebuf_t* buf)
> 
> static void pagebuf_free(pagebuf_t* buf)
> {
> +    if (buf->tdata.data != NULL) {
> +        free(buf->tdata.data);
> +        buf->tdata.data = NULL;
> +    }
>     if (buf->pages) {
>         free(buf->pages);
>         buf->pages = NULL;
> @@ -827,6 +838,19 @@ 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, &buf->tdata.len,
sizeof(buf->tdata.len));
> +            buf->tdata.data = (uint8_t*) realloc(buf->tdata.data,
buf->tdata.len);
> +            if ( buf->tdata.data == NULL )
> +            {
> +                PERROR("error memory allocation");
> +                return -1;
> +            }
> +            RDEXACT(fd, buf->tdata.data, buf->tdata.len);
> +            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 +1286,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;
> @@ -1310,6 +1335,7 @@ int xc_domain_restore(xc_interface *xch, int io_fd,
uint32_t dom,
> 
>     pagebuf_t pagebuf;
>     tailbuf_t tailbuf, tmptail;
> +    struct toolstack_data_t tdata, tdatatmp;
>     void* vcpup;
>     uint64_t console_pfn = 0;
> 
> @@ -1322,6 +1348,7 @@ int xc_domain_restore(xc_interface *xch, int io_fd,
uint32_t dom,
>     pagebuf_init(&pagebuf);
>     memset(&tailbuf, 0, sizeof(tailbuf));
>     tailbuf.ishvm = hvm;
> +    memset(&tdata, 0, sizeof(tdata));
> 
>     memset(ctx, 0, sizeof(*ctx));
> 
> @@ -1581,6 +1608,10 @@ int xc_domain_restore(xc_interface *xch, int io_fd,
uint32_t dom,
>         ERROR("Error, unknow acpi ioport location (%i)",
pagebuf.acpi_ioport_location);
>     }
> 
> +    tdatatmp = tdata;
> +    tdata = pagebuf.tdata;
> +    pagebuf.tdata = tdatatmp;
> +
>     if ( ctx->last_checkpoint )
>     {
>         // DPRINTF("Last checkpoint, finishing\n");
> @@ -2023,6 +2054,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 &&
> +            tdata.data != NULL )
> +    {
> +        if ( callbacks->toolstack_restore(dom, tdata.data, tdata.len,
> +                    callbacks->data) < 0 )
> +        {
> +            PERROR("error calling toolstack_restore");
> +            free(tdata.data);
> +            goto out;
> +        }
> +    }
> +    free(tdata.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 )
>     {
> 
Acked-by: Shriram Rajagopalan <rshriram@cs.ubc.ca>

On Wed, 1 Feb 2012, Shriram Rajagopalan wrote:
> On 2012-02-01, at 5:53 AM, Stefano Stabellini
<stefano.stabellini@eu.citrix.com> wrote:
> 
> > All your comments make perfect sense, so I made all the changes you
> > suggested.
> > 
> > 8<---
> > 
> > 
> > diff --git a/tools/libxc/xc_domain_restore.c
b/tools/libxc/xc_domain_restore.c
> > index 3fda6f8..958534c 100644
> > --- a/tools/libxc/xc_domain_restore.c
> > +++ b/tools/libxc/xc_domain_restore.c
> > @@ -659,6 +659,11 @@ static void tailbuf_free(tailbuf_t *buf)
> >         tailbuf_free_pv(&buf->u.pv);
> > }
> > 
> > +struct toolstack_data_t {
> > +    uint8_t *data;
> > +    uint32_t len;
> > +};
> > +
> > typedef struct {
> >     void* pages;
> >     /* pages is of length nr_physpages, pfn_types is of length
nr_pages */
> > @@ -682,6 +687,8 @@ typedef struct {
> >     uint64_t acpi_ioport_location;
> >     uint64_t viridian;
> >     uint64_t vm_generationid_addr;
> > +
> > +    struct toolstack_data_t tdata;
> > } pagebuf_t;
> > 
> > static int pagebuf_init(pagebuf_t* buf)
> > @@ -692,6 +699,10 @@ static int pagebuf_init(pagebuf_t* buf)
> > 
> > static void pagebuf_free(pagebuf_t* buf)
> > {
> > +    if (buf->tdata.data != NULL) {
> > +        free(buf->tdata.data);
> > +        buf->tdata.data = NULL;
> > +    }
> >     if (buf->pages) {
> >         free(buf->pages);
> >         buf->pages = NULL;
> > @@ -827,6 +838,19 @@ 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, &buf->tdata.len,
sizeof(buf->tdata.len));
> > +            buf->tdata.data = (uint8_t*)
realloc(buf->tdata.data, buf->tdata.len);
> > +            if ( buf->tdata.data == NULL )
> > +            {
> > +                PERROR("error memory allocation");
> > +                return -1;
> > +            }
> > +            RDEXACT(fd, buf->tdata.data, buf->tdata.len);
> > +            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 +1286,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;
> > @@ -1310,6 +1335,7 @@ int xc_domain_restore(xc_interface *xch, int
io_fd, uint32_t dom,
> > 
> >     pagebuf_t pagebuf;
> >     tailbuf_t tailbuf, tmptail;
> > +    struct toolstack_data_t tdata, tdatatmp;
> >     void* vcpup;
> >     uint64_t console_pfn = 0;
> > 
> > @@ -1322,6 +1348,7 @@ int xc_domain_restore(xc_interface *xch, int
io_fd, uint32_t dom,
> >     pagebuf_init(&pagebuf);
> >     memset(&tailbuf, 0, sizeof(tailbuf));
> >     tailbuf.ishvm = hvm;
> > +    memset(&tdata, 0, sizeof(tdata));
> > 
> >     memset(ctx, 0, sizeof(*ctx));
> > 
> > @@ -1581,6 +1608,10 @@ int xc_domain_restore(xc_interface *xch, int
io_fd, uint32_t dom,
> >         ERROR("Error, unknow acpi ioport location (%i)",
pagebuf.acpi_ioport_location);
> >     }
> > 
> > +    tdatatmp = tdata;
> > +    tdata = pagebuf.tdata;
> > +    pagebuf.tdata = tdatatmp;
> > +
> >     if ( ctx->last_checkpoint )
> >     {
> >         // DPRINTF("Last checkpoint, finishing\n");
> > @@ -2023,6 +2054,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 &&
> > +            tdata.data != NULL )
> > +    {
> > +        if ( callbacks->toolstack_restore(dom, tdata.data,
tdata.len,
> > +                    callbacks->data) < 0 )
> > +        {
> > +            PERROR("error calling toolstack_restore");
> > +            free(tdata.data);
> > +            goto out;
> > +        }
> > +    }
> > +    free(tdata.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 )
> >     {
> > 
> 
> Acked-by: Shriram Rajagopalan <rshriram@cs.ubc.ca>
> 
Thanks, I''ll resend the patch with a proper commit message.