Re: [Xen-devel] [PATCH 1 of 3] tools/libxl: QEMU device model suspend/resume

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Mon, 2012-01-23 at 22:46 +0000, rshriram@xxxxxxxxx wrote:
> # HG changeset patch
> # User Shriram Rajagopalan <rshriram@xxxxxxxxx>
> # Date 1327358638 28800
> # Node ID 11fb1dfda7de4d6759dec87d80cd16cf137f7369
> # Parent  80fdf2182bc62ca358ba2f1a3513b47a4f8d9dfd
> tools/libxl: QEMU device model suspend/resume
> 
>  * Refactor the libxl__domain_save_device_model.
>  * Add support for suspend/resume QEMU device model
>    (both traditional xen and QMP).
> 
> 
> Signed-off-by: Shriram Rajagopalan <rshriram@xxxxxxxxx>
> 
> diff -r 80fdf2182bc6 -r 11fb1dfda7de tools/libxl/libxl.c
> --- a/tools/libxl/libxl.c     Sat Jan 21 17:15:40 2012 +0000
> +++ b/tools/libxl/libxl.c     Mon Jan 23 14:43:58 2012 -0800
> @@ -477,7 +477,7 @@
>  
>      rc = libxl__domain_suspend_common(gc, domid, fd, type, live, debug);
>      if (!rc && type == LIBXL_DOMAIN_TYPE_HVM)
> -        rc = libxl__domain_save_device_model(gc, domid, fd);
> +        rc = libxl__domain_save_device_model(gc, domid, fd, /* No Remus */ 
> 0);
>      GC_FREE;
>      return rc;
>  }
> diff -r 80fdf2182bc6 -r 11fb1dfda7de tools/libxl/libxl_dom.c
> --- a/tools/libxl/libxl_dom.c Sat Jan 21 17:15:40 2012 +0000
> +++ b/tools/libxl/libxl_dom.c Mon Jan 23 14:43:58 2012 -0800
> @@ -534,6 +534,53 @@
>      return 0;
>  }
>  
> +static int libxl__remus_domain_suspend_qemu(libxl__gc *gc, uint32_t domid)
> +{
> +
> +    switch (libxl__device_model_version_running(gc, domid)) {
> +    case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL: {
> +        char *path = NULL;
> +        path = libxl__sprintf(gc, "/local/domain/0/device-model/%d/command",
> +                              domid);
> +        libxl__xs_write(gc, XBT_NULL, path, "save");
> +        libxl__wait_for_device_model(gc, domid, "paused", NULL, NULL, NULL);

This and libxl__remus_domain_resume_qemu, qemu_pci_add_xenstore,
qemu_pci_remove_xenstore, libxl__domain_save_device_model and
libxl_domain_unpause would probably benefit from a helper function to
send a command to a traditional qemu.

> +        break;
> +    }
> +    case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN:
> +        /* Stop QEMU */
> +        if (libxl__qmp_stop(gc, domid))
> +            return ERROR_FAIL;
> +        break;
> +    default:
> +        return ERROR_INVAL;
> +    }
> +
> +    return 0;
> +}
> +
> +static int libxl__remus_domain_resume_qemu(libxl__gc *gc, uint32_t domid)
> +{
> +
> +    switch (libxl__device_model_version_running(gc, domid)) {
> +    case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL: {
> +        char *path = NULL;
> +        path = libxl__sprintf(gc, "/local/domain/0/device-model/%d/command",
> +                              domid);
> +        libxl__xs_write(gc, XBT_NULL, path, "continue");
> +        break;
> +    }
> +    case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN:
> +        /* Stop QEMU */
> +        if (libxl__qmp_resume(gc, domid))
> +            return ERROR_FAIL;
> +        break;
> +    default:
> +        return ERROR_INVAL;
> +    }
> +
> +    return 0;
> +}
> +
>  int libxl__domain_suspend_common(libxl__gc *gc, uint32_t domid, int fd,
>                                   libxl_domain_type type,
>                                   int live, int debug)
> @@ -620,7 +667,7 @@
>      return rc;
>  }
>  
> -int libxl__domain_save_device_model(libxl__gc *gc, uint32_t domid, int fd)
> +int libxl__domain_save_device_model(libxl__gc *gc, uint32_t domid, int fd, 
> int remus)
>  {
>      libxl_ctx *ctx = libxl__gc_owner(gc);
>      int ret, fd2 = -1, c;
> @@ -631,13 +678,12 @@
>  
>      switch (libxl__device_model_version_running(gc, domid)) {
>      case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL: {
> -        char *path = NULL;
> -        LIBXL__LOG(ctx, LIBXL__LOG_DEBUG,
> -                   "Saving device model state to %s", filename);
> -        path = libxl__sprintf(gc, "/local/domain/0/device-model/%d/command",
> -                              domid);
> -        libxl__xs_write(gc, XBT_NULL, path, "save");
> -        libxl__wait_for_device_model(gc, domid, "paused", NULL, NULL, NULL);
> +        /* For Remus,we issue suspend_qemu call separately */

Why?

How does this interact with Stefano's XC_SAVEID_TOOLSTACK patches?

I think some sort of high level description of the control flow used by
Remus and how/why it differs from normal save/restore would be useful
for review.

> +        if (!remus) {
> +            c = libxl__remus_domain_suspend_qemu(gc, domid);

It seems odd to call this function libxl__remus_FOO and the use it
when !remus. The function doesn't look to be inherently specific to
either remus or !remus anyhow.

> +            if (c)
> +                return c;
> +        }
>          break;
>      }
>      case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN:
> @@ -668,8 +714,9 @@
>      qemu_state_len = st.st_size;
>      LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Qemu state is %d bytes\n", 
> qemu_state_len);
>  
> -    ret = libxl_write_exactly(ctx, fd, QEMU_SIGNATURE, 
> strlen(QEMU_SIGNATURE),
> -                              "saved-state file", "qemu signature");
> +    ret = libxl_write_exactly(ctx, fd, remus ? REMUS_QEMU_SIGNATURE : 
> QEMU_SIGNATURE,
> +                            remus ? strlen(REMUS_QEMU_SIGNATURE): 
> strlen(QEMU_SIGNATURE),
> +                            "saved-state file", "qemu signature");

QEMU_SIGNATURE is "DeviceModelRecord0002" which I believe libxc treats
identically to the Remus signature?

Again consideration of how this interacts with Stefano's patch would be
useful. If possible we'd quite like to pull the qemu-restore our of
xc_domain_restore for consistency with how xc_domain_save saves it, the
current layering is quite mad.

Ian.


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