diff mbox series

[v5,09/21] libxl: add save/restore support for qemu-xen in stubdomain

Message ID 20200428040433.23504-10-jandryuk@gmail.com (mailing list archive)
State Superseded
Headers show
Series Add support for qemu-xen runnning in a Linux-based stubdomain | expand

Commit Message

Jason Andryuk April 28, 2020, 4:04 a.m. UTC 
From: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>

Rely on a wrapper script in stubdomain to attach FD 3/4 of qemu to
relevant consoles.

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
Address TODO in dm_state_save_to_fdset: Only remove savefile for
non-stubdom.

Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
---
Changes in v3:
 - adjust for qmp_ev*
 - assume specific fdset id in qemu set in stubdomain
Changes in v5:
 - Only remove savefile for non-stubdom
---
 tools/libxl/libxl_dm.c  | 23 +++++++++++------------
 tools/libxl/libxl_qmp.c | 27 +++++++++++++++++++++++++--
 2 files changed, 36 insertions(+), 14 deletions(-)

Comments

Ian Jackson May 14, 2020, 4:35 p.m. UTC | #1 
Jason Andryuk writes ("[PATCH v5 09/21] libxl: add save/restore support for qemu-xen in stubdomain"):
> From: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> 
> Rely on a wrapper script in stubdomain to attach FD 3/4 of qemu to
> relevant consoles.
> 
> Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> Address TODO in dm_state_save_to_fdset: Only remove savefile for
> non-stubdom.
...
> +        if (is_stubdom) {
> +            /* Linux stubdomain connects specific FD to STUBDOM_CONSOLE_RESTORE
> +             */
> +            flexarray_append(dm_args, "-incoming");
> +            flexarray_append(dm_args, "fd:3");

Would it be possible to use a different fixed fd number ?  Low numbers
are particularly vulnerable to clashes with autoallocated numbers.

I suggest randomly allocating one in the range [64,192>.  My random
number generator picked 119.  So 118 and 119 ?

Also, why couldn't your wrapper script add this argument ?  If you do
that there then there is one place that knows the fd number and a
slightly less tortuous linkage between libxl and the script...

It's not stated anywhere here that I can see but I think what is
happening here is that your wrapper script knows the qemu savefile
pathname and reads it directly.  Maybbe a comment would be
worthwhile ?

The code looks like a good implementation of what it does, though.

Regards,
Ian.
Jason Andryuk May 17, 2020, 1:55 p.m. UTC | #2 
On Thu, May 14, 2020 at 12:35 PM Ian Jackson <ian.jackson@citrix.com> wrote:
>
> Jason Andryuk writes ("[PATCH v5 09/21] libxl: add save/restore support for qemu-xen in stubdomain"):
> > From: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> >
> > Rely on a wrapper script in stubdomain to attach FD 3/4 of qemu to
> > relevant consoles.
> >
> > Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> > Address TODO in dm_state_save_to_fdset: Only remove savefile for
> > non-stubdom.
> ...
> > +        if (is_stubdom) {
> > +            /* Linux stubdomain connects specific FD to STUBDOM_CONSOLE_RESTORE
> > +             */
> > +            flexarray_append(dm_args, "-incoming");
> > +            flexarray_append(dm_args, "fd:3");
>
> Would it be possible to use a different fixed fd number ?  Low numbers
> are particularly vulnerable to clashes with autoallocated numbers.
>
> I suggest randomly allocating one in the range [64,192>.  My random
> number generator picked 119.  So 118 and 119 ?

This makes sense and would be the easiest change.

> Also, why couldn't your wrapper script add this argument ?  If you do
> that there then there is one place that knows the fd number and a
> slightly less tortuous linkage between libxl and the script...

I like this idea, but there is a complication.  "-incoming" is only
added when performing a restore, so it cannot just be blindly added to
all qemu command lines in the stubdom.  Two options I see are to
either communicate a restore some other way (so the stubdom scripts
can add the appropriate option), or pass something though dm_args, but
let the script convert it into something usable.

There is "-incoming defer" where we can later specify
"migrate_incoming fd:119".  Another option is to `sed
s/defer/fd:119/`, but that is a little tricky since we need to look at
the preceding key to know if we should sed the second.  We could pass
only "-incoming" and require the stubdom script to modify that option.

I haven't tested any of this.

> It's not stated anywhere here that I can see but I think what is
> happening here is that your wrapper script knows the qemu savefile
> pathname and reads it directly.  Maybbe a comment would be
> worthwhile ?

The existing comment "Linux stubdomain connects specific FD to
STUBDOM_CONSOLE_RESTORE" is trying to state that.
STUBDOM_CONSOLE_RESTORE is defined as 2 for console 2 (/dev/hvc2), but
it is only a libxl_internal.h define.

Regards,
Jason
Ian Jackson May 18, 2020, 2:15 p.m. UTC | #3 
Jason Andryuk writes ("Re: [PATCH v5 09/21] libxl: add save/restore support for qemu-xen in stubdomain"):
> On Thu, May 14, 2020 at 12:35 PM Ian Jackson <ian.jackson@citrix.com> wrote:
> > I suggest randomly allocating one in the range [64,192>.  My random
> > number generator picked 119.  So 118 and 119 ?
> 
> This makes sense and would be the easiest change.

Cool.

> > Also, why couldn't your wrapper script add this argument ?  If you do
> > that there then there is one place that knows the fd number and a
> > slightly less tortuous linkage between libxl and the script...
> 
> I like this idea, but there is a complication.  "-incoming" is only
> added when performing a restore, so it cannot just be blindly added to
> all qemu command lines in the stubdom.  Two options I see are to
> either communicate a restore some other way (so the stubdom scripts
> can add the appropriate option), or pass something though dm_args, but
> let the script convert it into something usable.
> 
> There is "-incoming defer" where we can later specify
> "migrate_incoming fd:119".  Another option is to `sed
> s/defer/fd:119/`, but that is a little tricky since we need to look at
> the preceding key to know if we should sed the second.  We could pass
> only "-incoming" and require the stubdom script to modify that option.
> 
> I haven't tested any of this.

Erk.  I see now why you did it the way you did !

> > It's not stated anywhere here that I can see but I think what is
> > happening here is that your wrapper script knows the qemu savefile
> > pathname and reads it directly.  Maybbe a comment would be
> > worthwhile ?
> 
> The existing comment "Linux stubdomain connects specific FD to
> STUBDOM_CONSOLE_RESTORE" is trying to state that.
> STUBDOM_CONSOLE_RESTORE is defined as 2 for console 2 (/dev/hvc2), but
> it is only a libxl_internal.h define.

Err, by "the qemu savefile pathname" I meant the pathname in dom0.
I assume your wrapper script opens that and feeds it to the console.
Is that right ?

Thanks,
Ian.
Marek Marczykowski-Górecki May 18, 2020, 2:50 p.m. UTC | #4 
On Mon, May 18, 2020 at 03:15:17PM +0100, Ian Jackson wrote:
> Jason Andryuk writes ("Re: [PATCH v5 09/21] libxl: add save/restore support for qemu-xen in stubdomain"):
> > On Thu, May 14, 2020 at 12:35 PM Ian Jackson <ian.jackson@citrix.com> wrote:
> > > It's not stated anywhere here that I can see but I think what is
> > > happening here is that your wrapper script knows the qemu savefile
> > > pathname and reads it directly.  Maybbe a comment would be
> > > worthwhile ?
> > 
> > The existing comment "Linux stubdomain connects specific FD to
> > STUBDOM_CONSOLE_RESTORE" is trying to state that.
> > STUBDOM_CONSOLE_RESTORE is defined as 2 for console 2 (/dev/hvc2), but
> > it is only a libxl_internal.h define.
> 
> Err, by "the qemu savefile pathname" I meant the pathname in dom0.
> I assume your wrapper script opens that and feeds it to the console.
> Is that right ?

Not really a wrapper script. On dom0 side it is console backend (qemu)
instructed to connect the console to a file, instead of pty. I have
implemented similar feature in my xenconsoled patch series sent a while
ago (sent along with v2 of this series), but that series needs some more
love.

On the stubdomain side, it is a script that launches qemu - opens a
/dev/hvc2, then pass the FD to qemu via -incoming option (which is
really constructed by libxl).
Ian Jackson May 18, 2020, 3:18 p.m. UTC | #5 
> 
Marek Marczykowski-Górecki writes ("Re: [PATCH v5 09/21] libxl: add save/restore support for qemu-xen in stubdomain"):
> On Mon, May 18, 2020 at 03:15:17PM +0100, Ian Jackson wrote:
> > Err, by "the qemu savefile pathname" I meant the pathname in dom0.
> > I assume your wrapper script opens that and feeds it to the console.
> > Is that right ?
> 
> Not really a wrapper script. On dom0 side it is console backend (qemu)
> instructed to connect the console to a file, instead of pty. I have
> implemented similar feature in my xenconsoled patch series sent a while
> ago (sent along with v2 of this series), but that series needs some more
> love.
> 
> On the stubdomain side, it is a script that launches qemu - opens a
> /dev/hvc2, then pass the FD to qemu via -incoming option (which is
> really constructed by libxl).

Hi.  Thanks for trying to help me understand.  I was still confused
though.  I tried to explain another way and that helped me see what's
going on.

I think I understand now.

For reference, my confusion was this:

Jason Andryuk writes ("[PATCH v5 09/21] libxl: add save/restore support for qemu-xen in stubdomain"):
> index bdc23554eb..45d0dd56f5 100644
> --- a/tools/libxl/libxl_dm.c
> +++ b/tools/libxl/libxl_dm.c
> @@ -1744,10 +1744,17 @@ static int libxl__build_device_model_args_new(libxl__gc *gc,
>      }
>  
>      if (state->saved_state) {
> -        /* This file descriptor is meant to be used by QEMU */
> -        *dm_state_fd = open(state->saved_state, O_RDONLY);
> -        flexarray_append(dm_args, "-incoming");
> -        flexarray_append(dm_args, GCSPRINTF("fd:%d",*dm_state_fd));
> +        if (is_stubdom) {
> +            /* Linux stubdomain connects specific FD to STUBDOM_CONSOLE_RESTORE
> +             */
> +            flexarray_append(dm_args, "-incoming");
> +            flexarray_append(dm_args, "fd:3");
> +        } else {
> +            /* This file descriptor is meant to be used by QEMU */
> +            *dm_state_fd = open(state->saved_state, O_RDONLY);
> +            flexarray_append(dm_args, "-incoming");
> +            flexarray_append(dm_args, GCSPRINTF("fd:%d",*dm_state_fd));
> +        }

In this hunk, the call
           *dm_state_fd = open(state->saved_state, O_RDONLY);
becomes conditional.  It is no longer executed in the stubdomain
case.

So then, who opens state->saved_state ?  And how do they get told
where it is ?  If it's somewhere else in libxl, why doesn't it show up
in this patch ?

Posing the question liked that allowed me to see that the answer is

           console[i].output = GCSPRINTF("file:%s",
                           libxl__device_model_savefile(gc, guest_domid));

in spawn_stub_launch_dm.  And it doesn't appear in the patch because
it's already used for minios stub trad qemu and the same code is
now going to be executed for linux stub moderm qemu.

Ian.
Jason Andryuk May 18, 2020, 3:48 p.m. UTC | #6 
On Mon, May 18, 2020 at 11:18 AM Ian Jackson <ian.jackson@citrix.com> wrote:
>
> >
> Marek Marczykowski-Górecki writes ("Re: [PATCH v5 09/21] libxl: add save/restore support for qemu-xen in stubdomain"):
> > On Mon, May 18, 2020 at 03:15:17PM +0100, Ian Jackson wrote:
> > > Err, by "the qemu savefile pathname" I meant the pathname in dom0.
> > > I assume your wrapper script opens that and feeds it to the console.
> > > Is that right ?
> >
> > Not really a wrapper script. On dom0 side it is console backend (qemu)
> > instructed to connect the console to a file, instead of pty. I have
> > implemented similar feature in my xenconsoled patch series sent a while
> > ago (sent along with v2 of this series), but that series needs some more
> > love.
> >
> > On the stubdomain side, it is a script that launches qemu - opens a
> > /dev/hvc2, then pass the FD to qemu via -incoming option (which is
> > really constructed by libxl).
>
> Hi.  Thanks for trying to help me understand.  I was still confused
> though.  I tried to explain another way and that helped me see what's
> going on.
>
> I think I understand now.
>
> For reference, my confusion was this:
>
> Jason Andryuk writes ("[PATCH v5 09/21] libxl: add save/restore support for qemu-xen in stubdomain"):
> > index bdc23554eb..45d0dd56f5 100644
> > --- a/tools/libxl/libxl_dm.c
> > +++ b/tools/libxl/libxl_dm.c
> > @@ -1744,10 +1744,17 @@ static int libxl__build_device_model_args_new(libxl__gc *gc,
> >      }
> >
> >      if (state->saved_state) {
> > -        /* This file descriptor is meant to be used by QEMU */
> > -        *dm_state_fd = open(state->saved_state, O_RDONLY);
> > -        flexarray_append(dm_args, "-incoming");
> > -        flexarray_append(dm_args, GCSPRINTF("fd:%d",*dm_state_fd));
> > +        if (is_stubdom) {
> > +            /* Linux stubdomain connects specific FD to STUBDOM_CONSOLE_RESTORE
> > +             */
> > +            flexarray_append(dm_args, "-incoming");
> > +            flexarray_append(dm_args, "fd:3");
> > +        } else {
> > +            /* This file descriptor is meant to be used by QEMU */
> > +            *dm_state_fd = open(state->saved_state, O_RDONLY);
> > +            flexarray_append(dm_args, "-incoming");
> > +            flexarray_append(dm_args, GCSPRINTF("fd:%d",*dm_state_fd));
> > +        }
>
> In this hunk, the call
>            *dm_state_fd = open(state->saved_state, O_RDONLY);
> becomes conditional.  It is no longer executed in the stubdomain
> case.
>
> So then, who opens state->saved_state ?  And how do they get told
> where it is ?  If it's somewhere else in libxl, why doesn't it show up
> in this patch ?
>
> Posing the question liked that allowed me to see that the answer is
>
>            console[i].output = GCSPRINTF("file:%s",
>                            libxl__device_model_savefile(gc, guest_domid));
>
> in spawn_stub_launch_dm.  And it doesn't appear in the patch because
> it's already used for minios stub trad qemu and the same code is
> now going to be executed for linux stub moderm qemu.

Do you want the commit message to add a blurb about this?  So the
message becomes:
"""
Rely on a wrapper script in stubdomain to attach relevant consoles to
qemu.  The save console (1) must be attached to fdset/1.  When
performing a restore, $STUBDOM_RESTORE_INCOMING_ARG must be replaced on
the qemu command line by "fd:$FD", where $FD is an open file descriptor
number to the restore console (2).

Existing libxl code (for dom0) already connects the stubdom's save &
restore console outputs to the save & restore files.
"""

-Jason
Ian Jackson May 18, 2020, 4:37 p.m. UTC | #7 
Jason Andryuk writes ("Re: [PATCH v5 09/21] libxl: add save/restore support for qemu-xen in stubdomain [and 1 more messages]"):
> On Mon, May 18, 2020 at 11:18 AM Ian Jackson <ian.jackson@citrix.com> wrote:
> > [explanation of confusion]
> 
> Do you want the commit message to add a blurb about this?  So the
> message becomes:
> """
> Rely on a wrapper script in stubdomain to attach relevant consoles to
> qemu.  The save console (1) must be attached to fdset/1.  When
> performing a restore, $STUBDOM_RESTORE_INCOMING_ARG must be replaced on
> the qemu command line by "fd:$FD", where $FD is an open file descriptor
> number to the restore console (2).
> 
> Existing libxl code (for dom0) already connects the stubdom's save &
> restore console outputs to the save & restore files.
> """

I think that would be good, thanks, yes but I won't insist on it.

I think I already gave my ack for v6 of this.  If you add the commit
message text above you should obviously keep that ack.

Thanks,
Ian.
diff mbox series

Patch

diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index bdc23554eb..45d0dd56f5 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -1744,10 +1744,17 @@  static int libxl__build_device_model_args_new(libxl__gc *gc,
     }
 
     if (state->saved_state) {
-        /* This file descriptor is meant to be used by QEMU */
-        *dm_state_fd = open(state->saved_state, O_RDONLY);
-        flexarray_append(dm_args, "-incoming");
-        flexarray_append(dm_args, GCSPRINTF("fd:%d",*dm_state_fd));
+        if (is_stubdom) {
+            /* Linux stubdomain connects specific FD to STUBDOM_CONSOLE_RESTORE
+             */
+            flexarray_append(dm_args, "-incoming");
+            flexarray_append(dm_args, "fd:3");
+        } else {
+            /* This file descriptor is meant to be used by QEMU */
+            *dm_state_fd = open(state->saved_state, O_RDONLY);
+            flexarray_append(dm_args, "-incoming");
+            flexarray_append(dm_args, GCSPRINTF("fd:%d",*dm_state_fd));
+        }
     }
     for (i = 0; b_info->extra && b_info->extra[i] != NULL; i++)
         flexarray_append(dm_args, b_info->extra[i]);
@@ -2218,14 +2225,6 @@  void libxl__spawn_stub_dm(libxl__egc *egc, libxl__stub_dm_spawn_state *sdss)
 
     assert(libxl_defbool_val(guest_config->b_info.device_model_stubdomain));
 
-    if (libxl__stubdomain_is_linux(&guest_config->b_info)) {
-        if (d_state->saved_state) {
-            LOG(ERROR, "Save/Restore not supported yet with Linux Stubdom.");
-            ret = -1;
-            goto out;
-        }
-    }
-
     sdss->pvqemu.guest_domid = INVALID_DOMID;
 
     libxl_domain_create_info_init(&dm_config->c_info);
diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
index efaba91086..c394000ea9 100644
--- a/tools/libxl/libxl_qmp.c
+++ b/tools/libxl/libxl_qmp.c
@@ -962,6 +962,7 @@  static void dm_stopped(libxl__egc *egc, libxl__ev_qmp *ev,
                        const libxl__json_object *response, int rc);
 static void dm_state_fd_ready(libxl__egc *egc, libxl__ev_qmp *ev,
                               const libxl__json_object *response, int rc);
+static void dm_state_save_to_fdset(libxl__egc *egc, libxl__ev_qmp *ev, int fdset);
 static void dm_state_saved(libxl__egc *egc, libxl__ev_qmp *ev,
                            const libxl__json_object *response, int rc);
 
@@ -994,10 +995,17 @@  static void dm_stopped(libxl__egc *egc, libxl__ev_qmp *ev,
     EGC_GC;
     libxl__domain_suspend_state *dsps = CONTAINER_OF(ev, *dsps, qmp);
     const char *const filename = dsps->dm_savefile;
+    uint32_t dm_domid = libxl_get_stubdom_id(CTX, dsps->domid);
 
     if (rc)
         goto error;
 
+    if (dm_domid) {
+        /* see Linux stubdom interface in docs/stubdom.txt */
+        dm_state_save_to_fdset(egc, ev, 1);
+        return;
+    }
+
     ev->payload_fd = open(filename, O_WRONLY | O_CREAT, 0600);
     if (ev->payload_fd < 0) {
         LOGED(ERROR, ev->domid,
@@ -1028,7 +1036,6 @@  static void dm_state_fd_ready(libxl__egc *egc, libxl__ev_qmp *ev,
     EGC_GC;
     int fdset;
     const libxl__json_object *o;
-    libxl__json_object *args = NULL;
     libxl__domain_suspend_state *dsps = CONTAINER_OF(ev, *dsps, qmp);
 
     close(ev->payload_fd);
@@ -1043,6 +1050,21 @@  static void dm_state_fd_ready(libxl__egc *egc, libxl__ev_qmp *ev,
         goto error;
     }
     fdset = libxl__json_object_get_integer(o);
+    dm_state_save_to_fdset(egc, ev, fdset);
+    return;
+
+error:
+    assert(rc);
+    libxl__remove_file(gc, dsps->dm_savefile);
+    dsps->callback_device_model_done(egc, dsps, rc);
+}
+
+static void dm_state_save_to_fdset(libxl__egc *egc, libxl__ev_qmp *ev, int fdset)
+{
+    EGC_GC;
+    int rc;
+    libxl__json_object *args = NULL;
+    libxl__domain_suspend_state *dsps = CONTAINER_OF(ev, *dsps, qmp);
 
     ev->callback = dm_state_saved;
 
@@ -1060,7 +1082,8 @@  static void dm_state_fd_ready(libxl__egc *egc, libxl__ev_qmp *ev,
 
 error:
     assert(rc);
-    libxl__remove_file(gc, dsps->dm_savefile);
+    if (!libxl_get_stubdom_id(CTX, dsps->domid))
+        libxl__remove_file(gc, dsps->dm_savefile);
     dsps->callback_device_model_done(egc, dsps, rc);
 }