[PATCH 0/2] libxl: save/restore qemu physmap

[PATCH 0/2] libxl: save/restore qemu physmap

[Stefano Stabellini]

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.

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

 tools/libxc/xc_domain_restore.c |   66 ++++++++++++++------
 tools/libxc/xc_domain_save.c    |   16 +++++
 tools/libxc/xenguest.h          |   22 ++++++-
 tools/libxc/xg_save_restore.h   |    1 +
 tools/libxl/libxl_dom.c         |  132 ++++++++++++++++++++++++++++++++++++++-
 tools/xcutils/xc_restore.c      |    3 +-
 6 files changed, 218 insertions(+), 22 deletions(-)

Cheers,

Stefano

[PATCH 1/2] libxc: introduce XC_SAVE_ID_TOOLSTACK

[Stefano Stabellini]

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 |   66 +++++++++++++++++++++++++++-----------
 tools/libxc/xc_domain_save.c    |   16 +++++++++
 tools/libxc/xenguest.h          |   22 ++++++++++++-
 tools/libxc/xg_save_restore.h   |    1 +
 tools/libxl/libxl_dom.c         |    2 +-
 tools/xcutils/xc_restore.c      |    3 +-
 6 files changed, 88 insertions(+), 22 deletions(-)

diff --git a/tools/libxc/xc_domain_restore.c b/tools/libxc/xc_domain_restore.c
index 14451d1..72b6d5b 100644
--- a/tools/libxc/xc_domain_restore.c
+++ b/tools/libxc/xc_domain_restore.c
@@ -702,7 +702,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 restore_callbacks* callbacks)
 {
     int count, countpages, oldcount, i;
     void* ptmp;
@@ -725,7 +726,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, callbacks);
 
     case XC_SAVE_ID_VCPU_INFO:
         buf->new_ctxt_format = 1;
@@ -736,7 +737,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, callbacks);
 
     case XC_SAVE_ID_HVM_IDENT_PT:
         /* Skip padding 4 bytes then read the EPT identity PT location. */
@@ -747,7 +748,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, callbacks);
 
     case XC_SAVE_ID_HVM_VM86_TSS:
         /* Skip padding 4 bytes then read the vm86 TSS location. */
@@ -758,7 +759,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, callbacks);
 
     case XC_SAVE_ID_TMEM:
         DPRINTF("xc_domain_restore start tmem\n");
@@ -766,14 +767,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, callbacks);
 
     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, callbacks);
 
     case XC_SAVE_ID_TSC_INFO:
     {
@@ -787,7 +788,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, callbacks);
     }
 
     case XC_SAVE_ID_HVM_CONSOLE_PFN :
@@ -799,12 +800,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, callbacks);
 
     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, callbacks);
 
     case XC_SAVE_ID_HVM_ACPI_IOPORTS_LOCATION:
         /* Skip padding 4 bytes then read the acpi ioport location. */
@@ -814,7 +815,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, callbacks);
 
     case XC_SAVE_ID_HVM_VIRIDIAN:
         /* Skip padding 4 bytes then read the acpi ioport location. */
@@ -824,8 +825,33 @@ 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, callbacks);
 
+    case XC_SAVE_ID_TOOLSTACK:
+        {
+            uint32_t len;
+            uint8_t *buf2;
+            RDEXACT(fd, &len, sizeof(len));
+            buf2 = (uint8_t*) malloc(len);
+            if ( buf2 == NULL )
+            {
+                PERROR("error memory allocation");
+                return -1;
+            }
+            RDEXACT(fd, buf2, len);
+            if ( callbacks != NULL && callbacks->toolstack_restore != NULL )
+            {
+                if ( callbacks->toolstack_restore(dom,
+                            buf2, len, callbacks->data) < 0 )
+                {
+                    PERROR("error calling toolstack_restore");
+                    free(buf2);
+                    return -1;
+                }
+            }
+            free(buf2);
+            return pagebuf_get_one(xch, ctx, buf, fd, dom, callbacks);
+        }
     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
@@ -834,7 +860,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, callbacks);
 
     case XC_SAVE_ID_COMPRESSED_DATA:
 
@@ -902,7 +928,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, callbacks);
 
     oldcount = buf->nr_physpages;
     buf->nr_physpages += countpages;
