Re: [Xen-devel] [PATCH] libxl: do not start dom0 qemu for stubdomain when not needed

[Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>]

On Fri, Jul 28, 2017 at 04:17:51PM +0100, Wei Liu wrote:
> On Thu, Jul 27, 2017 at 12:44:25AM +0200, Marek Marczykowski-Górecki wrote:
> > Use xen-blkback for 'vbd' disk types by default and do not setup vfb+vkb
> > when no access method was configured. Then check if qemu is really
> > needed.
> > 
> > The only not configurable thing forcing qemu running in dom0 after this
> > change are consoles used to save/restore. But even in that case, there
> > is much smaller part of qemu exposed.
> > 
> > Signed-off-by: Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>
> > ---
> >  tools/libxl/libxl_disk.c |  4 +++-
> >  tools/libxl/libxl_dm.c   | 52 
> > ++++++++++++++++++++++++++++++++++--------------
> >  2 files changed, 40 insertions(+), 16 deletions(-)
> > 
> > diff --git a/tools/libxl/libxl_disk.c b/tools/libxl/libxl_disk.c
> > index 63de75c..7842d9b 100644
> > --- a/tools/libxl/libxl_disk.c
> > +++ b/tools/libxl/libxl_disk.c
> > @@ -56,10 +56,12 @@ static void disk_eject_xswatch_callback(libxl__egc 
> > *egc, libxl__ev_xswatch *w,
> >              "/local/domain/%d/backend/%" TOSTRING(BACKEND_STRING_SIZE)
> >             "[a-z]/%*d/%*d",
> >             &disk->backend_domid, backend_type);
> > -    if (!strcmp(backend_type, "tap") || !strcmp(backend_type, "vbd")) {
> > +    if (!strcmp(backend_type, "tap")) {
> >          disk->backend = LIBXL_DISK_BACKEND_TAP;
> >      } else if (!strcmp(backend_type, "qdisk")) {
> >          disk->backend = LIBXL_DISK_BACKEND_QDISK;
> > +    } else if (!strcmp(backend_type, "vbd")) {
> > +        disk->backend = LIBXL_DISK_BACKEND_PHY;
> 
> This should be split into a separate patch.
> 
> >      } else {
> >          disk->backend = LIBXL_DISK_BACKEND_UNKNOWN;
> >      }
> > diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> > index 44ebd70..c9aefe15 100644
> > --- a/tools/libxl/libxl_dm.c
> > +++ b/tools/libxl/libxl_dm.c
> > @@ -1868,13 +1868,17 @@ void libxl__spawn_stub_dm(libxl__egc *egc, 
> > libxl__stub_dm_spawn_state *sdss)
> >      ret = libxl__domain_build_info_setdefault(gc, &dm_config->b_info);
> >      if (ret) goto out;
> >  
> > -    GCNEW(vfb);
> > -    GCNEW(vkb);
> > -    libxl__vfb_and_vkb_from_hvm_guest_config(gc, guest_config, vfb, vkb);
> > -    dm_config->vfbs = vfb;
> > -    dm_config->num_vfbs = 1;
> > -    dm_config->vkbs = vkb;
> > -    dm_config->num_vkbs = 1;
> > +    if (libxl_defbool_val(guest_config->b_info.u.hvm.vnc.enable) ||
> > +            libxl_defbool_val(guest_config->b_info.u.hvm.spice.enable) ||
> > +            libxl_defbool_val(guest_config->b_info.u.hvm.sdl.enable)) {
> 
> Indentation.
> 
> > +        GCNEW(vfb);
> > +        GCNEW(vkb);
> > +        libxl__vfb_and_vkb_from_hvm_guest_config(gc, guest_config, vfb, 
> > vkb);
> > +        dm_config->vfbs = vfb;
> > +        dm_config->num_vfbs = 1;
> > +        dm_config->vkbs = vkb;
> > +        dm_config->num_vkbs = 1;
> > +    }
> >  
> >      stubdom_state->pv_kernel.path
> >          = libxl__abs_path(gc, "ioemu-stubdom.gz", 
> > libxl__xenfirmwaredir_path());
> > @@ -1959,6 +1963,7 @@ static void spawn_stub_launch_dm(libxl__egc *egc,
> >      libxl__domain_build_state *const d_state = sdss->dm.build_state;
> >      libxl__domain_build_state *const stubdom_state = &sdss->dm_state;
> >      uint32_t dm_domid = sdss->pvqemu.guest_domid;
> > +    int need_qemu;
> >  
> >      if (ret) {
> >          LOGD(ERROR, guest_domid, "error connecting disk devices");
> > @@ -1975,12 +1980,16 @@ static void spawn_stub_launch_dm(libxl__egc *egc,
> >          if (ret)
> >              goto out;
> >      }
> > -    ret = libxl__device_vfb_add(gc, dm_domid, &dm_config->vfbs[0]);
> > -    if (ret)
> > -        goto out;
> > -    ret = libxl__device_vkb_add(gc, dm_domid, &dm_config->vkbs[0]);
> > -    if (ret)
> > -        goto out;
> > +    if (dm_config->num_vfbs) {
> > +        ret = libxl__device_vfb_add(gc, dm_domid, &dm_config->vfbs[0]);
> > +        if (ret)
> > +            goto out;
> > +    }
> > +    if (dm_config->num_vkbs) {
> > +        ret = libxl__device_vkb_add(gc, dm_domid, &dm_config->vkbs[0]);
> > +        if (ret)
> > +            goto out;
> > +    }
> >  
> >      if (guest_config->b_info.u.hvm.serial)
> >          num_console++;
> > @@ -1988,7 +1997,6 @@ static void spawn_stub_launch_dm(libxl__egc *egc,
> >      console = libxl__calloc(gc, num_console, 
> > sizeof(libxl__device_console));
> >  
> >      for (i = 0; i < num_console; i++) {
> > -        libxl__device device;
> >          console[i].devid = i;
> >          console[i].consback = LIBXL__CONSOLE_BACKEND_IOEMU;
> >          /* STUBDOM_CONSOLE_LOGGING (console 0) is for minios logging
> > @@ -2005,6 +2013,9 @@ static void spawn_stub_launch_dm(libxl__egc *egc,
> >                  if (ret) goto out;
> >                  console[i].output = GCSPRINTF("file:%s", filename);
> >                  free(filename);
> > +                /* will be changed back to LIBXL__CONSOLE_BACKEND_IOEMU if 
> > qemu
> > +                 * will be in use */
> > +                console[i].consback = LIBXL__CONSOLE_BACKEND_XENCONSOLED;
> 
> Where is the code to change it back?

Oh, libxl__need_xenpv_qemu did that previously, but I it doesn't have this
part anymore. Will add it here.

> >                  break;
> >              case STUBDOM_CONSOLE_SAVE:
> >                  console[i].output = GCSPRINTF("file:%s",
> > @@ -2019,6 +2030,12 @@ static void spawn_stub_launch_dm(libxl__egc *egc,
> >                  console[i].output = "pty";
> >                  break;
> >          }
> > +    }
> > +
> > +    need_qemu = libxl__need_xenpv_qemu(gc, dm_config);
> > +
> > +    for (i = 0; i < num_console; i++) {
> > +        libxl__device device;
> >          ret = libxl__device_console_add(gc, dm_domid, &console[i],
> >                          i == STUBDOM_CONSOLE_LOGGING ? stubdom_state : 
> > NULL,
> >                          &device);
> > @@ -2032,7 +2049,12 @@ static void spawn_stub_launch_dm(libxl__egc *egc,
> >      sdss->pvqemu.build_state = &sdss->dm_state;
> >      sdss->pvqemu.callback = spawn_stubdom_pvqemu_cb;
> >  
> > -    libxl__spawn_local_dm(egc, &sdss->pvqemu);
> > +    if (!need_qemu) {
> > +        /* If dom0 qemu not needed, do not launch it */
> > +        spawn_stubdom_pvqemu_cb(egc, &sdss->pvqemu, 0);
> > +    } else {
> > +        libxl__spawn_local_dm(egc, &sdss->pvqemu);
> > +    }
> 
> Does this mean if the user doesn't configure certain devices he / she
> won't be able to migrate a guest served by a qemu-stubdom? That's not
> right IMO.

This patch itself does not break it (yet). Such guest will have all
consoles needed for migration by default. This also means that need_qemu
will always be true in practice. I imagine the next step would be either
adding some option to disable those consoles (maybe name it
"break_migration"? ;) ). Or adding support for multiple consoles to
xenconsoled.

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?

Attachment: signature.asc
Description: PGP signature

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel