Re: [Xen-devel] [PATCH v11 11/11] (lib)xl: soft reset support

2015-09-15 Thread Ian Campbell 
On Mon, 2015-09-14 at 17:15 +0100, Wei Liu wrote:
> FYI all other patches of this series were applied by Jan. You only need
> to resend this one.

Jan,

I appreciate you do not want to do so routinely for all commits you make
but when you are partially applying a series it would be very useful if you
could drop a line to the thread (i.e. the 00/NN mail) explaining what you
have applied vs not applied. Particularly when it is ten elevenths of the
series and so easy to miss that the eleventh patch has not been included.

I was under the incorrect impression that all 11 patches had gone in so had
removed this work from my queue.

Thanks,
Ian.

> 
> See below for a few comments.
> 
> On Fri, Sep 04, 2015 at 03:39:51PM +0200, Vitaly Kuznetsov wrote:
> > Use existing create/restore path to perform 'soft reset' for HVM
> > domains. Tear everything down, e.g. destroy domain's device model,
> > remove the domain from xenstore, save toolstack record and start
> > over.
> > 
> > Signed-off-by: Vitaly Kuznetsov 
> > ---
> > Changes since v10:
> > - Adapt to 'migration v2' changes.
> > - Use LIBXL_DEVICE_MODEL_SAVE_FILE as Qemu save file (and rename it to
> >   LIBXL_DEVICE_MODEL_RESTORE_FILE later) to support stubdom case (as
> >   we connect consoles to both files on create.
> > - Fix coding style, minor description change in xl.cfg.pod.5 [Wei Liu]
> > 
> > Signed-off-by: Vitaly Kuznetsov 
> > ---
> >  docs/man/xl.cfg.pod.5|   8 +-
> >  tools/libxl/libxl.c  |  22 -
> >  tools/libxl/libxl.h  |  15 
> >  tools/libxl/libxl_create.c   | 192
> > ++-
> >  tools/libxl/libxl_internal.h |   4 +
> >  tools/libxl/libxl_types.idl  |   2 +
> >  tools/libxl/xl.h |   1 +
> >  tools/libxl/xl_cmdimpl.c |  25 +-
> >  8 files changed, 242 insertions(+), 27 deletions(-)
> > 
> > diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
> > index c6345b8..d8c4186 100644
> > --- a/docs/man/xl.cfg.pod.5
> > +++ b/docs/man/xl.cfg.pod.5
> > @@ -349,6 +349,12 @@ destroy the domain.
> >  write a "coredump" of the domain to F and then
> >  restart the domain.
> >  
> > +=item B
> > +
> > +Reset all Xen specific interfaces for the Xen-aware HVM domain
> > allowing
> > +it to reestablish these interfaces and continue executing the domain.
> > PV
> > +guest is not supported.
> > +
> 
> And "non-Xen-aware HVM will crash" ?  If there is no definite answer to
> guest state maybe just saying "PV guest and non-Xen-aware HVM guests are
> not supported" ?
> 
> It's important to let user know about the consequence because libxl
> doesn't actually stop you from soft-resetting a HVM guest that is not
> Xen-aware.
> 
> [...]
> > +static int do_domain_soft_reset(libxl_ctx *ctx,
> > +libxl_domain_config *d_config,
> > +uint32_t domid_soft_reset,
> > +const libxl_asyncop_how *ao_how,
> > +const libxl_asyncprogress_how
> > +*aop_console_how)
> > +{
> > +AO_CREATE(ctx, 0, ao_how);
> > +libxl__domain_soft_reset_state *srs;
> > +libxl__app_domain_create_state *cdcs;
> > +libxl__domain_create_state *dcs;
> > +libxl__domain_build_state *state;
> > +libxl__domain_suspend_state *dss;
> > +char *dom_path, *xs_store_mfn, *xs_console_mfn;
> > +uint32_t domid_out;
> > +int rc;
> > +
> > +GCNEW(srs);
> > +cdcs = >cdcs;
> > +dcs = >dcs;
> > +state = >build_state;
> > +dss = >dss;
> > +
> > +srs->cdcs.dcs.ao = ao;
> > +srs->cdcs.dcs.guest_config = d_config;
> > +libxl_domain_config_init(>cdcs.dcs.guest_config_saved);
> > +libxl_domain_config_copy(ctx, >cdcs.dcs.guest_config_saved,
> > + d_config);
> > +cdcs->dcs.restore_fd = -1;
> > +cdcs->dcs.domid_soft_reset = domid_soft_reset;
> > +cdcs->dcs.callback = domain_create_cb;
> > +libxl__ao_progress_gethow(>cdcs.dcs.aop_console_how,
> > +  aop_console_how);
> > +cdcs->domid_out = _out;
> > +
> > +dom_path = libxl__xs_get_dompath(gc, domid_soft_reset);
> > +if (!dom_path) {
> > +LOG(ERROR, "failed to read domain path");
> > +return AO_CREATE_FAIL(ERROR_FAIL);
> 
> Sorry for not noticing this earlier. My bad and apologise.
> 
> This should be
> 
>if (!dom_path) {
>LOG(...);
>rc = ERROR_FAIL;
>  goto out;
>}
> 
> I.e. please use goto style error handling.
> 
> > +}
> > +
> > +xs_store_mfn = xs_read(ctx->xsh, XBT_NULL,
> > +   GCSPRINTF("%s/store/ring-ref", dom_path),
> > +   NULL);
> > +state->store_mfn = xs_store_mfn ? atol(xs_store_mfn): 0;
> > +free(xs_store_mfn);
> > +
> > +xs_console_mfn = xs_read(ctx->xsh, XBT_NULL,
> > +

Re: [Xen-devel] [PATCH v11 11/11] (lib)xl: soft reset support

2015-09-15 Thread Jan Beulich 
>>> On 15.09.15 at 10:38,  wrote:
> On Mon, 2015-09-14 at 17:15 +0100, Wei Liu wrote:
>> FYI all other patches of this series were applied by Jan. You only need
>> to resend this one.
> 
> I appreciate you do not want to do so routinely for all commits you make
> but when you are partially applying a series it would be very useful if you
> could drop a line to the thread (i.e. the 00/NN mail) explaining what you
> have applied vs not applied. Particularly when it is ten elevenths of the
> series and so easy to miss that the eleventh patch has not been included.
> 
> I was under the incorrect impression that all 11 patches had gone in so had
> removed this work from my queue.

I'm sorry; I'll try to remember to do better next time. In this case,
since I explicitly got feedback from Wei that he's still planning to
look at that patch, I assumed there would be another submission
(or an explicit subsequent ack) of that one patch anyway.

Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v11 11/11] (lib)xl: soft reset support

2015-09-14 Thread Wei Liu 
Sorry for the delay.

FYI all other patches of this series were applied by Jan. You only need
to resend this one.

See below for a few comments.

On Fri, Sep 04, 2015 at 03:39:51PM +0200, Vitaly Kuznetsov wrote:
> Use existing create/restore path to perform 'soft reset' for HVM
> domains. Tear everything down, e.g. destroy domain's device model,
> remove the domain from xenstore, save toolstack record and start
> over.
> 
> Signed-off-by: Vitaly Kuznetsov 
> ---
> Changes since v10:
> - Adapt to 'migration v2' changes.
> - Use LIBXL_DEVICE_MODEL_SAVE_FILE as Qemu save file (and rename it to
>   LIBXL_DEVICE_MODEL_RESTORE_FILE later) to support stubdom case (as
>   we connect consoles to both files on create.
> - Fix coding style, minor description change in xl.cfg.pod.5 [Wei Liu]
> 
> Signed-off-by: Vitaly Kuznetsov 
> ---
>  docs/man/xl.cfg.pod.5|   8 +-
>  tools/libxl/libxl.c  |  22 -
>  tools/libxl/libxl.h  |  15 
>  tools/libxl/libxl_create.c   | 192 
> ++-
>  tools/libxl/libxl_internal.h |   4 +
>  tools/libxl/libxl_types.idl  |   2 +
>  tools/libxl/xl.h |   1 +
>  tools/libxl/xl_cmdimpl.c |  25 +-
>  8 files changed, 242 insertions(+), 27 deletions(-)
> 
> diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
> index c6345b8..d8c4186 100644
> --- a/docs/man/xl.cfg.pod.5
> +++ b/docs/man/xl.cfg.pod.5
> @@ -349,6 +349,12 @@ destroy the domain.
>  write a "coredump" of the domain to F and then
>  restart the domain.
>  
> +=item B
> +
> +Reset all Xen specific interfaces for the Xen-aware HVM domain allowing
> +it to reestablish these interfaces and continue executing the domain. PV
> +guest is not supported.
> +

And "non-Xen-aware HVM will crash" ?  If there is no definite answer to
guest state maybe just saying "PV guest and non-Xen-aware HVM guests are
not supported" ?

It's important to let user know about the consequence because libxl
doesn't actually stop you from soft-resetting a HVM guest that is not
Xen-aware.

[...]
> +static int do_domain_soft_reset(libxl_ctx *ctx,
> +libxl_domain_config *d_config,
> +uint32_t domid_soft_reset,
> +const libxl_asyncop_how *ao_how,
> +const libxl_asyncprogress_how
> +*aop_console_how)
> +{
> +AO_CREATE(ctx, 0, ao_how);
> +libxl__domain_soft_reset_state *srs;
> +libxl__app_domain_create_state *cdcs;
> +libxl__domain_create_state *dcs;
> +libxl__domain_build_state *state;
> +libxl__domain_suspend_state *dss;
> +char *dom_path, *xs_store_mfn, *xs_console_mfn;
> +uint32_t domid_out;
> +int rc;
> +
> +GCNEW(srs);
> +cdcs = >cdcs;
> +dcs = >dcs;
> +state = >build_state;
> +dss = >dss;
> +
> +srs->cdcs.dcs.ao = ao;
> +srs->cdcs.dcs.guest_config = d_config;
> +libxl_domain_config_init(>cdcs.dcs.guest_config_saved);
> +libxl_domain_config_copy(ctx, >cdcs.dcs.guest_config_saved,
> + d_config);
> +cdcs->dcs.restore_fd = -1;
> +cdcs->dcs.domid_soft_reset = domid_soft_reset;
> +cdcs->dcs.callback = domain_create_cb;
> +libxl__ao_progress_gethow(>cdcs.dcs.aop_console_how,
> +  aop_console_how);
> +cdcs->domid_out = _out;
> +
> +dom_path = libxl__xs_get_dompath(gc, domid_soft_reset);
> +if (!dom_path) {
> +LOG(ERROR, "failed to read domain path");
> +return AO_CREATE_FAIL(ERROR_FAIL);

Sorry for not noticing this earlier. My bad and apologise.

This should be

   if (!dom_path) {
   LOG(...);
   rc = ERROR_FAIL;
   goto out;
   }

I.e. please use goto style error handling.

> +}
> +
> +xs_store_mfn = xs_read(ctx->xsh, XBT_NULL,
> +   GCSPRINTF("%s/store/ring-ref", dom_path),
> +   NULL);
> +state->store_mfn = xs_store_mfn ? atol(xs_store_mfn): 0;
> +free(xs_store_mfn);
> +
> +xs_console_mfn = xs_read(ctx->xsh, XBT_NULL,
> + GCSPRINTF("%s/console/ring-ref", dom_path),
> + NULL);
> +state->console_mfn = xs_console_mfn ? atol(xs_console_mfn): 0;
> +free(xs_console_mfn);
> +
> +dss->ao = ao;
> +dss->domid = domid_soft_reset;
> +dss->dm_savefile = GCSPRINTF(LIBXL_DEVICE_MODEL_SAVE_FILE".%d",
> + domid_soft_reset);
> +
> +rc = libxl__save_emulator_xenstore_data(dss, >toolstack_buf,
> +>toolstack_len);
> +if (rc) {
> +LOG(ERROR, "failed to save toolstack record.");
> +return AO_CREATE_FAIL(ERROR_FAIL);

Ditto, please use goto style error handling.

Furthermore please preserve rc from libxl__save_emulator_xenstore_data
instead of rewriting it with

Re: [Xen-devel] [PATCH v11 11/11] (lib)xl: soft reset support

2015-09-14 Thread Vitaly Kuznetsov 
Wei Liu  writes:

> Sorry for the delay.
>
> FYI all other patches of this series were applied by Jan. You only need
> to resend this one.

Cool, I will.

>
> See below for a few comments.
>
> On Fri, Sep 04, 2015 at 03:39:51PM +0200, Vitaly Kuznetsov wrote:
>> Use existing create/restore path to perform 'soft reset' for HVM
>> domains. Tear everything down, e.g. destroy domain's device model,
>> remove the domain from xenstore, save toolstack record and start
>> over.
>> 
>> Signed-off-by: Vitaly Kuznetsov 
>> ---
>> Changes since v10:
>> - Adapt to 'migration v2' changes.
>> - Use LIBXL_DEVICE_MODEL_SAVE_FILE as Qemu save file (and rename it to
>>   LIBXL_DEVICE_MODEL_RESTORE_FILE later) to support stubdom case (as
>>   we connect consoles to both files on create.
>> - Fix coding style, minor description change in xl.cfg.pod.5 [Wei Liu]
>> 
>> Signed-off-by: Vitaly Kuznetsov 
>> ---
>>  docs/man/xl.cfg.pod.5|   8 +-
>>  tools/libxl/libxl.c  |  22 -
>>  tools/libxl/libxl.h  |  15 
>>  tools/libxl/libxl_create.c   | 192 
>> ++-
>>  tools/libxl/libxl_internal.h |   4 +
>>  tools/libxl/libxl_types.idl  |   2 +
>>  tools/libxl/xl.h |   1 +
>>  tools/libxl/xl_cmdimpl.c |  25 +-
>>  8 files changed, 242 insertions(+), 27 deletions(-)
>> 
>> diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
>> index c6345b8..d8c4186 100644
>> --- a/docs/man/xl.cfg.pod.5
>> +++ b/docs/man/xl.cfg.pod.5
>> @@ -349,6 +349,12 @@ destroy the domain.
>>  write a "coredump" of the domain to F and then
>>  restart the domain.
>>  
>> +=item B
>> +
>> +Reset all Xen specific interfaces for the Xen-aware HVM domain allowing
>> +it to reestablish these interfaces and continue executing the domain. PV
>> +guest is not supported.
>> +
>
> And "non-Xen-aware HVM will crash" ?

Sorry, I should have replied to that suggestion earlier. Non-Xen-aware
HVM guest can't really trigger this action and (in theory) is capable of
doing kexec without any assistance.

> If there is no definite answer to
> guest state maybe just saying "PV guest and non-Xen-aware HVM guests are
> not supported" ?

This sounds correct.

>
> It's important to let user know about the consequence because libxl
> doesn't actually stop you from soft-resetting a HVM guest that is not
> Xen-aware.

The question is who (and when/how) is going to trigger this action? In
case someone does that while HVM domain (doesn't really matter if it is
Xen-aware or not) does not expect this action it will crash.

*In theory* nothing bad is going to happen to a non-Xen-aware HVM guest
if someone else will trigger this action for it (e.g. on 'reset'
signal), it will just get an assistance it doesn't need.

[...]

-- 
  Vitaly

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v11 11/11] (lib)xl: soft reset support

2015-09-14 Thread Wei Liu 
On Mon, Sep 14, 2015 at 06:54:51PM +0200, Vitaly Kuznetsov wrote:
> Wei Liu  writes:
> 
> > Sorry for the delay.
> >
> > FYI all other patches of this series were applied by Jan. You only need
> > to resend this one.
> 
> Cool, I will.
> 
> >
> > See below for a few comments.
> >
> > On Fri, Sep 04, 2015 at 03:39:51PM +0200, Vitaly Kuznetsov wrote:
> >> Use existing create/restore path to perform 'soft reset' for HVM
> >> domains. Tear everything down, e.g. destroy domain's device model,
> >> remove the domain from xenstore, save toolstack record and start
> >> over.
> >> 
> >> Signed-off-by: Vitaly Kuznetsov 
> >> ---
> >> Changes since v10:
> >> - Adapt to 'migration v2' changes.
> >> - Use LIBXL_DEVICE_MODEL_SAVE_FILE as Qemu save file (and rename it to
> >>   LIBXL_DEVICE_MODEL_RESTORE_FILE later) to support stubdom case (as
> >>   we connect consoles to both files on create.
> >> - Fix coding style, minor description change in xl.cfg.pod.5 [Wei Liu]
> >> 
> >> Signed-off-by: Vitaly Kuznetsov 
> >> ---
> >>  docs/man/xl.cfg.pod.5|   8 +-
> >>  tools/libxl/libxl.c  |  22 -
> >>  tools/libxl/libxl.h  |  15 
> >>  tools/libxl/libxl_create.c   | 192 
> >> ++-
> >>  tools/libxl/libxl_internal.h |   4 +
> >>  tools/libxl/libxl_types.idl  |   2 +
> >>  tools/libxl/xl.h |   1 +
> >>  tools/libxl/xl_cmdimpl.c |  25 +-
> >>  8 files changed, 242 insertions(+), 27 deletions(-)
> >> 
> >> diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
> >> index c6345b8..d8c4186 100644
> >> --- a/docs/man/xl.cfg.pod.5
> >> +++ b/docs/man/xl.cfg.pod.5
> >> @@ -349,6 +349,12 @@ destroy the domain.
> >>  write a "coredump" of the domain to F and then
> >>  restart the domain.
> >>  
> >> +=item B
> >> +
> >> +Reset all Xen specific interfaces for the Xen-aware HVM domain allowing
> >> +it to reestablish these interfaces and continue executing the domain. PV
> >> +guest is not supported.
> >> +
> >
> > And "non-Xen-aware HVM will crash" ?
> 
> Sorry, I should have replied to that suggestion earlier. Non-Xen-aware
> HVM guest can't really trigger this action and (in theory) is capable of
> doing kexec without any assistance.
> 
> > If there is no definite answer to
> > guest state maybe just saying "PV guest and non-Xen-aware HVM guests are
> > not supported" ?
> 
> This sounds correct.
> 
> >
> > It's important to let user know about the consequence because libxl
> > doesn't actually stop you from soft-resetting a HVM guest that is not
> > Xen-aware.
> 
> The question is who (and when/how) is going to trigger this action? In
> case someone does that while HVM domain (doesn't really matter if it is
> Xen-aware or not) does not expect this action it will crash.
> 
> *In theory* nothing bad is going to happen to a non-Xen-aware HVM guest
> if someone else will trigger this action for it (e.g. on 'reset'
> signal), it will just get an assistance it doesn't need.
> 

OK then. We should still make it clear it is unsupported for
non-Xen-aware HVM guest even if it happens to work.

Wei.

> [...]
> 
> -- 
>   Vitaly

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [PATCH v11 11/11] (lib)xl: soft reset support

2015-09-04 Thread Vitaly Kuznetsov 
Use existing create/restore path to perform 'soft reset' for HVM
domains. Tear everything down, e.g. destroy domain's device model,
remove the domain from xenstore, save toolstack record and start
over.

Signed-off-by: Vitaly Kuznetsov 
---
Changes since v10:
- Adapt to 'migration v2' changes.
- Use LIBXL_DEVICE_MODEL_SAVE_FILE as Qemu save file (and rename it to
  LIBXL_DEVICE_MODEL_RESTORE_FILE later) to support stubdom case (as
  we connect consoles to both files on create.
- Fix coding style, minor description change in xl.cfg.pod.5 [Wei Liu]

Signed-off-by: Vitaly Kuznetsov 
---
 docs/man/xl.cfg.pod.5|   8 +-
 tools/libxl/libxl.c  |  22 -
 tools/libxl/libxl.h  |  15 
 tools/libxl/libxl_create.c   | 192 ++-
 tools/libxl/libxl_internal.h |   4 +
 tools/libxl/libxl_types.idl  |   2 +
 tools/libxl/xl.h |   1 +
 tools/libxl/xl_cmdimpl.c |  25 +-
 8 files changed, 242 insertions(+), 27 deletions(-)

diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
index c6345b8..d8c4186 100644
--- a/docs/man/xl.cfg.pod.5
+++ b/docs/man/xl.cfg.pod.5
@@ -349,6 +349,12 @@ destroy the domain.
 write a "coredump" of the domain to F and then
 restart the domain.
 
+=item B
+
+Reset all Xen specific interfaces for the Xen-aware HVM domain allowing
+it to reestablish these interfaces and continue executing the domain. PV
+guest is not supported.
+
 =back
 
 The default for C is C.
@@ -370,7 +376,7 @@ Action to take if the domain crashes.  Default is 
C.
 =item