@@ -927,7 +953,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 restore_callbacks *callbacks)
 {
     int rc;
 
@@ -935,7 +962,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, callbacks);
     } while (rc > 0);
 
     if (rc < 0)
@@ -1248,7 +1275,8 @@ static int apply_batch(xc_interface *xch, uint32_t dom, struct restore_ctx *ctx,
 int xc_domain_restore(xc_interface *xch, int io_fd, uint32_t dom,
                       unsigned int store_evtchn, unsigned long *store_mfn,
                       unsigned int console_evtchn, unsigned long *console_mfn,
-                      unsigned int hvm, unsigned int pae, int superpages)
+                      unsigned int hvm, unsigned int pae, int superpages,
+                      struct restore_callbacks* callbacks)
 {
     DECLARE_DOMCTL;
     int rc = 1, frc, i, j, n, m, pae_extended_cr3 = 0, ext_vcpucontext = 0;
@@ -1427,7 +1455,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, callbacks) < 0 ) {
                 PERROR("Error when reading batch");
                 goto out;
             }
@@ -1542,7 +1570,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, callbacks) ) {
         PERROR("error when buffering batch, finishing");
         goto finish;
     }
diff --git a/tools/libxc/xc_domain_save.c b/tools/libxc/xc_domain_save.c
index a6bb894..3d11c39 100644
--- a/tools/libxc/xc_domain_save.c
+++ b/tools/libxc/xc_domain_save.c
@@ -1676,6 +1676,22 @@ 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);
+    }
+
     if ( !callbacks->checkpoint )
     {
         /*
diff --git a/tools/libxc/xenguest.h b/tools/libxc/xenguest.h
index 4475ee9..8120715 100644
--- a/tools/libxc/xenguest.h
+++ b/tools/libxc/xenguest.h
@@ -44,6 +44,13 @@ 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 and frees the buffer.
+     */
+    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;
 };
@@ -61,6 +68,16 @@ int xc_domain_save(xc_interface *xch, int io_fd, uint32_t dom, uint32_t max_iter
                    struct save_callbacks* callbacks, int hvm);
 
 
+/* 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.
  *
@@ -72,12 +89,15 @@ int xc_domain_save(xc_interface *xch, int io_fd, uint32_t dom, uint32_t max_iter
  * @parm hvm non-zero if this is a HVM restore
  * @parm pae non-zero if this HVM domain has PAE support enabled
  * @parm superpages non-zero to allocate guest memory with superpages
+ * @parm 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,
                       unsigned int store_evtchn, unsigned long *store_mfn,
                       unsigned int console_evtchn, unsigned long *console_mfn,
-                      unsigned int hvm, unsigned int pae, int superpages);
+                      unsigned int hvm, unsigned int pae, int superpages,
+                      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 c5e0743..68feec5 100644
--- a/tools/libxc/xg_save_restore.h
+++ b/tools/libxc/xg_save_restore.h
@@ -253,6 +253,7 @@
 #define XC_SAVE_ID_HVM_VIRIDIAN       -11
 #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_TOOLSTACK          -14 /* 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 c898d89..fd2b051 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -373,7 +373,7 @@ int libxl__domain_restore_common(libxl__gc *gc, uint32_t domid,
     rc = xc_domain_restore(ctx->xch, fd, domid,
                            state->store_port, &state->store_mfn,
                            state->console_port, &state->console_mfn,
-                           hvm, pae, superpages);
+                           hvm, pae, superpages, 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 985cbec..eb1d31c 100644
--- a/tools/xcutils/xc_restore.c
+++ b/tools/xcutils/xc_restore.c
@@ -46,7 +46,8 @@ main(int argc, char **argv)
 	    superpages = !!hvm;
 
     ret = xc_domain_restore(xch, io_fd, domid, store_evtchn, &store_mfn,
-                            console_evtchn, &console_mfn, hvm, pae, superpages);
+                            console_evtchn, &console_mfn, hvm, pae, superpages,
+                            NULL);
 
     if ( ret == 0 )
     {
-- 
1.7.2.5

Re: [PATCH 1/2] libxc: introduce XC_SAVE_ID_TOOLSTACK

[Ian Campbell]

On Fri, 2012-01-20 at 11:18 +0000, Stefano Stabellini wrote:
> diff --git a/tools/libxc/xc_domain_save.c b/tools/libxc/xc_domain_save.c
> index a6bb894..3d11c39 100644
> --- a/tools/libxc/xc_domain_save.c
> +++ b/tools/libxc/xc_domain_save.c
> @@ -1676,6 +1676,22 @@ 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);

Where is buf free'd? You say below "callee allocates and frees the
buffer" but there is no way that can be the case since the caller uses
the buffer after the callback.

I think it has to be callee-alloc, caller-free (perhaps callee-free on
error).

> +    }
> +
>      if ( !callbacks->checkpoint )
>      {
>          /*
> diff --git a/tools/libxc/xenguest.h b/tools/libxc/xenguest.h
> index 4475ee9..8120715 100644
> --- a/tools/libxc/xenguest.h
> +++ b/tools/libxc/xenguest.h
> @@ -44,6 +44,13 @@ 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 and frees the buffer.
> +     */
> +    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;
>  };

