[PATCH v6 0/3] libxl: save/restore qemu physmap

[PATCH v6 0/3] 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.

The QEMU side of this patch series has been accepted, so it is now safe
to commit it to xen-unstable.


Changes in v6:

- rebased on "kexec: Fix printing of paddr_t in 32bit mode.";

- use the command "xen-save-devices-state" to save the QEMU devices
state.



Changes in v5:

- rebased on "arm: lr register in hyp mode is really LR_usr.".



Changes in v4:

- addressed Shriram's comments about the first patch: saving/restoring
toolstack extra information should work we Remus too now;

- added a new patch to use QMP "save_devices" command rather than
"migrate" to save QEMU's device state.



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 (3):
      libxc: introduce XC_SAVE_ID_TOOLSTACK
      libxl: save/restore qemu's physmap
      libxl_qmp: remove libxl__qmp_migrate, introduce libxl__qmp_save

 tools/libxc/xc_domain_restore.c |   46 ++++++++++++-
 tools/libxc/xc_domain_save.c    |   17 +++++
 tools/libxc/xenguest.h          |   23 ++++++-
 tools/libxc/xg_save_restore.h   |    1 +
 tools/libxl/libxl_dom.c         |  143 ++++++++++++++++++++++++++++++++++++---
 tools/libxl/libxl_internal.h    |    2 +-
 tools/libxl/libxl_qmp.c         |   82 +---------------------
 tools/xcutils/xc_restore.c      |    2 +-
 8 files changed, 222 insertions(+), 94 deletions(-)


Cheers,

Stefano

[PATCH v6 1/3] 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 |   46 ++++++++++++++++++++++++++++++++++++++-
 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, 87 insertions(+), 4 deletions(-)

diff --git a/tools/libxc/xc_domain_restore.c b/tools/libxc/xc_domain_restore.c
index 3e4d518..90408e6 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 */
@@ -685,6 +690,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)
@@ -695,6 +702,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;
@@ -863,6 +874,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
@@ -1299,7 +1323,8 @@ int xc_domain_restore(xc_interface *xch, int io_fd, uint32_t dom,
                       unsigned long *console_mfn, domid_t console_domid,
                       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;
@@ -1347,6 +1372,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;
 
@@ -1359,6 +1385,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));
 
