Re: [PATCH v5 8/9] libxl: soft reset support

[Ian Campbell <Ian.Campbell@citrix.com>]

On Thu, 2014-12-11 at 14:45 +0100, Vitaly Kuznetsov wrote:
> Supported for HVM guests only.

Is it specifically PVHVM guests, or are unaware HVM guests also
supported? (I think the answer is that an unaware HVM guest has no way
to trigger a soft reset, so maybe it's moot...)

> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> index 0a123f1..710dc0e 100644
> --- a/tools/libxl/libxl.h
> +++ b/tools/libxl/libxl.h
> @@ -929,6 +929,12 @@ int static inline libxl_domain_create_restore_0x040200(
>  
>  #endif
>  
> +int libxl_domain_soft_reset(libxl_ctx *ctx, libxl_domain_config *d_config,
> +                            uint32_t *domid, uint32_t domid_old,
> +                            const libxl_asyncop_how *ao_how,
> +                            const libxl_asyncprogress_how *aop_console_how)
> +                            LIBXL_EXTERNAL_CALLERS_ONLY;
> +
>    /* A progress report will be made via ao_console_how, of type
>     * domain_create_console_available, when the domain's primary
>     * console is available and can be connected to.
> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> index 1198225..0a840c9 100644
> --- a/tools/libxl/libxl_create.c
> +++ b/tools/libxl/libxl_create.c
> @@ -25,6 +25,8 @@
>  #include <xen/hvm/hvm_info_table.h>
>  #include <xen/hvm/e820.h>
>  
> +#define INVALID_DOMID ~0

Is this completely internal to this file, or are you requiring that it
matches the one in xl_cmdimpl.c (i.e does it cross the library
interface)?

> +
> +void libxl__xc_domain_soft_reset(libxl__egc *egc,
> +                                 libxl__domain_create_state *dcs)
> +{
> +    STATE_AO_GC(dcs->ao);
> +    libxl_ctx *ctx = libxl__gc_owner(gc);
> +    const uint32_t domid_soft_reset = dcs->domid_soft_reset;
> +    const uint32_t domid = dcs->guest_domid;
> +    libxl_domain_config *const d_config = dcs->guest_config;
> +    libxl_domain_build_info *const info = &d_config->b_info;
> +    uint8_t *buf;
> +    uint32_t len;
> +    uint32_t console_domid, store_domid;
> +    unsigned long store_mfn, console_mfn;
> +    int rc;
> +    struct libxl__domain_suspend_state *dss;
> +
> +    GCNEW(dss);
> +
> +    dss->ao = ao;
> +    dss->domid = domid_soft_reset;
> +    dss->dm_savefile = GCSPRINTF("/var/lib/xen/qemu-save.%d",
> +                                 domid_soft_reset);
> +
> +    if (info->type == LIBXL_DOMAIN_TYPE_HVM) {

I thought the alternative  (PV) wasn't possible?

> +        rc = libxl__domain_suspend_device_model(gc, dss);
> +        if (rc) goto out;
> +    }
> +
> +    console_domid = dcs->build_state.console_domid;
> +    store_domid = dcs->build_state.store_domid;
[...]
> +    rc = xc_domain_soft_reset(ctx->xch, domid_soft_reset, domid, console_domid,
> +                              &console_mfn, store_domid, &store_mfn);
> +    if (rc) goto out;
[..]
> +    dcs->build_state.store_mfn = store_mfn;
> +    dcs->build_state.console_mfn = console_mfn;

Are you trying to avoid passing &dcs->build_state.store_mfn to the xc
function directly for some reason?

> +
> +    rc = libxl__toolstack_save(domid_soft_reset, &buf, &len, dss);
> +    if (rc) goto out;
> +
> +    rc = libxl__toolstack_restore(domid, buf, len, &dcs->shs);
> +    if (rc) goto out;
> +out:
> +    /*
> +     * Now pretend we did normal restore and simply call
> +     * libxl__xc_domain_restore_done().
> +     */
> +    libxl__xc_domain_restore_done(egc, dcs, rc, 0, 0);
> +}
> +
>  void libxl__srm_callout_callback_restore_results(unsigned long store_mfn,
>            unsigned long console_mfn, void *user)
>  {
> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index 4a0e2be..10ef652 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -121,6 +121,8 @@ libxl_action_on_shutdown = Enumeration("action_on_shutdown", [
>  
>      (5, "COREDUMP_DESTROY"),
>      (6, "COREDUMP_RESTART"),
> +
> +    (7, "SOFT_RESET"),

I think I mention a LIBXL_HAVE #define earlier on, since they are all
related I think you can have a single one for the overall feature rather
than ones for each new enum value. function etc. Probably
LIBXL_HAVE_DOMAIN_SOFT_RESET fits best?

> @@ -2519,7 +2538,17 @@ start:
>           * restore/migrate-receive it again.
>           */
>          restoring = 0;
> -    }else{
> +    } else if (domid_old != INVALID_DOMID) {
> +        /* Do soft reset */
> +        ret = libxl_domain_soft_reset(ctx, &d_config,
> +                                      &domid, domid_old,
> +                                      0, 0);
> +
> +        if ( ret ) {
> +            goto error_out;
> +        }
> +        domid_old = INVALID_DOMID;
> +    } else {
>          ret = libxl_domain_create_new(ctx, &d_config, &domid,
>                                        0, autoconnect_console_how);
>      }
> @@ -2583,6 +2612,8 @@ start:
>                  event->u.domain_shutdown.shutdown_reason,
>                  event->u.domain_shutdown.shutdown_reason);
>              switch (handle_domain_death(&domid, event, &d_config)) {
> +            case 3:
> +                domid_old = domid;

Please comment when falling through is deliberate.

I think we've now passed the point where the raw numbers are tolerable
any more, please could you convert to an enum and then add the new
value.

>              case 2:
>                  if (!preserve_domain(&domid, event, &d_config)) {
>                      /* If we fail then exit leaving the old domain in place. */

Ian.