From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marek =?utf-8?Q?Marczykowski-G=C3=B3recki?= Subject: Re: [PATCH] libxl: do not start dom0 qemu for stubdomain when not needed Date: Fri, 28 Jul 2017 17:34:42 +0200 Message-ID: <20170728153442.GP1095@mail-itl> References: <1501109065-8022-1-git-send-email-marmarek@invisiblethingslab.com> <20170728151750.5ksd6hpl7ij65phu@citrix.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============6993207915276139773==" Return-path: In-Reply-To: <20170728151750.5ksd6hpl7ij65phu@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xen.org Sender: "Xen-devel" To: Wei Liu Cc: Ian Jackson , xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org --===============6993207915276139773== Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="BqQJo5GnhE+DGCvB" Content-Disposition: inline --BqQJo5GnhE+DGCvB Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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=C3=B3recki= 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. > >=20 > > 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. > >=20 > > Signed-off-by: Marek Marczykowski-G=C3=B3recki > > --- > > tools/libxl/libxl_disk.c | 4 +++- > > tools/libxl/libxl_dm.c | 52 ++++++++++++++++++++++++++++++++++------= -------- > > 2 files changed, 40 insertions(+), 16 deletions(-) > >=20 > > 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 =3D LIBXL_DISK_BACKEND_TAP; > > } else if (!strcmp(backend_type, "qdisk")) { > > disk->backend =3D LIBXL_DISK_BACKEND_QDISK; > > + } else if (!strcmp(backend_type, "vbd")) { > > + disk->backend =3D LIBXL_DISK_BACKEND_PHY; >=20 > This should be split into a separate patch. >=20 > > } else { > > disk->backend =3D 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, libx= l__stub_dm_spawn_state *sdss) > > ret =3D libxl__domain_build_info_setdefault(gc, &dm_config->b_info= ); > > if (ret) goto out; > > =20 > > - GCNEW(vfb); > > - GCNEW(vkb); > > - libxl__vfb_and_vkb_from_hvm_guest_config(gc, guest_config, vfb, vk= b); > > - dm_config->vfbs =3D vfb; > > - dm_config->num_vfbs =3D 1; > > - dm_config->vkbs =3D vkb; > > - dm_config->num_vkbs =3D 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)) { >=20 > Indentation. >=20 > > + GCNEW(vfb); > > + GCNEW(vkb); > > + libxl__vfb_and_vkb_from_hvm_guest_config(gc, guest_config, vfb= , vkb); > > + dm_config->vfbs =3D vfb; > > + dm_config->num_vfbs =3D 1; > > + dm_config->vkbs =3D vkb; > > + dm_config->num_vkbs =3D 1; > > + } > > =20 > > stubdom_state->pv_kernel.path > > =3D libxl__abs_path(gc, "ioemu-stubdom.gz", libxl__xenfirmware= dir_path()); > > @@ -1959,6 +1963,7 @@ static void spawn_stub_launch_dm(libxl__egc *egc, > > libxl__domain_build_state *const d_state =3D sdss->dm.build_state; > > libxl__domain_build_state *const stubdom_state =3D &sdss->dm_state; > > uint32_t dm_domid =3D sdss->pvqemu.guest_domid; > > + int need_qemu; > > =20 > > if (ret) { > > LOGD(ERROR, guest_domid, "error connecting disk devices"); > > @@ -1975,12 +1980,16 @@ static void spawn_stub_launch_dm(libxl__egc *eg= c, > > if (ret) > > goto out; > > } > > - ret =3D libxl__device_vfb_add(gc, dm_domid, &dm_config->vfbs[0]); > > - if (ret) > > - goto out; > > - ret =3D libxl__device_vkb_add(gc, dm_domid, &dm_config->vkbs[0]); > > - if (ret) > > - goto out; > > + if (dm_config->num_vfbs) { > > + ret =3D libxl__device_vfb_add(gc, dm_domid, &dm_config->vfbs[0= ]); > > + if (ret) > > + goto out; > > + } > > + if (dm_config->num_vkbs) { > > + ret =3D libxl__device_vkb_add(gc, dm_domid, &dm_config->vkbs[0= ]); > > + if (ret) > > + goto out; > > + } > > =20 > > if (guest_config->b_info.u.hvm.serial) > > num_console++; > > @@ -1988,7 +1997,6 @@ static void spawn_stub_launch_dm(libxl__egc *egc, > > console =3D libxl__calloc(gc, num_console, sizeof(libxl__device_co= nsole)); > > =20 > > for (i =3D 0; i < num_console; i++) { > > - libxl__device device; > > console[i].devid =3D i; > > console[i].consback =3D 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 =3D GCSPRINTF("file:%s", filename); > > free(filename); > > + /* will be changed back to LIBXL__CONSOLE_BACKEND_IOEM= U if qemu > > + * will be in use */ > > + console[i].consback =3D LIBXL__CONSOLE_BACKEND_XENCONS= OLED; >=20 > 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 =3D GCSPRINTF("file:%s", > > @@ -2019,6 +2030,12 @@ static void spawn_stub_launch_dm(libxl__egc *egc, > > console[i].output =3D "pty"; > > break; > > } > > + } > > + > > + need_qemu =3D libxl__need_xenpv_qemu(gc, dm_config); > > + > > + for (i =3D 0; i < num_console; i++) { > > + libxl__device device; > > ret =3D libxl__device_console_add(gc, dm_domid, &console[i], > > i =3D=3D STUBDOM_CONSOLE_LOGGING ? stubdom_sta= te : NULL, > > &device); > > @@ -2032,7 +2049,12 @@ static void spawn_stub_launch_dm(libxl__egc *egc, > > sdss->pvqemu.build_state =3D &sdss->dm_state; > > sdss->pvqemu.callback =3D spawn_stubdom_pvqemu_cb; > > =20 > > - 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); > > + } >=20 > 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. --=20 Best Regards, Marek Marczykowski-G=C3=B3recki 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? --BqQJo5GnhE+DGCvB Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQEcBAEBCAAGBQJZe1mTAAoJENuP0xzK19csOv8H/1OIywRzeR/XabEbXFm1ptFd t4X+VNyPcbishGSlGwF6W3wHyQXBuJMwfFKBBJvpnJWksP292aIuMq4NSxihg8Mw YBv+dtP4iBKE9y4K3cQFpKkHAtpGviAPSdq7DiD4ZHWQ+y4ImDZTWFVTLYhe93p0 VC+fZQBU6/tgw3uMR9297GSCr39eIylawcpgfFfupt/xIfsGE1HaFP5bAFnfHkx/ bUtwspWeQblphbQHp/IQe5XwX0CNkoBuiPcQyzJq0A3tJyS7vQH8AIu6UleMBtUX eUuTmvKXchBd2KH6/X7DbDTXlmwHPDcnuVyr0XlNs26Tygnwzy/dUsexSH8Bt5g= =+KZ7 -----END PGP SIGNATURE----- --BqQJo5GnhE+DGCvB-- --===============6993207915276139773== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KWGVuLWRldmVs IG1haWxpbmcgbGlzdApYZW4tZGV2ZWxAbGlzdHMueGVuLm9yZwpodHRwczovL2xpc3RzLnhlbi5v cmcveGVuLWRldmVsCg== --===============6993207915276139773==--