@@ -1624,6 +1651,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");
@@ -2074,6 +2105,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 a9216dd..fcc7718 100644
--- a/tools/libxc/xc_domain_save.c
+++ b/tools/libxc/xc_domain_save.c
@@ -1723,6 +1723,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 8d885d3..6435f65 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,
@@ -83,7 +103,8 @@ int xc_domain_restore(xc_interface *xch, int io_fd, uint32_t dom,
                       unsigned long *console_mfn, domid_t console_domid,
                       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 89f3504..04e7892 100644
--- a/tools/libxc/xg_save_restore.h
+++ b/tools/libxc/xg_save_restore.h
@@ -258,6 +258,7 @@
 #define XC_SAVE_ID_HVM_PAGING_RING_PFN  -15
 #define XC_SAVE_ID_HVM_ACCESS_RING_PFN  -16
 #define XC_SAVE_ID_HVM_SHARING_RING_PFN -17
+#define XC_SAVE_ID_TOOLSTACK          -18 /* 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 9b33267..5c4c972 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -422,7 +422,7 @@ int libxl__domain_restore_common(libxl__gc *gc, uint32_t domid,
                            state->store_domid, state->console_port,
                            &state->console_mfn, state->console_domid,
                            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 e41a133..0235579 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, 0,
                             console_evtchn, &console_mfn, 0, hvm, pae, superpages,
-                            0, NULL);
+                            0, NULL, NULL);
 
     if ( ret == 0 )
     {
-- 
1.7.2.5

Re: [PATCH v6 1/3] libxc: introduce XC_SAVE_ID_TOOLSTACK

[Ian Jackson]

Stefano Stabellini writes ("[Xen-devel] [PATCH v6 1/3] libxc: introduce XC_SAVE_ID_TOOLSTACK"):
> Introduce a new save_id to save/restore toolstack specific extra
> information.

This looks plausible.

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

Although I do have one comment: are you sure it's appropriate that the
"toolstack data" is silently thrown away if the restore caller doesn't
supply the relevant callback ?

Ian.

Re: [PATCH v6 1/3] libxc: introduce XC_SAVE_ID_TOOLSTACK

[Stefano Stabellini]

On Tue, 3 Apr 2012, Ian Jackson wrote:
> Stefano Stabellini writes ("[Xen-devel] [PATCH v6 1/3] libxc: introduce XC_SAVE_ID_TOOLSTACK"):
> > Introduce a new save_id to save/restore toolstack specific extra
> > information.
> 
> This looks plausible.
> 
> Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
> 
> Although I do have one comment: are you sure it's appropriate that the
> "toolstack data" is silently thrown away if the restore caller doesn't
> supply the relevant callback ?

We should print a warning in that case and try to continue

Re: [PATCH v6 1/3] libxc: introduce XC_SAVE_ID_TOOLSTACK

[Ian Jackson]

Stefano Stabellini writes ("Re: [Xen-devel] [PATCH v6 1/3] libxc: introduce XC_SAVE_ID_TOOLSTACK"):
> On Tue, 3 Apr 2012, Ian Jackson wrote:
...
> > Although I do have one comment: are you sure it's appropriate that the
> > "toolstack data" is silently thrown away if the restore caller doesn't
> > supply the relevant callback ?
> 
> We should print a warning in that case and try to continue

Why is it not appropriate to bomb out ?  I am not a fan of warnings
(which end up dumped to some ignored logfile) for things which might
be critical problems.

Ian.

Re: [PATCH v6 1/3] libxc: introduce XC_SAVE_ID_TOOLSTACK

[Stefano Stabellini]

On Thu, 12 Apr 2012, Ian Jackson wrote:
> Stefano Stabellini writes ("Re: [Xen-devel] [PATCH v6 1/3] libxc: introduce XC_SAVE_ID_TOOLSTACK"):
> > On Tue, 3 Apr 2012, Ian Jackson wrote:
> ...
> > > Although I do have one comment: are you sure it's appropriate that the
> > > "toolstack data" is silently thrown away if the restore caller doesn't
> > > supply the relevant callback ?
> > 
> > We should print a warning in that case and try to continue
> 
> Why is it not appropriate to bomb out ?  I am not a fan of warnings
> (which end up dumped to some ignored logfile) for things which might
> be critical problems.
 
because there is a significant chance that the guest will resume
correctly anyway (it depends on the guest status and the VM config)

Re: [PATCH v6 1/3] libxc: introduce XC_SAVE_ID_TOOLSTACK

[Ian Campbell]

On Thu, 2012-04-12 at 12:58 +0100, Stefano Stabellini wrote:
> On Thu, 12 Apr 2012, Ian Jackson wrote:
> > Stefano Stabellini writes ("Re: [Xen-devel] [PATCH v6 1/3] libxc: introduce XC_SAVE_ID_TOOLSTACK"):
> > > On Tue, 3 Apr 2012, Ian Jackson wrote:
> > ...
> > > > Although I do have one comment: are you sure it's appropriate that the
> > > > "toolstack data" is silently thrown away if the restore caller doesn't
> > > > supply the relevant callback ?
> > > 
> > > We should print a warning in that case and try to continue
> > 
> > Why is it not appropriate to bomb out ?  I am not a fan of warnings
> > (which end up dumped to some ignored logfile) for things which might
> > be critical problems.
>  
> because there is a significant chance that the guest will resume
> correctly anyway (it depends on the guest status and the VM config)

But there is some non-zero chance that it won't, in which case we've
just silently killed the destination VM and told the source machine that
everything is OK, so it won't resume the original. That's not good. I
think we should propagate the error here.

Ian.

Re: [PATCH v6 1/3] libxc: introduce XC_SAVE_ID_TOOLSTACK

[Stefano Stabellini]

On Thu, 12 Apr 2012, Ian Campbell wrote:
> On Thu, 2012-04-12 at 12:58 +0100, Stefano Stabellini wrote:
> > On Thu, 12 Apr 2012, Ian Jackson wrote:
> > > Stefano Stabellini writes ("Re: [Xen-devel] [PATCH v6 1/3] libxc: introduce XC_SAVE_ID_TOOLSTACK"):
> > > > On Tue, 3 Apr 2012, Ian Jackson wrote:
> > > ...
> > > > > Although I do have one comment: are you sure it's appropriate that the
> > > > > "toolstack data" is silently thrown away if the restore caller doesn't
> > > > > supply the relevant callback ?
> > > > 
> > > > We should print a warning in that case and try to continue
> > > 
> > > Why is it not appropriate to bomb out ?  I am not a fan of warnings
> > > (which end up dumped to some ignored logfile) for things which might
> > > be critical problems.
> >  
> > because there is a significant chance that the guest will resume
> > correctly anyway (it depends on the guest status and the VM config)
> 
> But there is some non-zero chance that it won't, in which case we've
> just silently killed the destination VM and told the source machine that
> everything is OK, so it won't resume the original. That's not good. I
> think we should propagate the error here.

OK

[PATCH v6 2/3] 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 5c4c972..d6b4a90 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -391,6 +391,66 @@ int libxl__qemu_traditional_cmd(libxl__gc *gc, uint32_t domid,
     return libxl__xs_write(gc, XBT_NULL, path, "%s", cmd);
 }
 
+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,
@@ -400,6 +460,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:
@@ -407,6 +468,8 @@ int libxl__domain_restore_common(libxl__gc *gc, uint32_t domid,
         superpages = 1;
         pae = libxl_defbool_val(info->u.hvm.pae);
         no_incr_generationid = !libxl_defbool_val(info->u.hvm.incr_generationid);
+        callbacks.toolstack_restore = libxl__toolstack_restore;
+        callbacks.data = gc;
         break;
     case LIBXL_DOMAIN_TYPE_PV:
         hvm = 0;
@@ -422,7 +485,7 @@ int libxl__domain_restore_common(libxl__gc *gc, uint32_t domid,
                            state->store_domid, state->console_port,
                            &state->console_mfn, state->console_domid,
                            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;
@@ -577,6 +640,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)
@@ -639,6 +768,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

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

[Ian Jackson]

Stefano Stabellini writes ("[Xen-devel] [PATCH v6 2/3] libxl: save/restore qemu's physmap"):
> Read Qemu's physmap from xenstore and save it using toolstack_save.
> Restore Qemu's physmap using toolstack_restore.

> +static int libxl__toolstack_restore(uint32_t domid, uint8_t *buf,
> +        uint32_t size, void *data)
...
> +    if (size < sizeof(version) + sizeof(count))
> +        return -1;
> +
> +    memcpy(&version, ptr, sizeof(version));
> +    ptr += sizeof(version);
> +
> +    if (version != TOOLSTACK_SAVE_VERSION)
> +        return -1;

Surely these error exits need log messages.

> +    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",

Long line.

> +                    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);

This whole thing contains a lot of repetitive code.  Can you perhaps
break the xs_write into a helper function and then you'd make the
repetition more explicit by writing something like:

           helper(gc, domid, "start_addr", "%"PRIx64, pi->start_addr);
           helper(gc, domid, "name",       "%"PRIx64, pi->size);
           if (pi->namelen)
                helper(gc, domid, "name",       "%s",      pi->name);

> +static int libxl__toolstack_save(uint32_t domid, uint8_t **buf,
> +        uint32_t *len, void *data)
> +{
...
> +    *buf = calloc(1, *len);

Surely this should come from the gc.

> +        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));

Again, quite a lot of repetition which obscures the similarities
between the different calls.

> +        if (start_addr == NULL || size == NULL || phys_offset == NULL)
> +            return -1;

This seems a rather odd condition.  Surely it is an error if only some
of these parameters are in xenstore ?

> +        if (name == NULL)
> +            namelen = 0;
> +        else
> +            namelen = strlen(name) + 1;
> +        *len += namelen + sizeof(struct libxl__physmap_info);
> +        offset = ptr - (*buf);
> +        *buf = realloc(*buf, *len);

Shouldn't this come from the gc ?

Ian.

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

[Stefano Stabellini]

On Tue, 3 Apr 2012, Ian Jackson wrote:
> > +                    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);
> 
> This whole thing contains a lot of repetitive code.  Can you perhaps
> break the xs_write into a helper function and then you'd make the
> repetition more explicit by writing something like:
> 
>            helper(gc, domid, "start_addr", "%"PRIx64, pi->start_addr);
>            helper(gc, domid, "name",       "%"PRIx64, pi->size);
>            if (pi->namelen)
>                 helper(gc, domid, "name",       "%s",      pi->name);

I'll try to make the code more readable.


> > +static int libxl__toolstack_save(uint32_t domid, uint8_t **buf,
> > +        uint32_t *len, void *data)
> > +{
> ...
> > +    *buf = calloc(1, *len);
> 
> Surely this should come from the gc.

Nope: libxc frees it.

[PATCH v6 3/3] libxl_qmp: remove libxl__qmp_migrate, introduce libxl__qmp_save

[Stefano Stabellini]

Following the recent changes to upstream Qemu, the best monitor command
to suit or needs is "xen-save-devices-state" rather than "migrate".
This patch removes libxl__qmp_migrate and introduces libxl__qmp_save
instead, that uses "xen-save-devices-state".

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 tools/libxl/libxl_dom.c      |   11 +-----
 tools/libxl/libxl_internal.h |    2 +-
 tools/libxl/libxl_qmp.c      |   82 ++----------------------------------------
 3 files changed, 5 insertions(+), 90 deletions(-)

diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index d6b4a90..5d1d65d 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -811,18 +811,9 @@ int libxl__domain_save_device_model(libxl__gc *gc, uint32_t domid, int fd)
         break;
     }
     case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN:
