[PATCH v2 1/2] libxl: use xen-blkback for 'vbd' disk types by default

[PATCH v2 1/2] libxl: use xen-blkback for 'vbd' disk types by default

[Marek Marczykowski-Górecki]

This will allow later to make HVM domain without qemu in dom0 (in
addition to the one in stubdomain).

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>

---
This is extracted from v1 of "libxl: do not start dom0 qemu for
stubdomain when not needed".

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
---
 tools/libxl/libxl_disk.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

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;
     } else {
         disk->backend = LIBXL_DISK_BACKEND_UNKNOWN;
     }
-- 
2.7.5


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

[PATCH v2 2/2] libxl: do not start dom0 qemu for stubdomain when not needed

[Marek Marczykowski-Górecki]

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@invisiblethingslab.com>
---
 tools/libxl/libxl_dm.c | 54 ++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 39 insertions(+), 15 deletions(-)

diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 44ebd70..e0e6a99 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)) {
+        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;
                 break;
             case STUBDOM_CONSOLE_SAVE:
                 console[i].output = GCSPRINTF("file:%s",
@@ -2019,6 +2030,14 @@ 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;
+        if (need_qemu)
+            console[i].consback = LIBXL__CONSOLE_BACKEND_IOEMU;
         ret = libxl__device_console_add(gc, dm_domid, &console[i],
                         i == STUBDOM_CONSOLE_LOGGING ? stubdom_state : NULL,
                         &device);
@@ -2032,7 +2051,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);
+    }
 
     return;
 
-- 
2.7.5


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

Re: [PATCH v2 2/2] libxl: do not start dom0 qemu for stubdomain when not needed

[Wei Liu]

On Fri, Jul 28, 2017 at 06:42:14PM +0200, Marek Marczykowski-Górecki wrote:
> 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@invisiblethingslab.com>

Acked-by: Wei Liu <wei.liu2@citrix.com>

> +                /* will be changed back to LIBXL__CONSOLE_BACKEND_IOEMU if qemu
> +                 * will be in use */

Hmm... I don't think there is requirement in CODING_STYLE for
multiple-line comment, so there are quite a few styles in use. But
looking at libxl code the prevailing style seems to be:

   /*
    *  xxxx
    *  xxxx
    */

I will make the adjustment while committing.

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

Re: [PATCH v2 2/2] libxl: do not start dom0 qemu for stubdomain when not needed

[Ian Jackson]

Wei Liu writes ("Re: [PATCH v2 2/2] libxl: do not start dom0 qemu for stubdomain when not needed"):
> Hmm... I don't think there is requirement in CODING_STYLE for
> multiple-line comment, so there are quite a few styles in use. But
> looking at libxl code the prevailing style seems to be:
> 
>    /*
>     *  xxxx
>     *  xxxx
>     */
> 
> I will make the adjustment while committing.

Personally I think this is well into the kind of territory that is not
worth arguing over.  Even a mix of styles here is fine.  (I think we
should mandate the initial `*' on continuation lines.)

Ian.

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

Re: [PATCH v2 1/2] libxl: use xen-blkback for 'vbd' disk types by default

[Wei Liu]

On Fri, Jul 28, 2017 at 06:42:13PM +0200, Marek Marczykowski-Górecki wrote:
> This will allow later to make HVM domain without qemu in dom0 (in
> addition to the one in stubdomain).
> 
> Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> 
> ---
> This is extracted from v1 of "libxl: do not start dom0 qemu for
> stubdomain when not needed".
> 
> Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> ---
>  tools/libxl/libxl_disk.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> 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;

Wait, it only occurred to me until now this patch is changing
disk_eject_xswatch_callback.

Is this a bug fix? How is it possible for the backend_type to be "vbd"
when there isn't such thing in libxl_types.idl?

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

Re: [PATCH v2 1/2] libxl: use xen-blkback for 'vbd' disk types by default

[Wei Liu]

On Mon, Jul 31, 2017 at 04:56:04PM +0100, Wei Liu wrote:
> On Fri, Jul 28, 2017 at 06:42:13PM +0200, Marek Marczykowski-Górecki wrote:
> > This will allow later to make HVM domain without qemu in dom0 (in
> > addition to the one in stubdomain).
> > 
> > Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> > 
> > ---
> > This is extracted from v1 of "libxl: do not start dom0 qemu for
> > stubdomain when not needed".
> > 
> > Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> > ---
> >  tools/libxl/libxl_disk.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > 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;
> 
> Wait, it only occurred to me until now this patch is changing
> disk_eject_xswatch_callback.
> 
> Is this a bug fix? How is it possible for the backend_type to be "vbd"
> when there isn't such thing in libxl_types.idl?

Oh, I'm an idiot. That's read from xenstore path. I think this patch is
correct. But I still tend to think this is a bug fix. How do you
discover this problem? Has disk eject ever worked for you?

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

Re: [PATCH v2 1/2] libxl: use xen-blkback for 'vbd' disk types by default

[Marek Marczykowski-Górecki]

[-- Attachment #1.1: Type: text/plain, Size: 2449 bytes --]

On Mon, Jul 31, 2017 at 05:01:08PM +0100, Wei Liu wrote:
> On Mon, Jul 31, 2017 at 04:56:04PM +0100, Wei Liu wrote:
> > On Fri, Jul 28, 2017 at 06:42:13PM +0200, Marek Marczykowski-Górecki wrote:
> > > This will allow later to make HVM domain without qemu in dom0 (in
> > > addition to the one in stubdomain).
> > > 
> > > Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> > > 
> > > ---
> > > This is extracted from v1 of "libxl: do not start dom0 qemu for
> > > stubdomain when not needed".
> > > 
> > > Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> > > ---
> > >  tools/libxl/libxl_disk.c | 4 +++-
> > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > 
> > > 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;
> > 
> > Wait, it only occurred to me until now this patch is changing
> > disk_eject_xswatch_callback.
> > 
> > Is this a bug fix? How is it possible for the backend_type to be "vbd"
> > when there isn't such thing in libxl_types.idl?
> 
> Oh, I'm an idiot. That's read from xenstore path. I think this patch is
> correct. But I still tend to think this is a bug fix. How do you
> discover this problem? Has disk eject ever worked for you?

Hmm, I think I've failed with forward-porting this patch. In current Xen
it isn't possible to create cdrom with backend_type!=qdisk...

So, this patch alone isn't sufficient. Please ignore it.

-- 
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 #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

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