Ian.

Re: [PATCH 1/2] libxc: introduce XC_SAVE_ID_TOOLSTACK

[Stefano Stabellini]

On Fri, 20 Jan 2012, Ian Campbell wrote:
> On Fri, 2012-01-20 at 11:18 +0000, Stefano Stabellini wrote:
> > diff --git a/tools/libxc/xc_domain_save.c b/tools/libxc/xc_domain_save.c
> > index a6bb894..3d11c39 100644
> > --- a/tools/libxc/xc_domain_save.c
> > +++ b/tools/libxc/xc_domain_save.c
> > @@ -1676,6 +1676,22 @@ 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);
> 
> Where is buf free'd? You say below "callee allocates and frees the
> buffer" but there is no way that can be the case since the caller uses
> the buffer after the callback.

The callee can free the buffer after the completion of xc_domain_save.
So libxl allocates the buffer in toolstack_save and frees it after
xc_domain_save returns.


> I think it has to be callee-alloc, caller-free (perhaps callee-free on
> error).

I though that it is more straightforward if the callee does both, but if
you think that is actually more confusing, I'll change it.

Re: [PATCH 1/2] libxc: introduce XC_SAVE_ID_TOOLSTACK

[Ian Campbell]

On Fri, 2012-01-20 at 12:24 +0000, Stefano Stabellini wrote:
> On Fri, 20 Jan 2012, Ian Campbell wrote:
> > On Fri, 2012-01-20 at 11:18 +0000, Stefano Stabellini wrote:
> > > diff --git a/tools/libxc/xc_domain_save.c b/tools/libxc/xc_domain_save.c
> > > index a6bb894..3d11c39 100644
> > > --- a/tools/libxc/xc_domain_save.c
> > > +++ b/tools/libxc/xc_domain_save.c
> > > @@ -1676,6 +1676,22 @@ 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);
> > 
> > Where is buf free'd? You say below "callee allocates and frees the
> > buffer" but there is no way that can be the case since the caller uses
> > the buffer after the callback.
> 
> The callee can free the buffer after the completion of xc_domain_save.
> So libxl allocates the buffer in toolstack_save and frees it after
> xc_domain_save returns.
> 
> 
> > I think it has to be callee-alloc, caller-free (perhaps callee-free on
> > error).
> 
> I though that it is more straightforward if the callee does both, but if
> you think that is actually more confusing, I'll change it.

Well, it enforces that the caller has some sort of infrastructure (like
gc) which they may not have. I suppose they could have a little struct
passed via the closure where they stash the allocation to free after
xc_domain_save but I'd contend that this is even weirder/more confusing
than doing caller-free -- you just don't see this because libxl's gc
hides it from you.

Ian.

Re: [PATCH 1/2] libxc: introduce XC_SAVE_ID_TOOLSTACK

[Tim Deegan]