-        fd2 = open(filename, O_WRONLY | O_CREAT | O_TRUNC, S_IRUSR | S_IWUSR);
-        if (fd2 < 0) {
-            LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR,
-                             "Unable to create a QEMU save file\n");
-            return ERROR_FAIL;
-        }
-        /* Save DM state into fd2 */
-        ret = libxl__qmp_migrate(gc, domid, fd2);
+        ret = libxl__qmp_save(gc, domid, (char *)filename);
         if (ret)
             goto out;
-        close(fd2);
-        fd2 = -1;
         break;
     default:
         return ERROR_INVAL;
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index e0a1070..b5cce17 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1028,7 +1028,7 @@ _hidden int libxl__qmp_pci_add(libxl__gc *gc, int d, libxl_device_pci *pcidev);
 _hidden int libxl__qmp_pci_del(libxl__gc *gc, int domid,
                                libxl_device_pci *pcidev);
 /* Save current QEMU state into fd. */
-_hidden int libxl__qmp_migrate(libxl__gc *gc, int domid, int fd);
+_hidden int libxl__qmp_save(libxl__gc *gc, int domid, const char *filename);
 /* close and free the QMP handler */
 _hidden void libxl__qmp_close(libxl__qmp_handler *qmp);
 /* remove the socket file, if the file has already been removed,
diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
index f5a3edc..9a6ecb6 100644
--- a/tools/libxl/libxl_qmp.c
+++ b/tools/libxl/libxl_qmp.c
@@ -538,52 +538,6 @@ out:
     return rc;
 }
 
-static int qmp_send_fd(libxl__gc *gc, libxl__qmp_handler *qmp,
-                       libxl_key_value_list *args,
-                       qmp_callback_t callback, void *opaque,
-                       qmp_request_context *context,
-                       int fd)
-{
-    struct msghdr msg = { 0 };
-    struct cmsghdr *cmsg;
-    char control[CMSG_SPACE(sizeof (fd))];
-    struct iovec iov;
-    char *buf = NULL;
-
-    buf = qmp_send_prepare(gc, qmp, "getfd", args, callback, opaque, context);
-
-    /* Response data */
-    iov.iov_base = buf;
-    iov.iov_len  = strlen(buf);
-
-    /* compose the message */
-    msg.msg_iov = &iov;
-    msg.msg_iovlen = 1;
-    msg.msg_control = control;
-    msg.msg_controllen = sizeof (control);
-
-    /* attach open fd */
-    cmsg = CMSG_FIRSTHDR(&msg);
-    cmsg->cmsg_level = SOL_SOCKET;
-    cmsg->cmsg_type = SCM_RIGHTS;
-    cmsg->cmsg_len = CMSG_LEN(sizeof (fd));
-    *(int *)CMSG_DATA(cmsg) = fd;
-
-    msg.msg_controllen = cmsg->cmsg_len;
-
-    if (sendmsg(qmp->qmp_fd, &msg, 0) < 0) {
-        LIBXL__LOG_ERRNO(qmp->ctx, LIBXL__LOG_ERROR,
-                         "Failed to send a QMP message to QEMU.");
-        return ERROR_FAIL;
-    }
-    if (libxl_write_exactly(qmp->ctx, qmp->qmp_fd, "\r\n", 2,
-                            "CRLF", "QMP socket")) {
-        return ERROR_FAIL;
-    }
-
-    return qmp->last_id_used;
-}
-
 static int qmp_synchronous_send(libxl__qmp_handler *qmp, const char *cmd,
                                 libxl_key_value_list *args,
                                 qmp_callback_t callback, void *opaque,
@@ -817,34 +771,8 @@ int libxl__qmp_pci_del(libxl__gc *gc, int domid, libxl_device_pci *pcidev)
     return qmp_device_del(gc, domid, id);
 }
 
-static int qmp_getfd(libxl__gc *gc, libxl__qmp_handler *qmp,
-                     int fd, const char *name)
-{
-    flexarray_t *parameters = NULL;
-    libxl_key_value_list args = NULL;
-    int rc = 0;
-
-    parameters = flexarray_make(2, 1);
-    if (!parameters)
-        return ERROR_NOMEM;
-    flexarray_append_pair(parameters, "fdname", (char*)name);
-    args = libxl__xs_kvs_of_flexarray(gc, parameters, parameters->count);
-    if (!args) {
-        rc = ERROR_NOMEM;
-        goto out;
-    }
-
-    if (qmp_send_fd(gc, qmp, &args, NULL, NULL, NULL, fd) < 0) {
-        rc = ERROR_FAIL;
-    }
-out:
-    flexarray_free(parameters);
-    return rc;
-}
-
-int libxl__qmp_migrate(libxl__gc *gc, int domid, int fd)
+int libxl__qmp_save(libxl__gc *gc, int domid, const char *filename)
 {
-#define MIGRATE_FD_NAME "dm-migrate"
     libxl__qmp_handler *qmp = NULL;
     flexarray_t *parameters = NULL;
     libxl_key_value_list args = NULL;
@@ -854,23 +782,19 @@ int libxl__qmp_migrate(libxl__gc *gc, int domid, int fd)
     if (!qmp)
         return ERROR_FAIL;
 
-    rc = qmp_getfd(gc, qmp, fd, MIGRATE_FD_NAME);
-    if (rc)
-        goto out;
-
     parameters = flexarray_make(2, 1);
     if (!parameters) {
         rc = ERROR_NOMEM;
         goto out;
     }
-    flexarray_append_pair(parameters, "uri", "fd:" MIGRATE_FD_NAME);
+    flexarray_append_pair(parameters, "filename", (char *)filename);
     args = libxl__xs_kvs_of_flexarray(gc, parameters, parameters->count);
     if (!args) {
         rc = ERROR_NOMEM;
         goto out2;
     }
 
-    rc = qmp_synchronous_send(qmp, "migrate", &args,
+    rc = qmp_synchronous_send(qmp, "xen-save-devices-state", &args,
                               NULL, NULL, qmp->timeout);
 
 out2:
-- 
1.7.2.5

Re: [PATCH v6 3/3] libxl_qmp: remove libxl__qmp_migrate, introduce libxl__qmp_save

[Ian Jackson]

Stefano Stabellini writes ("[Xen-devel] [PATCH v6 3/3] libxl_qmp: remove libxl__qmp_migrate, introduce libxl__qmp_save"):
> Following the recent changes to upstream Qemu, the best monitor command
> to suit or needs is "xen-save-devices-state" rather than "migrate".
> This patch removes libxl__qmp_migrate and introduces libxl__qmp_save
> instead, that uses "xen-save-devices-state".

This looks plausible to me.

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

Note that the meat of qmp_send_fd will need to be reintroduced in my
child process series, but I think it's fine for you to delete it here
and for me to later reintroduce it with a different name and
functionality and in a different file.

Ian.