At 12:32 +0000 on 20 Jan (1327062730), Ian Campbell wrote:
> On Fri, 2012-01-20 at 12:24 +0000, Stefano Stabellini wrote:
> > On Fri, 20 Jan 2012, Ian Campbell wrote:
> > > On Fri, 2012-01-20 at 11:18 +0000, Stefano Stabellini wrote:
> > > > diff --git a/tools/libxc/xc_domain_save.c b/tools/libxc/xc_domain_save.c
> > > > index a6bb894..3d11c39 100644
> > > > --- a/tools/libxc/xc_domain_save.c
> > > > +++ b/tools/libxc/xc_domain_save.c
> > > > @@ -1676,6 +1676,22 @@ 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);
> > > 
> > > Where is buf free'd? You say below "callee allocates and frees the
> > > buffer" but there is no way that can be the case since the caller uses
> > > the buffer after the callback.
> > 
> > The callee can free the buffer after the completion of xc_domain_save.
> > So libxl allocates the buffer in toolstack_save and frees it after
> > xc_domain_save returns.
> > 
> > 
> > > I think it has to be callee-alloc, caller-free (perhaps callee-free on
> > > error).
> > 
> > I though that it is more straightforward if the callee does both, but if
> > you think that is actually more confusing, I'll change it.
> 
> Well, it enforces that the caller has some sort of infrastructure (like
> gc) which they may not have. I suppose they could have a little struct
> passed via the closure where they stash the allocation to free after
> xc_domain_save but I'd contend that this is even weirder/more confusing
> than doing caller-free -- you just don't see this because libxl's gc
> hides it from you.

Other idioms for this sort of thing include having two callbacks, the
first of which returns the size (erk), or two callbacks, one just for
freeing the pointer (meh).  On *NIx, requring that the callback return a
pointer suitable for free()ing is OK, I think.  On Windows that kind of
thing can be a real PITA, trying to be sure that both sides have the
same heap.   But maybe that's a bridge better crossed when we come to
it. 

Tim.

[PATCH 2/2] libxl: save/restore qemu's physmap

[Stefano Stabellini]

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 fd2b051..3d60a35 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -347,6 +347,62 @@ out:
     return rc;
 }
 
+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 namelen = 0;
+    char *name = NULL;
+    uint32_t count = 0;
+    uint64_t phys_offset_v = 0, start_addr_v = 0, size_v = 0;
+
+    if (size < sizeof(count))
+        return -1;
+
+    memcpy(&count, ptr, sizeof(count));
+    ptr += sizeof(count);
+ 
+    if (size <
+            sizeof(count) + count * (sizeof(uint64_t) * 3 + sizeof(uint32_t)))
+        return -1;
+
+    for (i = 0; i < count; i++) {
+        memcpy(&namelen, ptr, sizeof(namelen));
+        ptr += sizeof(namelen);
+        if (namelen > 0) {
+            name = (char *)ptr;
+            ptr += namelen;
+        }
+        memcpy(&phys_offset_v, ptr, sizeof(uint64_t));
+        ptr += sizeof(uint64_t);
+        memcpy(&start_addr_v, ptr, sizeof(uint64_t));
+        ptr += sizeof(uint64_t);
+        memcpy(&size_v, ptr, sizeof(uint64_t));
+        ptr += sizeof(uint64_t);
+
+        ret = libxl__xs_write(gc, 0, libxl__sprintf(gc,
+                    "/local/domain/0/device-model/%d/physmap/%"PRIx64"/start_addr",
+                    domid, phys_offset_v), "%"PRIx64, start_addr_v);
+        if (ret)
+            return -1;
+        ret = libxl__xs_write(gc, 0, libxl__sprintf(gc,
+                    "/local/domain/0/device-model/%d/physmap/%"PRIx64"/size",
+                    domid, phys_offset_v), "%"PRIx64, size_v);
+        if (ret)
+            return -1;
+        if (namelen > 0) {
+            ret = libxl__xs_write(gc, 0, libxl__sprintf(gc,
+                        "/local/domain/0/device-model/%d/physmap/%"PRIx64"/name",
+                        domid, phys_offset_v), "%s", 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,
@@ -356,11 +412,14 @@ int libxl__domain_restore_common(libxl__gc *gc, uint32_t domid,
     /* read signature */
     int rc;
     int hvm, pae, superpages;
+    struct restore_callbacks callbacks;
     switch (info->type) {
     case LIBXL_DOMAIN_TYPE_HVM:
         hvm = 1;
         superpages = 1;
         pae = info->u.hvm.pae;
+        callbacks.toolstack_restore = libxl__toolstack_restore;
+        callbacks.data = gc;
         break;
     case LIBXL_DOMAIN_TYPE_PV:
         hvm = 0;
@@ -373,7 +432,7 @@ int libxl__domain_restore_common(libxl__gc *gc, uint32_t domid,
     rc = xc_domain_restore(ctx->xch, fd, domid,
                            state->store_port, &state->store_mfn,
                            state->console_port, &state->console_mfn,
-                           hvm, pae, superpages, NULL);
+                           hvm, pae, superpages, &callbacks);
     if ( rc ) {
         LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "restoring domain");
         return ERROR_FAIL;
@@ -528,6 +587,76 @@ 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;
+    char *start_addr = NULL, *size = NULL, *phys_offset = NULL, *name = NULL;
+    int i = 0;
+    unsigned int num = 0;
+    uint32_t count = 0;
+    uint8_t *ptr = NULL;
+    char **entries = NULL;
+    uint64_t val = 0;
+    uint32_t namelen = 0;
+
+    entries = libxl__xs_directory(gc, 0, libxl__sprintf(gc,
+                "/local/domain/0/device-model/%d/physmap", domid), &num);
+    count = num;
+
+    *len = sizeof(count) + count * (sizeof(val) * 3 + sizeof(namelen));
+    *buf = libxl__calloc(gc, 1, *len);
+    ptr = *buf;
+
+    memcpy(ptr, &count, sizeof(count));
+    ptr += sizeof(count);
+
+    for (i = 0; i < count; i++) {
+        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 {
+            unsigned long offset;
+            namelen = strlen(name) + 1;
+            *len += namelen;
+            offset = ptr - (*buf);
+            *buf = libxl__realloc(gc, *buf, *len);
+            ptr = (*buf) + offset;
+        }
+        memcpy(ptr, &namelen, sizeof(namelen));
+        ptr += sizeof(namelen);
+        if (namelen > 0) {
+            memcpy(ptr, name, namelen);
+            ptr += namelen;
+        }
+        val = strtoll(phys_offset, NULL, 16);
+        memcpy(ptr, &val, sizeof(val));
+        ptr += sizeof(val);
+        val = strtoll(start_addr, NULL, 16);
+        memcpy(ptr, &val, sizeof(val));
+        ptr += sizeof(val);
+        val = strtoll(size, NULL, 16);
+        memcpy(ptr, &val, sizeof(val));
+        ptr += sizeof(val);
+    }
+
+    return 0;
+}
+
 int libxl__domain_suspend_common(libxl__gc *gc, uint32_t domid, int fd,
                                  libxl_domain_type type,
                                  int live, int debug)
@@ -579,6 +708,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, hvm);
-- 
1.7.2.5

Re: [PATCH 2/2] libxl: save/restore qemu's physmap

[Ian Campbell]

On Fri, 2012-01-20 at 11:18 +0000, Stefano Stabellini wrote:
> Read Qemu's physmap from xenstore and save it using toolstack_save.
> Restore Qemu's physmap using toolstack_restore.

Shall we have a version field now so we don't dig ourselves a hole we
can't get out of?

> 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 fd2b051..3d60a35 100644
> --- a/tools/libxl/libxl_dom.c
> +++ b/tools/libxl/libxl_dom.c
> @@ -347,6 +347,62 @@ out:
>      return rc;
>  }
>  
> +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 namelen = 0;
> +    char *name = NULL;
> +    uint32_t count = 0;
> +    uint64_t phys_offset_v = 0, start_addr_v = 0, size_v = 0;
> +
> +    if (size < sizeof(count))
> +        return -1;
> +
> +    memcpy(&count, ptr, sizeof(count));
> +    ptr += sizeof(count);
> + 
> +    if (size <
> +            sizeof(count) + count * (sizeof(uint64_t) * 3 + sizeof(uint32_t)))
> +        return -1;
> +
> +    for (i = 0; i < count; i++) {
> +        memcpy(&namelen, ptr, sizeof(namelen));
> +        ptr += sizeof(namelen);
> +        if (namelen > 0) {
> +            name = (char *)ptr;
> +            ptr += namelen;
> +        }
> +        memcpy(&phys_offset_v, ptr, sizeof(uint64_t));
> +        ptr += sizeof(uint64_t);
> +        memcpy(&start_addr_v, ptr, sizeof(uint64_t));
> +        ptr += sizeof(uint64_t);
> +        memcpy(&size_v, ptr, sizeof(uint64_t));
> +        ptr += sizeof(uint64_t);

Why not define a struct libxl__physmap_info for these three? You could
even do the trick with the "char name[]" at the end and incorporate
namelen too if you wanted.

The maximum size of the allocation is
	sizeof(count) + count * (sizeof(uint64_t) * 3 + sizeof(uint32_t)))
here but in the save it is allocating:
	sizeof(count) + count * (sizeof(val) * 3 + sizeof(namelen));
these work out the same but it would be more obviously correct if it
were "count * sizeof(struct)"

Actually, hang on where is the space for name itself allocated? I see,
you are reallocing. If you have to do that anyway you may as well start
off with just sizeof(count) and realloc namelen + sizeof(struct) at each
stage.

> +
> +        ret = libxl__xs_write(gc, 0, libxl__sprintf(gc,
> +                    "/local/domain/0/device-model/%d/physmap/%"PRIx64"/start_addr",
> +                    domid, phys_offset_v), "%"PRIx64, start_addr_v);
> +        if (ret)
> +            return -1;
> +        ret = libxl__xs_write(gc, 0, libxl__sprintf(gc,
> +                    "/local/domain/0/device-model/%d/physmap/%"PRIx64"/size",
> +                    domid, phys_offset_v), "%"PRIx64, size_v);
> +        if (ret)
> +            return -1;
> +        if (namelen > 0) {
> +            ret = libxl__xs_write(gc, 0, libxl__sprintf(gc,
> +                        "/local/domain/0/device-model/%d/physmap/%"PRIx64"/name",
> +                        domid, phys_offset_v), "%s", 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,

Re: [PATCH 2/2] libxl: save/restore qemu's physmap

[Stefano Stabellini]

On Fri, 20 Jan 2012, Ian Campbell wrote:
> On Fri, 2012-01-20 at 11:18 +0000, Stefano Stabellini wrote:
> > Read Qemu's physmap from xenstore and save it using toolstack_save.
> > Restore Qemu's physmap using toolstack_restore.
> 
> Shall we have a version field now so we don't dig ourselves a hole we
> can't get out of?

good idea

> > 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 fd2b051..3d60a35 100644
> > --- a/tools/libxl/libxl_dom.c
> > +++ b/tools/libxl/libxl_dom.c
> > @@ -347,6 +347,62 @@ out:
> >      return rc;
> >  }
> >  
> > +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 namelen = 0;
> > +    char *name = NULL;
> > +    uint32_t count = 0;
> > +    uint64_t phys_offset_v = 0, start_addr_v = 0, size_v = 0;
> > +
> > +    if (size < sizeof(count))
> > +        return -1;
> > +
> > +    memcpy(&count, ptr, sizeof(count));
> > +    ptr += sizeof(count);
> > + 
> > +    if (size <
> > +            sizeof(count) + count * (sizeof(uint64_t) * 3 + sizeof(uint32_t)))
> > +        return -1;
> > +
> > +    for (i = 0; i < count; i++) {
> > +        memcpy(&namelen, ptr, sizeof(namelen));
> > +        ptr += sizeof(namelen);
> > +        if (namelen > 0) {
> > +            name = (char *)ptr;
> > +            ptr += namelen;
> > +        }
> > +        memcpy(&phys_offset_v, ptr, sizeof(uint64_t));
> > +        ptr += sizeof(uint64_t);
> > +        memcpy(&start_addr_v, ptr, sizeof(uint64_t));
> > +        ptr += sizeof(uint64_t);
> > +        memcpy(&size_v, ptr, sizeof(uint64_t));
> > +        ptr += sizeof(uint64_t);
> 
> Why not define a struct libxl__physmap_info for these three? You could
> even do the trick with the "char name[]" at the end and incorporate
> namelen too if you wanted.
> 
> The maximum size of the allocation is
> 	sizeof(count) + count * (sizeof(uint64_t) * 3 + sizeof(uint32_t)))
> here but in the save it is allocating:
> 	sizeof(count) + count * (sizeof(val) * 3 + sizeof(namelen));
> these work out the same but it would be more obviously correct if it
> were "count * sizeof(struct)"
> 
> Actually, hang on where is the space for name itself allocated? I see,
> you are reallocing. If you have to do that anyway you may as well start
> off with just sizeof(count) and realloc namelen + sizeof(struct) at each
> stage.

I played with the idea of the struct for a bit and it has improved the
code, especially on restore.