[PATCH v5 0/6] libxl: xs_restrict QEMU

[PATCH v5 0/6] libxl: xs_restrict QEMU

[Stefano Stabellini]

Hi all,

this patch series changes libxl to start QEMU as device model with the
new xsrestrict option (http://marc.info/?l=xen-devel&m=143341692707358).
It also starts a second QEMU to provide PV backends in userspace (qdisk)
to HVM guests.


Changes in v5:
- improve commit messages with security details

Changes in v4:
- update xenstore-paths.markdown
- add error message in case count > MAX_PHYSMAP_ENTRIES
- add a note to xenstore-paths.markdown about the possible change in
privilege level
- only change permissions if xsrestrict is supported

Changes in v3:
- use LIBXL_TOOLSTACK_DOMID instead of 0 in the commit message
- update commit message with more info on why it is safe
- add a limit on the number of physmap entries to save and restore
- add emulator_ids
- mark patch #3 as WIP
- use LIBXL_TOOLSTACK_DOMID instead of 0 in the commit message
- change xs path to include the emulator_id
- change qdisk-backend-pid path on xenstore
- use dcs->dmss.pvqemu to spawn the second QEMU
- keep track of the rc of both QEMUs before proceeding


Stefano Stabellini (6):
      libxl: do not add a vkb backend to hvm guests
      [WIP] libxl: xsrestrict QEMU
      libxl: allow /local/domain/$LIBXL_TOOLSTACK_DOMID/device-model/$DOMID to be written by $DOMID
      libxl: change xs path for QEMU
      libxl: change qdisk-backend-pid path on xenstore
      libxl: spawns two QEMUs for HVM guests

 docs/misc/xenstore-paths.markdown |   30 ++++++++--
 tools/libxl/libxl.c               |    2 +-
 tools/libxl/libxl_create.c        |   58 +++++++++++++------
 tools/libxl/libxl_device.c        |    2 +-
 tools/libxl/libxl_dm.c            |  115 +++++++++++++++++++++++++++++++++----
 tools/libxl/libxl_dom.c           |   19 ++++--
 tools/libxl/libxl_internal.c      |   19 ++++--
 tools/libxl/libxl_internal.h      |   15 ++++-
 tools/libxl/libxl_pci.c           |   14 ++---
 tools/libxl/libxl_utils.c         |   10 ++++
 10 files changed, 225 insertions(+), 59 deletions(-)

Cheers,

Stefano

[PATCH v5 1/6] libxl: do not add a vkb backend to hvm guests

[Stefano Stabellini]

When QEMU restricts its xenstore connection, it cannot provide PV
backends. A separate QEMU instance is required to provide PV backends in
userspace, such as qdisk. With two separate instances, it is not
possible to take advantage of vkb for mouse and keyboard, as the QEMU
that emulates the graphic card (the device model), would be separate
from the QEMU running the vkb backend (PV QEMU).

Removing this functionality is acceptable, because is only useful for
power saving when usb emulation is off, letting QEMU sleep for longer
periods of time.  However usb emulation is on by default, and how to
take advantage of this configuration has never been documented.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
 tools/libxl/libxl_create.c |    5 -----
 1 file changed, 5 deletions(-)

diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index f0da7dc..a74b340 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -1275,17 +1275,12 @@ static void domcreate_launch_dm(libxl__egc *egc, libxl__multidev *multidev,
     {
         libxl__device_console console;
         libxl__device device;
-        libxl_device_vkb vkb;
 
         init_console_info(gc, &console, 0);
         console.backend_domid = state->console_domid;
         libxl__device_console_add(gc, domid, &console, state, &device);
         libxl__device_console_dispose(&console);
 
-        libxl_device_vkb_init(&vkb);
-        libxl__device_vkb_add(gc, domid, &vkb);
-        libxl_device_vkb_dispose(&vkb);
-
         dcs->dmss.dm.guest_domid = domid;
         if (libxl_defbool_val(d_config->b_info.device_model_stubdomain))
             libxl__spawn_stub_dm(egc, &dcs->dmss);
-- 
1.7.10.4

Re: [PATCH v5 1/6] libxl: do not add a vkb backend to hvm guests

[Paul Durrant]

> -----Original Message-----
> From: xen-devel-bounces@lists.xen.org [mailto:xen-devel-
> bounces@lists.xen.org] On Behalf Of Stefano Stabellini
> Sent: 23 July 2015 18:28
> To: xen-devel@lists.xensource.com
> Cc: Wei Liu; Ian Jackson; Ian Campbell; Stefano Stabellini
> Subject: [Xen-devel] [PATCH v5 1/6] libxl: do not add a vkb backend to hvm
> guests
> 
> When QEMU restricts its xenstore connection, it cannot provide PV
> backends. A separate QEMU instance is required to provide PV backends in
> userspace, such as qdisk. With two separate instances, it is not
> possible to take advantage of vkb for mouse and keyboard, as the QEMU
> that emulates the graphic card (the device model), would be separate
> from the QEMU running the vkb backend (PV QEMU).
> 
> Removing this functionality is acceptable, because is only useful for
> power saving when usb emulation is off, letting QEMU sleep for longer
> periods of time.  However usb emulation is on by default, and how to
> take advantage of this configuration has never been documented.
> 

I don't think I agree. Turning off USB emulation for HVM guests (particularly Windows) has been shown to be highly advantageous in performance and scalability terms, and we have a prototype HID driver (not yet part of the XenProject driver set, but hopefully soon will be) which uses vkb.

  Paul

> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>  tools/libxl/libxl_create.c |    5 -----
>  1 file changed, 5 deletions(-)
> 
> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> index f0da7dc..a74b340 100644
> --- a/tools/libxl/libxl_create.c
> +++ b/tools/libxl/libxl_create.c
> @@ -1275,17 +1275,12 @@ static void domcreate_launch_dm(libxl__egc
> *egc, libxl__multidev *multidev,
>      {
>          libxl__device_console console;
>          libxl__device device;
> -        libxl_device_vkb vkb;
> 
>          init_console_info(gc, &console, 0);
>          console.backend_domid = state->console_domid;
>          libxl__device_console_add(gc, domid, &console, state, &device);
>          libxl__device_console_dispose(&console);
> 
> -        libxl_device_vkb_init(&vkb);
> -        libxl__device_vkb_add(gc, domid, &vkb);
> -        libxl_device_vkb_dispose(&vkb);
> -
>          dcs->dmss.dm.guest_domid = domid;
>          if (libxl_defbool_val(d_config->b_info.device_model_stubdomain))
>              libxl__spawn_stub_dm(egc, &dcs->dmss);
> --
> 1.7.10.4
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH v5 1/6] libxl: do not add a vkb backend to hvm guests

[Stefano Stabellini]

On Fri, 24 Jul 2015, Paul Durrant wrote:
> > -----Original Message-----
> > From: xen-devel-bounces@lists.xen.org [mailto:xen-devel-
> > bounces@lists.xen.org] On Behalf Of Stefano Stabellini
> > Sent: 23 July 2015 18:28
> > To: xen-devel@lists.xensource.com
> > Cc: Wei Liu; Ian Jackson; Ian Campbell; Stefano Stabellini
> > Subject: [Xen-devel] [PATCH v5 1/6] libxl: do not add a vkb backend to hvm
> > guests
> > 
> > When QEMU restricts its xenstore connection, it cannot provide PV
> > backends. A separate QEMU instance is required to provide PV backends in
> > userspace, such as qdisk. With two separate instances, it is not
> > possible to take advantage of vkb for mouse and keyboard, as the QEMU
> > that emulates the graphic card (the device model), would be separate
> > from the QEMU running the vkb backend (PV QEMU).
> > 
> > Removing this functionality is acceptable, because is only useful for
> > power saving when usb emulation is off, letting QEMU sleep for longer
> > periods of time.  However usb emulation is on by default, and how to
> > take advantage of this configuration has never been documented.
> > 
> 
> I don't think I agree. Turning off USB emulation for HVM guests (particularly Windows) has been shown to be highly advantageous in performance and scalability terms, and we have a prototype HID driver (not yet part of the XenProject driver set, but hopefully soon will be) which uses vkb.

I would appreciate if this kind of comments were made at v1 or v2, not
v5 of a series :-)


I know that turning USB emulation off is a big win, but nobody is really
doing it. The reason is that we didn't properly documented how to do it.
As you say, not even the Xen Project Windows PV drivers take advantage
of vkb yet, even though they might soon. I still think that removing vkb 
cannot be considered a regression.

If it comes to a choice, I think that securing QEMU is more important
that turning USB emulation off and the two are fundamentally
incompatible.

Even if we run two QEMUs, one for emulation, one for the backends, the
vkb backend would need to be running in the same QEMU that offers vga
emulation because of the cursor rendering. It is a no go.

We could expose vkb but only together with vfb, so the vkb cursor would
only affect the vfb screen and not the vga screen. I don't know if you
would like that solution, but in any case it could still be introduced
after this work as it is independent.

Another possibility would be to add an option to request a vkb backend
for HVM guests in the config file. If the option is selected QEMU would
run as root. I wouldn't dare to recommend it to anybody.


> > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > Acked-by: Ian Campbell <ian.campbell@citrix.com>
> > ---
> >  tools/libxl/libxl_create.c |    5 -----
> >  1 file changed, 5 deletions(-)
> > 
> > diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> > index f0da7dc..a74b340 100644
> > --- a/tools/libxl/libxl_create.c
> > +++ b/tools/libxl/libxl_create.c
> > @@ -1275,17 +1275,12 @@ static void domcreate_launch_dm(libxl__egc
> > *egc, libxl__multidev *multidev,
> >      {
> >          libxl__device_console console;
> >          libxl__device device;
> > -        libxl_device_vkb vkb;
> > 
> >          init_console_info(gc, &console, 0);
> >          console.backend_domid = state->console_domid;
> >          libxl__device_console_add(gc, domid, &console, state, &device);
> >          libxl__device_console_dispose(&console);
> > 
> > -        libxl_device_vkb_init(&vkb);
> > -        libxl__device_vkb_add(gc, domid, &vkb);
> > -        libxl_device_vkb_dispose(&vkb);
> > -
> >          dcs->dmss.dm.guest_domid = domid;
> >          if (libxl_defbool_val(d_config->b_info.device_model_stubdomain))
> >              libxl__spawn_stub_dm(egc, &dcs->dmss);
> > --
> > 1.7.10.4
> > 
> > 
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xen.org
> > http://lists.xen.org/xen-devel
> 

Re: [PATCH v5 1/6] libxl: do not add a vkb backend to hvm guests

[Paul Durrant]

> -----Original Message-----
> From: Stefano Stabellini [mailto:stefano.stabellini@eu.citrix.com]
> Sent: 24 July 2015 11:21
> To: Paul Durrant
> Cc: Stefano Stabellini; xen-devel@lists.xensource.com; Wei Liu; Ian Jackson;
> Ian Campbell
> Subject: RE: [Xen-devel] [PATCH v5 1/6] libxl: do not add a vkb backend to
> hvm guests
> 
> On Fri, 24 Jul 2015, Paul Durrant wrote:
> > > -----Original Message-----
> > > From: xen-devel-bounces@lists.xen.org [mailto:xen-devel-
> > > bounces@lists.xen.org] On Behalf Of Stefano Stabellini
> > > Sent: 23 July 2015 18:28
> > > To: xen-devel@lists.xensource.com
> > > Cc: Wei Liu; Ian Jackson; Ian Campbell; Stefano Stabellini
> > > Subject: [Xen-devel] [PATCH v5 1/6] libxl: do not add a vkb backend to
> hvm
> > > guests
> > >
> > > When QEMU restricts its xenstore connection, it cannot provide PV
> > > backends. A separate QEMU instance is required to provide PV backends
> in
> > > userspace, such as qdisk. With two separate instances, it is not
> > > possible to take advantage of vkb for mouse and keyboard, as the QEMU
> > > that emulates the graphic card (the device model), would be separate
> > > from the QEMU running the vkb backend (PV QEMU).
> > >
> > > Removing this functionality is acceptable, because is only useful for
> > > power saving when usb emulation is off, letting QEMU sleep for longer
> > > periods of time.  However usb emulation is on by default, and how to
> > > take advantage of this configuration has never been documented.
> > >
> >
> > I don't think I agree. Turning off USB emulation for HVM guests (particularly
> Windows) has been shown to be highly advantageous in performance and
> scalability terms, and we have a prototype HID driver (not yet part of the
> XenProject driver set, but hopefully soon will be) which uses vkb.
> 
> I would appreciate if this kind of comments were made at v1 or v2, not
> v5 of a series :-)
> 

Yes, I realise that, but I've been busy... sorry.

> 
> I know that turning USB emulation off is a big win, but nobody is really
> doing it. The reason is that we didn't properly documented how to do it.

It's documented for XenServer and we have toolstack support to do it.

> As you say, not even the Xen Project Windows PV drivers take advantage
> of vkb yet, even though they might soon. I still think that removing vkb
> cannot be considered a regression.
> 
> If it comes to a choice, I think that securing QEMU is more important
> that turning USB emulation off and the two are fundamentally
> incompatible.
> 
> Even if we run two QEMUs, one for emulation, one for the backends, the
> vkb backend would need to be running in the same QEMU that offers vga
> emulation because of the cursor rendering. It is a no go.

I realise it would be a bit odd typing into one window and seeing output in another, but is that a reason to disallow it?

  Paul

> 
> We could expose vkb but only together with vfb, so the vkb cursor would
> only affect the vfb screen and not the vga screen. I don't know if you
> would like that solution, but in any case it could still be introduced
> after this work as it is independent.
> 
> Another possibility would be to add an option to request a vkb backend
> for HVM guests in the config file. If the option is selected QEMU would
> run as root. I wouldn't dare to recommend it to anybody.
> 
> 
> > > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > > Acked-by: Ian Campbell <ian.campbell@citrix.com>
> > > ---
> > >  tools/libxl/libxl_create.c |    5 -----
> > >  1 file changed, 5 deletions(-)
> > >
> > > diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> > > index f0da7dc..a74b340 100644
> > > --- a/tools/libxl/libxl_create.c
> > > +++ b/tools/libxl/libxl_create.c
> > > @@ -1275,17 +1275,12 @@ static void domcreate_launch_dm(libxl__egc
> > > *egc, libxl__multidev *multidev,
> > >      {
> > >          libxl__device_console console;
> > >          libxl__device device;
> > > -        libxl_device_vkb vkb;
> > >
> > >          init_console_info(gc, &console, 0);
> > >          console.backend_domid = state->console_domid;
> > >          libxl__device_console_add(gc, domid, &console, state, &device);
> > >          libxl__device_console_dispose(&console);
> > >
> > > -        libxl_device_vkb_init(&vkb);
> > > -        libxl__device_vkb_add(gc, domid, &vkb);
> > > -        libxl_device_vkb_dispose(&vkb);
> > > -
> > >          dcs->dmss.dm.guest_domid = domid;
> > >          if (libxl_defbool_val(d_config->b_info.device_model_stubdomain))
> > >              libxl__spawn_stub_dm(egc, &dcs->dmss);
> > > --
> > > 1.7.10.4
> > >
> > >
> > > _______________________________________________
> > > Xen-devel mailing list
> > > Xen-devel@lists.xen.org
> > > http://lists.xen.org/xen-devel
> >

Re: [PATCH v5 1/6] libxl: do not add a vkb backend to hvm guests

[Stefano Stabellini]

On Fri, 24 Jul 2015, Paul Durrant wrote:
> > -----Original Message-----
> > From: Stefano Stabellini [mailto:stefano.stabellini@eu.citrix.com]
> > Sent: 24 July 2015 11:21
> > To: Paul Durrant
> > Cc: Stefano Stabellini; xen-devel@lists.xensource.com; Wei Liu; Ian Jackson;
> > Ian Campbell
> > Subject: RE: [Xen-devel] [PATCH v5 1/6] libxl: do not add a vkb backend to
> > hvm guests
> > 
> > On Fri, 24 Jul 2015, Paul Durrant wrote:
> > > > -----Original Message-----
> > > > From: xen-devel-bounces@lists.xen.org [mailto:xen-devel-
> > > > bounces@lists.xen.org] On Behalf Of Stefano Stabellini
> > > > Sent: 23 July 2015 18:28
> > > > To: xen-devel@lists.xensource.com
> > > > Cc: Wei Liu; Ian Jackson; Ian Campbell; Stefano Stabellini
> > > > Subject: [Xen-devel] [PATCH v5 1/6] libxl: do not add a vkb backend to
> > hvm
> > > > guests
> > > >
> > > > When QEMU restricts its xenstore connection, it cannot provide PV
> > > > backends. A separate QEMU instance is required to provide PV backends
> > in
> > > > userspace, such as qdisk. With two separate instances, it is not
> > > > possible to take advantage of vkb for mouse and keyboard, as the QEMU
> > > > that emulates the graphic card (the device model), would be separate
> > > > from the QEMU running the vkb backend (PV QEMU).
> > > >
> > > > Removing this functionality is acceptable, because is only useful for
> > > > power saving when usb emulation is off, letting QEMU sleep for longer
> > > > periods of time.  However usb emulation is on by default, and how to
> > > > take advantage of this configuration has never been documented.
> > > >
> > >
> > > I don't think I agree. Turning off USB emulation for HVM guests (particularly
> > Windows) has been shown to be highly advantageous in performance and
> > scalability terms, and we have a prototype HID driver (not yet part of the
> > XenProject driver set, but hopefully soon will be) which uses vkb.
> > 
> > I would appreciate if this kind of comments were made at v1 or v2, not
> > v5 of a series :-)
> > 
> 
> Yes, I realise that, but I've been busy... sorry.
> 
> > 
> > I know that turning USB emulation off is a big win, but nobody is really
> > doing it. The reason is that we didn't properly documented how to do it.
> 
> It's documented for XenServer and we have toolstack support to do it.

You could still use it if you call libxl_device_vkb_add explicitely and
you avoid creating any of depriv QEMU users (xen-qemudepriv-domid* and
xen-qemudepriv-shared).



> > As you say, not even the Xen Project Windows PV drivers take advantage
> > of vkb yet, even though they might soon. I still think that removing vkb
> > cannot be considered a regression.
> > 
> > If it comes to a choice, I think that securing QEMU is more important
> > that turning USB emulation off and the two are fundamentally
> > incompatible.
> > 
> > Even if we run two QEMUs, one for emulation, one for the backends, the
> > vkb backend would need to be running in the same QEMU that offers vga
> > emulation because of the cursor rendering. It is a no go.
> 
> I realise it would be a bit odd typing into one window and seeing output in another, but is that a reason to disallow it?

The reason is that it is a complex solution: we would need 2 vnc
servers, one for the QEMU that does emulation and one for the QEMU that
runs the PV backends. They would need to bind to different ports. And
the benefit is doubtful because, as you wrote, it would be difficult to
use. I wouldn't want to add code to handle this case to libxl as part of
this series.



> > We could expose vkb but only together with vfb, so the vkb cursor would
> > only affect the vfb screen and not the vga screen. I don't know if you
> > would like that solution, but in any case it could still be introduced
> > after this work as it is independent.
> > 
> > Another possibility would be to add an option to request a vkb backend
> > for HVM guests in the config file. If the option is selected QEMU would
> > run as root. I wouldn't dare to recommend it to anybody.
> > 
> > 
> > > > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > > > Acked-by: Ian Campbell <ian.campbell@citrix.com>
> > > > ---
> > > >  tools/libxl/libxl_create.c |    5 -----
> > > >  1 file changed, 5 deletions(-)
> > > >
> > > > diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> > > > index f0da7dc..a74b340 100644
> > > > --- a/tools/libxl/libxl_create.c
> > > > +++ b/tools/libxl/libxl_create.c
> > > > @@ -1275,17 +1275,12 @@ static void domcreate_launch_dm(libxl__egc
> > > > *egc, libxl__multidev *multidev,
> > > >      {
> > > >          libxl__device_console console;
> > > >          libxl__device device;
> > > > -        libxl_device_vkb vkb;
> > > >
> > > >          init_console_info(gc, &console, 0);
> > > >          console.backend_domid = state->console_domid;
> > > >          libxl__device_console_add(gc, domid, &console, state, &device);
> > > >          libxl__device_console_dispose(&console);
> > > >
> > > > -        libxl_device_vkb_init(&vkb);
> > > > -        libxl__device_vkb_add(gc, domid, &vkb);
> > > > -        libxl_device_vkb_dispose(&vkb);
> > > > -
> > > >          dcs->dmss.dm.guest_domid = domid;
> > > >          if (libxl_defbool_val(d_config->b_info.device_model_stubdomain))
> > > >              libxl__spawn_stub_dm(egc, &dcs->dmss);
> > > > --
> > > > 1.7.10.4
> > > >
> > > >
> > > > _______________________________________________
> > > > Xen-devel mailing list
> > > > Xen-devel@lists.xen.org
> > > > http://lists.xen.org/xen-devel
> > >
> 

Re: [PATCH v5 1/6] libxl: do not add a vkb backend to hvm guests

[Ian Campbell]

On Fri, 2015-07-24 at 11:56 +0100, Stefano Stabellini wrote:
> On Fri, 24 Jul 2015, Paul Durrant wrote:
> > > -----Original Message-----
> > > From: Stefano Stabellini [mailto:stefano.stabellini@eu.citrix.com
> > > ]
> > > Sent: 24 July 2015 11:21
> > > To: Paul Durrant
> > > Cc: Stefano Stabellini; xen-devel@lists.xensource.com; Wei Liu; 
> > > Ian Jackson;
> > > Ian Campbell
> > > Subject: RE: [Xen-devel] [PATCH v5 1/6] libxl: do not add a vkb 
> > > backend to
> > > hvm guests
> > > 
> > > On Fri, 24 Jul 2015, Paul Durrant wrote:
> > > > > -----Original Message-----
> > > > > From: xen-devel-bounces@lists.xen.org [mailto:xen-devel-
> > > > > bounces@lists.xen.org] On Behalf Of Stefano Stabellini
> > > > > Sent: 23 July 2015 18:28
> > > > > To: xen-devel@lists.xensource.com
> > > > > Cc: Wei Liu; Ian Jackson; Ian Campbell; Stefano Stabellini
> > > > > Subject: [Xen-devel] [PATCH v5 1/6] libxl: do not add a vkb 
> > > > > backend to
> > > hvm
> > > > > guests
> > > > > 
> > > > > When QEMU restricts its xenstore connection, it cannot 
> > > > > provide PV
> > > > > backends. A separate QEMU instance is required to provide PV 
> > > > > backends
> > > in
> > > > > userspace, such as qdisk. With two separate instances, it is 
> > > > > not
> > > > > possible to take advantage of vkb for mouse and keyboard, as 
> > > > > the QEMU
> > > > > that emulates the graphic card (the device model), would be 
> > > > > separate
> > > > > from the QEMU running the vkb backend (PV QEMU).
> > > > > 
> > > > > Removing this functionality is acceptable, because is only 
> > > > > useful for
> > > > > power saving when usb emulation is off, letting QEMU sleep 
> > > > > for longer
> > > > > periods of time.  However usb emulation is on by default, and 
> > > > > how to
> > > > > take advantage of this configuration has never been 
> > > > > documented.
> > > > > 
> > > > 
> > > > I don't think I agree. Turning off USB emulation for HVM guests 
> > > > (particularly
> > > Windows) has been shown to be highly advantageous in performance 
> > > and
> > > scalability terms, and we have a prototype HID driver (not yet 
> > > part of the
> > > XenProject driver set, but hopefully soon will be) which uses 
> > > vkb.
> > > 
> > > I would appreciate if this kind of comments were made at v1 or 
> > > v2, not
> > > v5 of a series :-)
> > > 
> > 
> > Yes, I realise that, but I've been busy... sorry.
> > 
> > > 
> > > I know that turning USB emulation off is a big win, but nobody is 
> > > really
> > > doing it. The reason is that we didn't properly documented how to 
> > > do it.
> > 
> > It's documented for XenServer and we have toolstack support to do 
> > it.
> 
> You could still use it if you call libxl_device_vkb_add explicitely and
> you avoid creating any of depriv QEMU users (xen-qemudepriv-domid* and
> xen-qemudepriv-shared).

There really ought to be a way at the libxl level (but not necessarily
in xl) to disable the depriv without having to arrange things "just so"
such that it doesn't occur...

Ian.

Re: [PATCH v5 1/6] libxl: do not add a vkb backend to hvm guests

[Ian Jackson]

Ian Campbell writes ("Re: [Xen-devel] [PATCH v5 1/6] libxl: do not add a vkb backend to hvm guests"):
> There really ought to be a way at the libxl level (but not necessarily
> in xl) to disable the depriv without having to arrange things "just so"
> such that it doesn't occur...

And a way to force it to "on" so that if things are accidentally
arranged so that it wouldn't happen, we fail instead.

Ian.

Re: [PATCH v5 1/6] libxl: do not add a vkb backend to hvm guests

[Stefano Stabellini]

On Fri, 24 Jul 2015, Ian Campbell wrote:
> On Fri, 2015-07-24 at 11:56 +0100, Stefano Stabellini wrote:
> > On Fri, 24 Jul 2015, Paul Durrant wrote:
> > > > -----Original Message-----
> > > > From: Stefano Stabellini [mailto:stefano.stabellini@eu.citrix.com
> > > > ]
> > > > Sent: 24 July 2015 11:21
> > > > To: Paul Durrant
> > > > Cc: Stefano Stabellini; xen-devel@lists.xensource.com; Wei Liu; 
> > > > Ian Jackson;
> > > > Ian Campbell
> > > > Subject: RE: [Xen-devel] [PATCH v5 1/6] libxl: do not add a vkb 
> > > > backend to
> > > > hvm guests
> > > > 
> > > > On Fri, 24 Jul 2015, Paul Durrant wrote:
> > > > > > -----Original Message-----
> > > > > > From: xen-devel-bounces@lists.xen.org [mailto:xen-devel-
> > > > > > bounces@lists.xen.org] On Behalf Of Stefano Stabellini
> > > > > > Sent: 23 July 2015 18:28
> > > > > > To: xen-devel@lists.xensource.com
> > > > > > Cc: Wei Liu; Ian Jackson; Ian Campbell; Stefano Stabellini
> > > > > > Subject: [Xen-devel] [PATCH v5 1/6] libxl: do not add a vkb 
> > > > > > backend to
> > > > hvm
> > > > > > guests
> > > > > > 
> > > > > > When QEMU restricts its xenstore connection, it cannot 
> > > > > > provide PV
> > > > > > backends. A separate QEMU instance is required to provide PV 
> > > > > > backends
> > > > in
> > > > > > userspace, such as qdisk. With two separate instances, it is 
> > > > > > not
> > > > > > possible to take advantage of vkb for mouse and keyboard, as 
> > > > > > the QEMU
> > > > > > that emulates the graphic card (the device model), would be 
> > > > > > separate
> > > > > > from the QEMU running the vkb backend (PV QEMU).
> > > > > > 
> > > > > > Removing this functionality is acceptable, because is only 
> > > > > > useful for
> > > > > > power saving when usb emulation is off, letting QEMU sleep 
> > > > > > for longer
> > > > > > periods of time.  However usb emulation is on by default, and 
> > > > > > how to
> > > > > > take advantage of this configuration has never been 
> > > > > > documented.
> > > > > > 
> > > > > 
> > > > > I don't think I agree. Turning off USB emulation for HVM guests 
> > > > > (particularly
> > > > Windows) has been shown to be highly advantageous in performance 
> > > > and
> > > > scalability terms, and we have a prototype HID driver (not yet 
> > > > part of the
> > > > XenProject driver set, but hopefully soon will be) which uses 
> > > > vkb.
> > > > 
> > > > I would appreciate if this kind of comments were made at v1 or 
> > > > v2, not
> > > > v5 of a series :-)
> > > > 
> > > 
> > > Yes, I realise that, but I've been busy... sorry.
> > > 
> > > > 
> > > > I know that turning USB emulation off is a big win, but nobody is 
> > > > really
> > > > doing it. The reason is that we didn't properly documented how to 
> > > > do it.
> > > 
> > > It's documented for XenServer and we have toolstack support to do 
> > > it.
> > 
> > You could still use it if you call libxl_device_vkb_add explicitely and
> > you avoid creating any of depriv QEMU users (xen-qemudepriv-domid* and
> > xen-qemudepriv-shared).
> 
> There really ought to be a way at the libxl level (but not necessarily
> in xl) to disable the depriv without having to arrange things "just so"
> such that it doesn't occur...

I have already introduced a device_model_user option, would that be
enough? We could check on device_model_user == root.

Re: [PATCH v5 1/6] libxl: do not add a vkb backend to hvm guests

[Ian Campbell]

On Fri, 2015-07-24 at 12:29 +0100, Stefano Stabellini wrote:
> 
> > There really ought to be a way at the libxl level (but not 
> > necessarily
> > in xl) to disable the depriv without having to arrange things "just 
> > so"
> > such that it doesn't occur...
> 
> I have already introduced a device_model_user option, would that be
> enough? We could check on device_model_user == root.

Sounds sufficient. If --runas=root does what is desired you may not
even need special handling.

Ian.

Re: [PATCH v5 1/6] libxl: do not add a vkb backend to hvm guests

[Paul Durrant]

> -----Original Message-----
> From: Stefano Stabellini [mailto:stefano.stabellini@eu.citrix.com]
> Sent: 24 July 2015 11:56
> To: Paul Durrant
> Cc: Stefano Stabellini; xen-devel@lists.xensource.com; Wei Liu; Ian Jackson;
> Ian Campbell
> Subject: RE: [Xen-devel] [PATCH v5 1/6] libxl: do not add a vkb backend to
> hvm guests
> 
> On Fri, 24 Jul 2015, Paul Durrant wrote:
> > > -----Original Message-----
> > > From: Stefano Stabellini [mailto:stefano.stabellini@eu.citrix.com]
> > > Sent: 24 July 2015 11:21
> > > To: Paul Durrant
> > > Cc: Stefano Stabellini; xen-devel@lists.xensource.com; Wei Liu; Ian
> Jackson;
> > > Ian Campbell
> > > Subject: RE: [Xen-devel] [PATCH v5 1/6] libxl: do not add a vkb backend to
> > > hvm guests
> > >
> > > On Fri, 24 Jul 2015, Paul Durrant wrote:
> > > > > -----Original Message-----
> > > > > From: xen-devel-bounces@lists.xen.org [mailto:xen-devel-
> > > > > bounces@lists.xen.org] On Behalf Of Stefano Stabellini
> > > > > Sent: 23 July 2015 18:28
> > > > > To: xen-devel@lists.xensource.com
> > > > > Cc: Wei Liu; Ian Jackson; Ian Campbell; Stefano Stabellini
> > > > > Subject: [Xen-devel] [PATCH v5 1/6] libxl: do not add a vkb backend to
> > > hvm
> > > > > guests
> > > > >
> > > > > When QEMU restricts its xenstore connection, it cannot provide PV
> > > > > backends. A separate QEMU instance is required to provide PV
> backends
> > > in
> > > > > userspace, such as qdisk. With two separate instances, it is not
> > > > > possible to take advantage of vkb for mouse and keyboard, as the
> QEMU
> > > > > that emulates the graphic card (the device model), would be separate
> > > > > from the QEMU running the vkb backend (PV QEMU).
> > > > >
> > > > > Removing this functionality is acceptable, because is only useful for
> > > > > power saving when usb emulation is off, letting QEMU sleep for
> longer
> > > > > periods of time.  However usb emulation is on by default, and how to
> > > > > take advantage of this configuration has never been documented.
> > > > >
> > > >
> > > > I don't think I agree. Turning off USB emulation for HVM guests
> (particularly
> > > Windows) has been shown to be highly advantageous in performance
> and
> > > scalability terms, and we have a prototype HID driver (not yet part of the
> > > XenProject driver set, but hopefully soon will be) which uses vkb.
> > >
> > > I would appreciate if this kind of comments were made at v1 or v2, not
> > > v5 of a series :-)
> > >
> >
> > Yes, I realise that, but I've been busy... sorry.
> >
> > >
> > > I know that turning USB emulation off is a big win, but nobody is really
> > > doing it. The reason is that we didn't properly documented how to do it.
> >
> > It's documented for XenServer and we have toolstack support to do it.
> 
> You could still use it if you call libxl_device_vkb_add explicitely and
> you avoid creating any of depriv QEMU users (xen-qemudepriv-domid* and
> xen-qemudepriv-shared).
> 
> 
> 
> > > As you say, not even the Xen Project Windows PV drivers take advantage
> > > of vkb yet, even though they might soon. I still think that removing vkb
> > > cannot be considered a regression.
> > >
> > > If it comes to a choice, I think that securing QEMU is more important
> > > that turning USB emulation off and the two are fundamentally
> > > incompatible.
> > >
> > > Even if we run two QEMUs, one for emulation, one for the backends, the
> > > vkb backend would need to be running in the same QEMU that offers vga
> > > emulation because of the cursor rendering. It is a no go.
> >
> > I realise it would be a bit odd typing into one window and seeing output in
> another, but is that a reason to disallow it?
> 
> The reason is that it is a complex solution: we would need 2 vnc
> servers, one for the QEMU that does emulation and one for the QEMU that
> runs the PV backends. They would need to bind to different ports. And
> the benefit is doubtful because, as you wrote, it would be difficult to
> use. I wouldn't want to add code to handle this case to libxl as part of
> this series.
> 

You'd need a console in both QEMUs but I don't think that's necessarily a problem is it? Clearly, if you are going to use a simple VNC client, it's going to look weird. But it would be feasible to write a client that sends kbd/mouse messages to two servers whilst only displaying the framebuffer of one. I really don't think there's any reason to enforce no vkb for HVM guests.

  Paul

> 
> 
> > > We could expose vkb but only together with vfb, so the vkb cursor would
> > > only affect the vfb screen and not the vga screen. I don't know if you
> > > would like that solution, but in any case it could still be introduced
> > > after this work as it is independent.
> > >
> > > Another possibility would be to add an option to request a vkb backend
> > > for HVM guests in the config file. If the option is selected QEMU would
> > > run as root. I wouldn't dare to recommend it to anybody.
> > >
> > >
> > > > > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > > > > Acked-by: Ian Campbell <ian.campbell@citrix.com>
> > > > > ---
> > > > >  tools/libxl/libxl_create.c |    5 -----
> > > > >  1 file changed, 5 deletions(-)
> > > > >
> > > > > diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> > > > > index f0da7dc..a74b340 100644
> > > > > --- a/tools/libxl/libxl_create.c
> > > > > +++ b/tools/libxl/libxl_create.c
> > > > > @@ -1275,17 +1275,12 @@ static void
> domcreate_launch_dm(libxl__egc
> > > > > *egc, libxl__multidev *multidev,
> > > > >      {
> > > > >          libxl__device_console console;
> > > > >          libxl__device device;
> > > > > -        libxl_device_vkb vkb;
> > > > >
> > > > >          init_console_info(gc, &console, 0);
> > > > >          console.backend_domid = state->console_domid;
> > > > >          libxl__device_console_add(gc, domid, &console, state, &device);
> > > > >          libxl__device_console_dispose(&console);
> > > > >
> > > > > -        libxl_device_vkb_init(&vkb);
> > > > > -        libxl__device_vkb_add(gc, domid, &vkb);
> > > > > -        libxl_device_vkb_dispose(&vkb);
> > > > > -
> > > > >          dcs->dmss.dm.guest_domid = domid;
> > > > >          if (libxl_defbool_val(d_config-
> >b_info.device_model_stubdomain))
> > > > >              libxl__spawn_stub_dm(egc, &dcs->dmss);
> > > > > --
> > > > > 1.7.10.4
> > > > >
> > > > >
> > > > > _______________________________________________
> > > > > Xen-devel mailing list
> > > > > Xen-devel@lists.xen.org
> > > > > http://lists.xen.org/xen-devel
> > > >
> >

Re: [PATCH v5 1/6] libxl: do not add a vkb backend to hvm guests

[Stefano Stabellini]

On Fri, 24 Jul 2015, Paul Durrant wrote:
> > -----Original Message-----
> > From: Stefano Stabellini [mailto:stefano.stabellini@eu.citrix.com]
> > Sent: 24 July 2015 11:56
> > To: Paul Durrant
> > Cc: Stefano Stabellini; xen-devel@lists.xensource.com; Wei Liu; Ian Jackson;
> > Ian Campbell
> > Subject: RE: [Xen-devel] [PATCH v5 1/6] libxl: do not add a vkb backend to
> > hvm guests
> > 
> > On Fri, 24 Jul 2015, Paul Durrant wrote:
> > > > -----Original Message-----
> > > > From: Stefano Stabellini [mailto:stefano.stabellini@eu.citrix.com]
> > > > Sent: 24 July 2015 11:21
> > > > To: Paul Durrant
> > > > Cc: Stefano Stabellini; xen-devel@lists.xensource.com; Wei Liu; Ian
> > Jackson;
> > > > Ian Campbell
> > > > Subject: RE: [Xen-devel] [PATCH v5 1/6] libxl: do not add a vkb backend to
> > > > hvm guests
> > > >
> > > > On Fri, 24 Jul 2015, Paul Durrant wrote:
> > > > > > -----Original Message-----
> > > > > > From: xen-devel-bounces@lists.xen.org [mailto:xen-devel-
> > > > > > bounces@lists.xen.org] On Behalf Of Stefano Stabellini
> > > > > > Sent: 23 July 2015 18:28
> > > > > > To: xen-devel@lists.xensource.com
> > > > > > Cc: Wei Liu; Ian Jackson; Ian Campbell; Stefano Stabellini
> > > > > > Subject: [Xen-devel] [PATCH v5 1/6] libxl: do not add a vkb backend to
> > > > hvm
> > > > > > guests
> > > > > >
> > > > > > When QEMU restricts its xenstore connection, it cannot provide PV
> > > > > > backends. A separate QEMU instance is required to provide PV
> > backends
> > > > in
> > > > > > userspace, such as qdisk. With two separate instances, it is not
> > > > > > possible to take advantage of vkb for mouse and keyboard, as the
> > QEMU
> > > > > > that emulates the graphic card (the device model), would be separate
> > > > > > from the QEMU running the vkb backend (PV QEMU).
> > > > > >
> > > > > > Removing this functionality is acceptable, because is only useful for
> > > > > > power saving when usb emulation is off, letting QEMU sleep for
> > longer
> > > > > > periods of time.  However usb emulation is on by default, and how to
> > > > > > take advantage of this configuration has never been documented.
> > > > > >
> > > > >
> > > > > I don't think I agree. Turning off USB emulation for HVM guests
> > (particularly
> > > > Windows) has been shown to be highly advantageous in performance
> > and
> > > > scalability terms, and we have a prototype HID driver (not yet part of the
> > > > XenProject driver set, but hopefully soon will be) which uses vkb.
> > > >
> > > > I would appreciate if this kind of comments were made at v1 or v2, not
> > > > v5 of a series :-)
> > > >
> > >
> > > Yes, I realise that, but I've been busy... sorry.
> > >
> > > >
> > > > I know that turning USB emulation off is a big win, but nobody is really
> > > > doing it. The reason is that we didn't properly documented how to do it.
> > >
> > > It's documented for XenServer and we have toolstack support to do it.
> > 
> > You could still use it if you call libxl_device_vkb_add explicitely and
> > you avoid creating any of depriv QEMU users (xen-qemudepriv-domid* and
> > xen-qemudepriv-shared).
> > 
> > 
> > 
> > > > As you say, not even the Xen Project Windows PV drivers take advantage
> > > > of vkb yet, even though they might soon. I still think that removing vkb
> > > > cannot be considered a regression.
> > > >
> > > > If it comes to a choice, I think that securing QEMU is more important
> > > > that turning USB emulation off and the two are fundamentally
> > > > incompatible.
> > > >
> > > > Even if we run two QEMUs, one for emulation, one for the backends, the
> > > > vkb backend would need to be running in the same QEMU that offers vga
> > > > emulation because of the cursor rendering. It is a no go.
> > >
> > > I realise it would be a bit odd typing into one window and seeing output in
> > another, but is that a reason to disallow it?
> > 
> > The reason is that it is a complex solution: we would need 2 vnc
> > servers, one for the QEMU that does emulation and one for the QEMU that
> > runs the PV backends. They would need to bind to different ports. And
> > the benefit is doubtful because, as you wrote, it would be difficult to
> > use. I wouldn't want to add code to handle this case to libxl as part of
> > this series.
> > 
> 
> You'd need a console in both QEMUs but I don't think that's necessarily a problem is it? Clearly, if you are going to use a simple VNC client, it's going to look weird. But it would be feasible to write a client that sends kbd/mouse messages to two servers whilst only displaying the framebuffer of one. I really don't think there's any reason to enforce no vkb for HVM guests.

I am afraid it could confuse some unprepared frontends. For example a
PV on HVM linux user might be confused by the outcome. I would rather go
with allowing people to ask for QEMU to run as root.

Re: [PATCH v5 1/6] libxl: do not add a vkb backend to hvm guests

[Stefano Stabellini]

On Fri, 24 Jul 2015, Stefano Stabellini wrote:
> On Fri, 24 Jul 2015, Paul Durrant wrote:
> > > -----Original Message-----
> > > From: Stefano Stabellini [mailto:stefano.stabellini@eu.citrix.com]
> > > Sent: 24 July 2015 11:56
> > > To: Paul Durrant
> > > Cc: Stefano Stabellini; xen-devel@lists.xensource.com; Wei Liu; Ian Jackson;
> > > Ian Campbell
> > > Subject: RE: [Xen-devel] [PATCH v5 1/6] libxl: do not add a vkb backend to
> > > hvm guests
> > > 
> > > On Fri, 24 Jul 2015, Paul Durrant wrote:
> > > > > -----Original Message-----
> > > > > From: Stefano Stabellini [mailto:stefano.stabellini@eu.citrix.com]
> > > > > Sent: 24 July 2015 11:21
> > > > > To: Paul Durrant
> > > > > Cc: Stefano Stabellini; xen-devel@lists.xensource.com; Wei Liu; Ian
> > > Jackson;
> > > > > Ian Campbell
> > > > > Subject: RE: [Xen-devel] [PATCH v5 1/6] libxl: do not add a vkb backend to
> > > > > hvm guests
> > > > >
> > > > > On Fri, 24 Jul 2015, Paul Durrant wrote:
> > > > > > > -----Original Message-----
> > > > > > > From: xen-devel-bounces@lists.xen.org [mailto:xen-devel-
> > > > > > > bounces@lists.xen.org] On Behalf Of Stefano Stabellini
> > > > > > > Sent: 23 July 2015 18:28
> > > > > > > To: xen-devel@lists.xensource.com
> > > > > > > Cc: Wei Liu; Ian Jackson; Ian Campbell; Stefano Stabellini
> > > > > > > Subject: [Xen-devel] [PATCH v5 1/6] libxl: do not add a vkb backend to
> > > > > hvm
> > > > > > > guests
> > > > > > >
> > > > > > > When QEMU restricts its xenstore connection, it cannot provide PV
> > > > > > > backends. A separate QEMU instance is required to provide PV
> > > backends
> > > > > in
> > > > > > > userspace, such as qdisk. With two separate instances, it is not
> > > > > > > possible to take advantage of vkb for mouse and keyboard, as the
> > > QEMU
> > > > > > > that emulates the graphic card (the device model), would be separate
> > > > > > > from the QEMU running the vkb backend (PV QEMU).
> > > > > > >
> > > > > > > Removing this functionality is acceptable, because is only useful for
> > > > > > > power saving when usb emulation is off, letting QEMU sleep for
> > > longer
> > > > > > > periods of time.  However usb emulation is on by default, and how to
> > > > > > > take advantage of this configuration has never been documented.
> > > > > > >
> > > > > >
> > > > > > I don't think I agree. Turning off USB emulation for HVM guests
> > > (particularly
> > > > > Windows) has been shown to be highly advantageous in performance
> > > and
> > > > > scalability terms, and we have a prototype HID driver (not yet part of the
> > > > > XenProject driver set, but hopefully soon will be) which uses vkb.
> > > > >
> > > > > I would appreciate if this kind of comments were made at v1 or v2, not
> > > > > v5 of a series :-)
> > > > >
> > > >
> > > > Yes, I realise that, but I've been busy... sorry.
> > > >
> > > > >
> > > > > I know that turning USB emulation off is a big win, but nobody is really
> > > > > doing it. The reason is that we didn't properly documented how to do it.
> > > >
> > > > It's documented for XenServer and we have toolstack support to do it.
> > > 
> > > You could still use it if you call libxl_device_vkb_add explicitely and
> > > you avoid creating any of depriv QEMU users (xen-qemudepriv-domid* and
> > > xen-qemudepriv-shared).
> > > 
> > > 
> > > 
> > > > > As you say, not even the Xen Project Windows PV drivers take advantage
> > > > > of vkb yet, even though they might soon. I still think that removing vkb
> > > > > cannot be considered a regression.
> > > > >
> > > > > If it comes to a choice, I think that securing QEMU is more important
> > > > > that turning USB emulation off and the two are fundamentally
> > > > > incompatible.
> > > > >
> > > > > Even if we run two QEMUs, one for emulation, one for the backends, the
> > > > > vkb backend would need to be running in the same QEMU that offers vga
> > > > > emulation because of the cursor rendering. It is a no go.
> > > >
> > > > I realise it would be a bit odd typing into one window and seeing output in
> > > another, but is that a reason to disallow it?
> > > 
> > > The reason is that it is a complex solution: we would need 2 vnc
> > > servers, one for the QEMU that does emulation and one for the QEMU that
> > > runs the PV backends. They would need to bind to different ports. And
> > > the benefit is doubtful because, as you wrote, it would be difficult to
> > > use. I wouldn't want to add code to handle this case to libxl as part of
> > > this series.
> > > 
> > 
> > You'd need a console in both QEMUs but I don't think that's necessarily a problem is it? Clearly, if you are going to use a simple VNC client, it's going to look weird. But it would be feasible to write a client that sends kbd/mouse messages to two servers whilst only displaying the framebuffer of one. I really don't think there's any reason to enforce no vkb for HVM guests.
> 
> I am afraid it could confuse some unprepared frontends. For example a
> PV on HVM linux user might be confused by the outcome. I would rather go
> with allowing people to ask for QEMU to run as root.

Actually Paul is right, no confusion. I can just drop this patch and
everything works as it should. In the depriv case, we just have one more
backend that is never going to receive any events from the user or
inject any into the guest.

Re: [PATCH v5 1/6] libxl: do not add a vkb backend to hvm guests

[Paul Durrant]

> -----Original Message-----
> From: Stefano Stabellini [mailto:stefano.stabellini@eu.citrix.com]
> Sent: 24 July 2015 15:11
> To: Stefano Stabellini
> Cc: Paul Durrant; Stefano Stabellini; xen-devel@lists.xensource.com; Wei Liu;
> Ian Jackson; Ian Campbell
> Subject: RE: [Xen-devel] [PATCH v5 1/6] libxl: do not add a vkb backend to
> hvm guests
> 
> On Fri, 24 Jul 2015, Stefano Stabellini wrote:
> > On Fri, 24 Jul 2015, Paul Durrant wrote:
> > > > -----Original Message-----
> > > > From: Stefano Stabellini [mailto:stefano.stabellini@eu.citrix.com]
> > > > Sent: 24 July 2015 11:56
> > > > To: Paul Durrant
> > > > Cc: Stefano Stabellini; xen-devel@lists.xensource.com; Wei Liu; Ian
> Jackson;
> > > > Ian Campbell
> > > > Subject: RE: [Xen-devel] [PATCH v5 1/6] libxl: do not add a vkb backend
> to
> > > > hvm guests
> > > >
> > > > On Fri, 24 Jul 2015, Paul Durrant wrote:
> > > > > > -----Original Message-----
> > > > > > From: Stefano Stabellini [mailto:stefano.stabellini@eu.citrix.com]
> > > > > > Sent: 24 July 2015 11:21
> > > > > > To: Paul Durrant
> > > > > > Cc: Stefano Stabellini; xen-devel@lists.xensource.com; Wei Liu; Ian
> > > > Jackson;
> > > > > > Ian Campbell
> > > > > > Subject: RE: [Xen-devel] [PATCH v5 1/6] libxl: do not add a vkb
> backend to
> > > > > > hvm guests
> > > > > >
> > > > > > On Fri, 24 Jul 2015, Paul Durrant wrote:
> > > > > > > > -----Original Message-----
> > > > > > > > From: xen-devel-bounces@lists.xen.org [mailto:xen-devel-
> > > > > > > > bounces@lists.xen.org] On Behalf Of Stefano Stabellini
> > > > > > > > Sent: 23 July 2015 18:28
> > > > > > > > To: xen-devel@lists.xensource.com
> > > > > > > > Cc: Wei Liu; Ian Jackson; Ian Campbell; Stefano Stabellini
> > > > > > > > Subject: [Xen-devel] [PATCH v5 1/6] libxl: do not add a vkb
> backend to
> > > > > > hvm
> > > > > > > > guests
> > > > > > > >
> > > > > > > > When QEMU restricts its xenstore connection, it cannot provide
> PV
> > > > > > > > backends. A separate QEMU instance is required to provide PV
> > > > backends
> > > > > > in
> > > > > > > > userspace, such as qdisk. With two separate instances, it is not
> > > > > > > > possible to take advantage of vkb for mouse and keyboard, as
> the
> > > > QEMU
> > > > > > > > that emulates the graphic card (the device model), would be
> separate
> > > > > > > > from the QEMU running the vkb backend (PV QEMU).
> > > > > > > >
> > > > > > > > Removing this functionality is acceptable, because is only useful
> for
> > > > > > > > power saving when usb emulation is off, letting QEMU sleep for
> > > > longer
> > > > > > > > periods of time.  However usb emulation is on by default, and
> how to
> > > > > > > > take advantage of this configuration has never been
> documented.
> > > > > > > >
> > > > > > >
> > > > > > > I don't think I agree. Turning off USB emulation for HVM guests
> > > > (particularly
> > > > > > Windows) has been shown to be highly advantageous in
> performance
> > > > and
> > > > > > scalability terms, and we have a prototype HID driver (not yet part
> of the
> > > > > > XenProject driver set, but hopefully soon will be) which uses vkb.
> > > > > >
> > > > > > I would appreciate if this kind of comments were made at v1 or v2,
> not
> > > > > > v5 of a series :-)
> > > > > >
> > > > >
> > > > > Yes, I realise that, but I've been busy... sorry.
> > > > >
> > > > > >
> > > > > > I know that turning USB emulation off is a big win, but nobody is
> really
> > > > > > doing it. The reason is that we didn't properly documented how to
> do it.
> > > > >
> > > > > It's documented for XenServer and we have toolstack support to do
> it.
> > > >
> > > > You could still use it if you call libxl_device_vkb_add explicitely and
> > > > you avoid creating any of depriv QEMU users (xen-qemudepriv-domid*
> and
> > > > xen-qemudepriv-shared).
> > > >
> > > >
> > > >
> > > > > > As you say, not even the Xen Project Windows PV drivers take
> advantage
> > > > > > of vkb yet, even though they might soon. I still think that removing
> vkb
> > > > > > cannot be considered a regression.
> > > > > >
> > > > > > If it comes to a choice, I think that securing QEMU is more important
> > > > > > that turning USB emulation off and the two are fundamentally
> > > > > > incompatible.
> > > > > >
> > > > > > Even if we run two QEMUs, one for emulation, one for the
> backends, the
> > > > > > vkb backend would need to be running in the same QEMU that
> offers vga
> > > > > > emulation because of the cursor rendering. It is a no go.
> > > > >
> > > > > I realise it would be a bit odd typing into one window and seeing
> output in
> > > > another, but is that a reason to disallow it?
> > > >
> > > > The reason is that it is a complex solution: we would need 2 vnc
> > > > servers, one for the QEMU that does emulation and one for the QEMU
> that
> > > > runs the PV backends. They would need to bind to different ports. And
> > > > the benefit is doubtful because, as you wrote, it would be difficult to
> > > > use. I wouldn't want to add code to handle this case to libxl as part of
> > > > this series.
> > > >
> > >
> > > You'd need a console in both QEMUs but I don't think that's necessarily a
> problem is it? Clearly, if you are going to use a simple VNC client, it's going to
> look weird. But it would be feasible to write a client that sends kbd/mouse
> messages to two servers whilst only displaying the framebuffer of one. I
> really don't think there's any reason to enforce no vkb for HVM guests.
> >
> > I am afraid it could confuse some unprepared frontends. For example a
> > PV on HVM linux user might be confused by the outcome. I would rather
> go
> > with allowing people to ask for QEMU to run as root.
> 
> Actually Paul is right, no confusion. I can just drop this patch and
> everything works as it should. In the depriv case, we just have one more
> backend that is never going to receive any events from the user or
> inject any into the guest.

Excellent. Thanks for that :-)

  Paul

Re: [PATCH v5 1/6] libxl: do not add a vkb backend to hvm guests

[Fabio Fantoni]

Il 24/07/2015 12:21, Stefano Stabellini ha scritto:
> On Fri, 24 Jul 2015, Paul Durrant wrote:
>>> -----Original Message-----
>>> From: xen-devel-bounces@lists.xen.org [mailto:xen-devel-
>>> bounces@lists.xen.org] On Behalf Of Stefano Stabellini
>>> Sent: 23 July 2015 18:28
>>> To: xen-devel@lists.xensource.com
>>> Cc: Wei Liu; Ian Jackson; Ian Campbell; Stefano Stabellini
>>> Subject: [Xen-devel] [PATCH v5 1/6] libxl: do not add a vkb backend to hvm
>>> guests
>>>
>>> When QEMU restricts its xenstore connection, it cannot provide PV
>>> backends. A separate QEMU instance is required to provide PV backends in
>>> userspace, such as qdisk. With two separate instances, it is not
>>> possible to take advantage of vkb for mouse and keyboard, as the QEMU
>>> that emulates the graphic card (the device model), would be separate
>>> from the QEMU running the vkb backend (PV QEMU).
>>>
>>> Removing this functionality is acceptable, because is only useful for
>>> power saving when usb emulation is off, letting QEMU sleep for longer
>>> periods of time.  However usb emulation is on by default, and how to
>>> take advantage of this configuration has never been documented.
>>>
>> I don't think I agree. Turning off USB emulation for HVM guests (particularly Windows) has been shown to be highly advantageous in performance and scalability terms, and we have a prototype HID driver (not yet part of the XenProject driver set, but hopefully soon will be) which uses vkb.
> I would appreciate if this kind of comments were made at v1 or v2, not
> v5 of a series :-)
>
>
> I know that turning USB emulation off is a big win, but nobody is really
> doing it. The reason is that we didn't properly documented how to do it.
> As you say, not even the Xen Project Windows PV drivers take advantage
> of vkb yet, even though they might soon. I still think that removing vkb
> cannot be considered a regression.
>
> If it comes to a choice, I think that securing QEMU is more important
> that turning USB emulation off and the two are fundamentally
> incompatible.
>
> Even if we run two QEMUs, one for emulation, one for the backends, the
> vkb backend would need to be running in the same QEMU that offers vga
> emulation because of the cursor rendering. It is a no go.
>
> We could expose vkb but only together with vfb, so the vkb cursor would
> only affect the vfb screen and not the vga screen. I don't know if you
> would like that solution, but in any case it could still be introduced
> after this work as it is independent.
>
> Another possibility would be to add an option to request a vkb backend
> for HVM guests in the config file. If the option is selected QEMU would
> run as root. I wouldn't dare to recommend it to anybody.

Time ago I tried this patch (without others of this serie) and if I 
remember good I tried also windows domUs (with new winpv drivers) 
without found regressions.
But from a fast look to all patches the thing with more probable 
regression can be the "qemu split" and restriction instead this, today I 
don't have enought time but probably next week I'll try complete serie 
on both linux and windows domUs and including all features that may risk 
regressions FWIK.

>
>
>>> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>>> Acked-by: Ian Campbell <ian.campbell@citrix.com>
>>> ---
>>>   tools/libxl/libxl_create.c |    5 -----
>>>   1 file changed, 5 deletions(-)
>>>
>>> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
>>> index f0da7dc..a74b340 100644
>>> --- a/tools/libxl/libxl_create.c
>>> +++ b/tools/libxl/libxl_create.c
>>> @@ -1275,17 +1275,12 @@ static void domcreate_launch_dm(libxl__egc
>>> *egc, libxl__multidev *multidev,
>>>       {
>>>           libxl__device_console console;
>>>           libxl__device device;
>>> -        libxl_device_vkb vkb;
>>>
>>>           init_console_info(gc, &console, 0);
>>>           console.backend_domid = state->console_domid;
>>>           libxl__device_console_add(gc, domid, &console, state, &device);
>>>           libxl__device_console_dispose(&console);
>>>
>>> -        libxl_device_vkb_init(&vkb);
>>> -        libxl__device_vkb_add(gc, domid, &vkb);
>>> -        libxl_device_vkb_dispose(&vkb);
>>> -
>>>           dcs->dmss.dm.guest_domid = domid;
>>>           if (libxl_defbool_val(d_config->b_info.device_model_stubdomain))
>>>               libxl__spawn_stub_dm(egc, &dcs->dmss);
>>> --
>>> 1.7.10.4
>>>
>>>

[PATCH v5 2/6] [WIP] libxl: xsrestrict QEMU

[Stefano Stabellini]

Check whether QEMU supports the xsrestrict option, by parsing its --help
output. Store the result on xenstore for future reference on a per QEMU
binary basis, so that device_model_override still works fine with it.

Replace / with _ in the QEMU binary path before writing it to xenstore,
so that it doesn't get confused with xenstore paths.

If QEMU supports xsrestrict and emulator_id, pass xsrestrict=on to it.
Statically reserve two emulator_ids, one for device models and another
for pv qemus. Use the emulator_ids appropriately.

WIP: direct use of fork is forbidden in libxl

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

---
Changes in v4:
- update xenstore-paths.markdown

Changes in v3:
- add emulator_ids
- mark as WIP
---
 docs/misc/xenstore-paths.markdown |    8 +++++
 tools/libxl/libxl_dm.c            |   72 +++++++++++++++++++++++++++++++++++++
 tools/libxl/libxl_internal.h      |    7 ++++
 tools/libxl/libxl_utils.c         |   10 ++++++
 4 files changed, 97 insertions(+)

diff --git a/docs/misc/xenstore-paths.markdown b/docs/misc/xenstore-paths.markdown
index d94ea9d..780f601 100644
--- a/docs/misc/xenstore-paths.markdown
+++ b/docs/misc/xenstore-paths.markdown
@@ -397,6 +397,14 @@ The device model version for a domain.
 
 ifb device used by Remus to buffer network output from the associated vif.
 
+#### ~/libxl/$DEVICE_MODEL_BINARY/* [n,INTERNAL]
+
+Contains a list of options supported by the device model, in the form:
+"$OPTION" = ("1"|"0").
+$DEVICE_MODEL_BINARY is the full path to the device model binary with
+'/' replaced by '_'. So for example /usr/lib/xen/bin/qemu-system-i386
+would be /libxl/_usr_lib_xen_bin_qemu-system-i386.
+
 [BLKIF]: http://xenbits.xen.org/docs/unstable/hypercall/include,public,io,blkif.h.html
 [FBIF]: http://xenbits.xen.org/docs/unstable/hypercall/include,public,io,fbif.h.html
 [HVMPARAMS]: http://xenbits.xen.org/docs/unstable/hypercall/include,public,hvm,params.h.html
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 24c43df..455b66c 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -447,6 +447,65 @@ retry:
     return 0;
 }
 
+int libxl__check_qemu_supported(libxl__gc *gc, const char *dm, char *opt)
+{
+    libxl_ctx *ctx = libxl__gc_owner(gc);
+    pid_t pid;
+    int pipefd[2], status;
+    FILE *fp;
+    char *buf;
+    ssize_t buf_size = 512;
+    int ret = 0;
+    char *s;
+
+    s = libxl__strdup(gc, dm);
+    libxl__replace_chr(gc, s, '/', '_');
+    s = libxl__sprintf(gc, "libxl/%s/%s", s, opt);
+    buf = libxl__xs_read(gc, XBT_NULL, s);
+    if (buf != NULL)
+        return !strcmp(buf, "1");
+
+    if (access(dm, X_OK) < 0) {
+        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR,
+                         "device model %s is not executable", dm);
+        return ERROR_FAIL;
+    }
+
+    if (libxl_pipe(ctx, pipefd) < 0)
+        return ERROR_FAIL;
+
+    pid = fork();
+    if (pid < 0)
+        return ERROR_FAIL;
+
+    /* child spawn QEMU */
+    if (!pid) {
+        char *args[] = {(char*)dm, "--help", NULL};
+        close(pipefd[0]);
+        libxl__exec(gc, -1, pipefd[1], pipefd[1], dm, args, NULL);
+        exit(1);
+    }
+
+    /* parent parses the output */
+    close(pipefd[1]);
+    fp = fdopen(pipefd[0], "r");
+    buf = libxl__malloc(gc, buf_size);
+    while (fgets(buf, buf_size, fp) != NULL) {
+        if (strstr(buf, opt) != NULL) {
+            ret = 1;
+            goto out;
+        }
+    }
+out:
+    close(pipefd[0]);
+    waitpid(pid, &status, pid);
+    libxl_report_child_exitstatus(ctx, XTL_WARN, dm, pid, status);
+
+    ret = libxl__xs_write(gc, XBT_NULL, s, "%d", ret);
+
+    return ret;
+}
+
 static char ** libxl__build_device_model_args_new(libxl__gc *gc,
                                         const char *dm, int guest_domid,
                                         const libxl_domain_config *guest_config,
@@ -932,6 +991,14 @@ end_search:
         if (user) {
             flexarray_append(dm_args, "-runas");
             flexarray_append(dm_args, user);
+            if (libxl__check_qemu_supported(gc, dm, "xsrestrict") &&
+                libxl__check_qemu_supported(gc, dm, "emulator_id")) {
+                flexarray_append(dm_args, "-xenopts");
+                flexarray_append(dm_args,
+                        GCSPRINTF("xsrestrict=on,emulator_id=%u",
+                            (b_info->type == LIBXL_DOMAIN_TYPE_PV) ?
+                            QEMU_XEN_PV_ID : QEMU_XEN_DEVICE_MODEL_ID));
+            }
         }
     }
     flexarray_append(dm_args, NULL);
@@ -1658,6 +1725,11 @@ void libxl__spawn_qdisk_backend(libxl__egc *egc, libxl__dm_spawn_state *dmss)
     flexarray_vappend(dm_args, "-monitor", "/dev/null", NULL);
     flexarray_vappend(dm_args, "-serial", "/dev/null", NULL);
     flexarray_vappend(dm_args, "-parallel", "/dev/null", NULL);
+    if (libxl__check_qemu_supported(gc, dm, "emulator_id")) {
+        flexarray_append(dm_args, "-xenopts");
+        flexarray_append(dm_args,
+                GCSPRINTF("emulator_id=%u", QEMU_XEN_PV_ID));
+    }
     flexarray_append(dm_args, NULL);
     args = (char **) flexarray_contents(dm_args);
 
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 7d0af40..b4bae2f 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -106,6 +106,10 @@
 #define TAP_DEVICE_SUFFIX "-emu"
 #define DISABLE_UDEV_PATH "libxl/disable_udev"
 #define DOMID_XS_PATH "domid"
+/* Reserved QEMU emulator_ids. For the moment assume max two QEMUs: one
+ * device model and one PV backends provider. */
+#define QEMU_XEN_DEVICE_MODEL_ID  0
+#define QEMU_XEN_PV_ID            1
 
 #define ARRAY_SIZE(a) (sizeof(a) / sizeof(a[0]))
 
@@ -1505,6 +1509,7 @@ _hidden int libxl__need_xenpv_qemu(libxl__gc *gc,
         int nr_vfbs, libxl_device_vfb *vfbs,
         int nr_disks, libxl_device_disk *disks,
         int nr_channels, libxl_device_channel *channels);
+_hidden int libxl__check_qemu_supported(libxl__gc *gc, const char *dm, char *opt);
 
 /*
  * This function will cause the whole libxl process to hang
@@ -3554,6 +3559,8 @@ int libxl__string_parse_json(libxl__gc *gc, const libxl__json_object *o,
                              char **p);
 
 int libxl__random_bytes(libxl__gc *gc, uint8_t *buf, size_t len);
+/* replace all occurrences of old with new inside s */
+void libxl__replace_chr(libxl__gc *gc, char *s, char old, char new);
 
 /*
  * Compile time assertion
diff --git a/tools/libxl/libxl_utils.c b/tools/libxl/libxl_utils.c
index 67c0b1c..ea08473 100644
--- a/tools/libxl/libxl_utils.c
+++ b/tools/libxl/libxl_utils.c
@@ -1158,6 +1158,16 @@ int libxl__random_bytes(libxl__gc *gc, uint8_t *buf, size_t len)
     return ret;
 }
 
+void libxl__replace_chr(libxl__gc *gc, char *s, char old, char new)
+{
+	int i = 0;
+
+	for (i = 0; s[i] != '\0'; i++) {
+		if (s[i] == old)
+			s[i] = new;
+	}
+}
+
 /*
  * Local variables:
  * mode: C
-- 
1.7.10.4

[PATCH v5 3/6] libxl: allow /local/domain/$LIBXL_TOOLSTACK_DOMID/device-model/$DOMID to be written by $DOMID

[Stefano Stabellini]

The device model is going to restrict its xenstore connection to $DOMID
level, using XS_RESTRICT, only implemented by oxenstored at present.

Let qemu-xen access
/local/domain/$LIBXL_TOOLSTACK_DOMID/device-model/$DOMID, as it is
required by QEMU to read/write the physmap. It doesn't contain any
information the guest doesn't already know. However be aware that it
still means relaxing an hypervisor/guest interface. See below.

Add a maximum limit of physmap entries to save, so that the guest cannot
DOS the toolstack.

In the case of qemu-traditional, the state node is used to issue
commands to the device model, so avoid to change permissions.


Information under
/local/domain/$LIBXL_TOOLSTACK_DOMID/device-model/$DOMID include:

- the device model status, which is updated by the device model before
the guest is unpaused, then ignored by both QEMU and libxl

- the guest physmap, which contains the memory layout of the guest. The
guest is already in possession of this information. A malicious guest
could modify the physmap entries causing QEMU to read wrong information
at restore time. QEMU would populate its XenPhysmap list with wrong
entries that wouldn't match its own device emulation addresses, leading
to a failure in restoring the domain. But it could not compromise the
security of the system because the addresses are only used for checks
against QEMU's addresses.


Is it dangerous to let the guest write values that later QEMU and libxl
read back? Let's dig a bit deeper into the code. QEMU and libxl use very
similar functions to parse the physmap entries. Let's start from libxl,
the function that does the job is
tools/libxl/libxl_dom.c:libxl__toolstack_save. It calls:

- libxl__xs_directory on /physmap
this is safe

- libxl__xs_read
if the path is wrong (nothing is there), it returns NULL, and it is
handled correctly

- strtoll on the values read
The values are under guest control but strtoll can handle bad formats.
strtoll always returns an unsigned long long. In case of errors, it can
return LLONG_MIN or LLONG_MAX.  libxl__toolstack_save doesn't check for
conversion errors, but it never uses the returned values anywhere.
That's OK.  tools/libxl/libxl_dom.c:libxl__toolstack_restore writes back
the values to xenstore.

So in case of errors:

1) libxl__toolstack_save could return early with an error, if the
xenstore paths are wrong (nothing is on xenstore)

2) libxl__toolstack_restore could write back to xenstore any unsigned
long long, including LLONG_MIN or LLONG_MAX

Either way we are fine.

>From the QEMU side, the code is very similar and uses strtoll to parse
the entries, that can deal with bad input values. The values are used to
avoid memory allocations at restore time for memory that has already
been allocated (video memory). If the values are wrong, QEMU will
attempt another memory allocation, failing because the maxmem limit has
been reached. Either way we are fine.


Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>


---
Changes in v5:
- improve commit message with security details

Changes in v4:
- add error message in case count > MAX_PHYSMAP_ENTRIES
- add a note to xenstore-paths.markdown about the possible change in
privilege level
- only change permissions if xsrestrict is supported

Changes in v3:
- use LIBXL_TOOLSTACK_DOMID instead of 0 in the commit message
- update commit message with more info on why it is safe
- add a limit on the number of physmap entries to save and restore

Changes in v2:
- fix permissions to actually do what intended
- use LIBXL_TOOLSTACK_DOMID instead of 0
---
 docs/misc/xenstore-paths.markdown |    7 +++++--
 tools/libxl/libxl_dm.c            |   12 +++++++++++-
 tools/libxl/libxl_dom.c           |    7 +++++++
 3 files changed, 23 insertions(+), 3 deletions(-)

diff --git a/docs/misc/xenstore-paths.markdown b/docs/misc/xenstore-paths.markdown
index 780f601..99e038b 100644
--- a/docs/misc/xenstore-paths.markdown
+++ b/docs/misc/xenstore-paths.markdown
@@ -306,10 +306,13 @@ A virtual network device backend. Described by
 
 A PV console backend. Described in [console.txt](console.txt)
 
-#### ~/device-model/$DOMID/* [INTERNAL]
+#### ~/device-model/$DOMID/* [w]
 
 Information relating to device models running in the domain. $DOMID is
-the target domain of the device model.
+the target domain of the device model. When the device model is running
+at the same privilege level of the guest domain, this path does not
+contain any sensitive information and becomes guest writeable. Otherwise
+it is INTERNAL.
 
 #### ~/libxl/disable_udev = ("1"|"0") []
 
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 455b66c..99e2553 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -1529,6 +1529,7 @@ void libxl__spawn_local_dm(libxl__egc *egc, libxl__dm_spawn_state *dmss)
     char **pass_stuff;
     const char *dm;
     int dm_state_fd = -1;
+    struct xs_permissions rwperm[2];
 
     if (libxl_defbool_val(b_info->device_model_stubdomain)) {
         abort();
@@ -1571,7 +1572,16 @@ void libxl__spawn_local_dm(libxl__egc *egc, libxl__dm_spawn_state *dmss)
     }
 
     path = libxl__device_model_xs_path(gc, LIBXL_TOOLSTACK_DOMID, domid, "");
-    xs_mkdir(ctx->xsh, XBT_NULL, path);
+    if (b_info->device_model_version == LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN &&
+        libxl__check_qemu_supported(gc, dm, "xsrestrict")) {
+        rwperm[0].id = LIBXL_TOOLSTACK_DOMID;
+        rwperm[0].perms = XS_PERM_NONE;
+        rwperm[1].id = domid;
+        rwperm[1].perms = XS_PERM_WRITE;
+        libxl__xs_mkdir(gc, XBT_NULL, path, rwperm, 2);
+    } else {
+        xs_mkdir(ctx->xsh, XBT_NULL, path);
+    }
 
     if (b_info->type == LIBXL_DOMAIN_TYPE_HVM &&
         b_info->device_model_version
diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index f408646..c8523da 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -1677,6 +1677,8 @@ static inline char *physmap_path(libxl__gc *gc, uint32_t dm_domid,
                                        phys_offset, node);
 }
 
+#define MAX_PHYSMAP_ENTRIES 12
+
 int libxl__toolstack_save(uint32_t domid, uint8_t **buf,
         uint32_t *len, void *dss_void)
 {
@@ -1698,6 +1700,11 @@ int libxl__toolstack_save(uint32_t domid, uint8_t **buf,
                 &num);
     count = num;
 
+    if (count > MAX_PHYSMAP_ENTRIES) {
+        LOG(ERROR, "Max physmap entries reached");
+        return -1;
+    }
+
     *len = sizeof(version) + sizeof(count);
     *buf = calloc(1, *len);
     ptr = *buf;
-- 
1.7.10.4

[PATCH v5 4/6] libxl: change xs path for QEMU

[Stefano Stabellini]

Change the QEMU xenstore watch path to
/local/domain/$LIBXL_TOOLSTACK_DOMID/device-model/$DOMID/$EMULATOR_ID.
Currently two emulator_ids are statically allocated: one for device models and
one for pv qemus.

Add a parameter to libxl__device_model_xs_path to distinguish the device
model from the pv backends provider.

Store the device model binary path under
/local/domain/$DOMID/device-model on xenstore, so that we can fetch it
later and retrieve the list of supported options from
/local/domain/$LIBXL_TOOLSTACK_DOMID/libxl/$device_model_binary,
introduce in the previous path.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

---

Changes in v4:
- update xenstore-paths.markdown

Changes in v3:
- use LIBXL_TOOLSTACK_DOMID instead of 0 in the commit message
- change xs path to include the emulator_id
---
 docs/misc/xenstore-paths.markdown |   16 +++++++++++-----
 tools/libxl/libxl.c               |    2 +-
 tools/libxl/libxl_create.c        |    3 +++
 tools/libxl/libxl_device.c        |    2 +-
 tools/libxl/libxl_dm.c            |   18 ++++++++++--------
 tools/libxl/libxl_dom.c           |   12 ++++++------
 tools/libxl/libxl_internal.c      |   19 +++++++++++++++----
 tools/libxl/libxl_internal.h      |    5 ++---
 tools/libxl/libxl_pci.c           |   14 +++++++-------
 9 files changed, 56 insertions(+), 35 deletions(-)

diff --git a/docs/misc/xenstore-paths.markdown b/docs/misc/xenstore-paths.markdown
index 99e038b..e6ed25f 100644
--- a/docs/misc/xenstore-paths.markdown
+++ b/docs/misc/xenstore-paths.markdown
@@ -115,6 +115,10 @@ The domain's own ID.
 The process ID of the device model associated with this domain, if it
 has one.
 
+#### ~/device-model = STRING [n,INTERNAL]
+
+The full device model binary path.
+
 #### ~/cpu/[0-9]+/availability = ("online"|"offline") [PV]
 
 One node for each virtual CPU up to the guest's configured
@@ -306,13 +310,15 @@ A virtual network device backend. Described by
 
 A PV console backend. Described in [console.txt](console.txt)
 
-#### ~/device-model/$DOMID/* [w]
+#### ~/device-model/$DOMID/$EMULATOR_ID/* [w]
 
 Information relating to device models running in the domain. $DOMID is
-the target domain of the device model. When the device model is running
-at the same privilege level of the guest domain, this path does not
-contain any sensitive information and becomes guest writeable. Otherwise
-it is INTERNAL.
+the target domain of the device model. $EMULATOR_ID indentifies a
+specific device model instance (multiple device model instances for the
+same domain are possible).  When the device model is running at the same
+privilege level of the guest domain, this path does not contain any
+sensitive information and becomes guest writeable. Otherwise
+it is INTERNAL
 
 #### ~/libxl/disable_udev = ("1"|"0") []
 
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 516713e..bca4c88 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -1035,7 +1035,7 @@ int libxl_domain_unpause(libxl_ctx *ctx, uint32_t domid)
     if (type == LIBXL_DOMAIN_TYPE_HVM) {
         uint32_t dm_domid = libxl_get_stubdom_id(ctx, domid);
 
-        path = libxl__device_model_xs_path(gc, dm_domid, domid, "/state");
+        path = libxl__device_model_xs_path(gc, false, dm_domid, domid, "/state");
         state = libxl__xs_read(gc, XBT_NULL, path);
         if (state != NULL && !strcmp(state, "paused")) {
             libxl__qemu_traditional_cmd(gc, domid, "continue");
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index a74b340..df946e2 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -1002,6 +1002,9 @@ static void domcreate_bootloader_done(libxl__egc *egc,
         return;
     }
 
+    libxl__xs_write(gc, XBT_NULL, GCSPRINTF("/local/domain/%u/device-model", domid),
+            "%s", libxl__domain_device_model(gc, info));
+
     /* consume bootloader outputs. state->pv_{kernel,ramdisk} have
      * been initialised by the bootloader already.
      */
diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
index 93bb41e..aadcd08 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -1190,7 +1190,7 @@ int libxl__wait_for_device_model_deprecated(libxl__gc *gc,
     char *path;
     uint32_t dm_domid = libxl_get_stubdom_id(CTX, domid);
 
-    path = libxl__device_model_xs_path(gc, dm_domid, domid, "/state");
+    path = libxl__device_model_xs_path(gc, false, dm_domid, domid, "/state");
     return libxl__xenstore_child_wait_deprecated(gc, domid,
                                      LIBXL_DEVICE_MODEL_START_TIMEOUT,
                                      "Device Model", path, state, spawning,
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 99e2553..8bd7b82 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -1272,9 +1272,9 @@ void libxl__spawn_stub_dm(libxl__egc *egc, libxl__stub_dm_spawn_state *sdss)
 retry_transaction:
     t = xs_transaction_start(ctx->xsh);
     xs_mkdir(ctx->xsh, t,
-             libxl__device_model_xs_path(gc, dm_domid, guest_domid, ""));
+             libxl__device_model_xs_path(gc, false, dm_domid, guest_domid, ""));
     xs_set_permissions(ctx->xsh, t,
-                       libxl__device_model_xs_path(gc, dm_domid,
+                       libxl__device_model_xs_path(gc, false, dm_domid,
                                                    guest_domid, ""),
                        perm, ARRAY_SIZE(perm));
     if (!xs_transaction_end(ctx->xsh, t, 0))
@@ -1435,7 +1435,7 @@ static void stubdom_pvqemu_cb(libxl__egc *egc,
     sdss->xswait.what = GCSPRINTF("Stubdom %u for %u startup",
                                   dm_domid, sdss->dm.guest_domid);
     sdss->xswait.path =
-        libxl__device_model_xs_path(gc, dm_domid, sdss->dm.guest_domid,
+        libxl__device_model_xs_path(gc, true, dm_domid, sdss->dm.guest_domid,
                                     "/state");
     sdss->xswait.timeout_ms = LIBXL_STUBDOM_START_TIMEOUT * 1000;
     sdss->xswait.callback = stubdom_xswait_cb;
@@ -1571,7 +1571,8 @@ void libxl__spawn_local_dm(libxl__egc *egc, libxl__dm_spawn_state *dmss)
         free(path);
     }
 
-    path = libxl__device_model_xs_path(gc, LIBXL_TOOLSTACK_DOMID, domid, "");
+    path = libxl__device_model_xs_path(gc, b_info->type == LIBXL_DOMAIN_TYPE_PV,
+            LIBXL_TOOLSTACK_DOMID, domid, "");
     if (b_info->device_model_version == LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN &&
         libxl__check_qemu_supported(gc, dm, "xsrestrict")) {
         rwperm[0].id = LIBXL_TOOLSTACK_DOMID;
@@ -1629,8 +1630,9 @@ retry_transaction:
         LIBXL__LOG(CTX, XTL_DEBUG, "  %s", *arg);
 
     spawn->what = GCSPRINTF("domain %d device model", domid);
-    spawn->xspath = libxl__device_model_xs_path(gc, LIBXL_TOOLSTACK_DOMID,
-                                                domid, "/state");
+    spawn->xspath = libxl__device_model_xs_path(gc,
+            b_info->type == LIBXL_DOMAIN_TYPE_PV, LIBXL_TOOLSTACK_DOMID,
+            domid, "/state");
     spawn->timeout_ms = LIBXL_DEVICE_MODEL_START_TIMEOUT * 1000;
     spawn->pidpath = GCSPRINTF("%s/image/device-model-pid", dom_path);
     spawn->midproc_cb = libxl__spawn_record_pid;
@@ -1763,7 +1765,7 @@ void libxl__spawn_qdisk_backend(libxl__egc *egc, libxl__dm_spawn_state *dmss)
     dmss->build_state->saved_state = 0;
 
     dmss->spawn.what = GCSPRINTF("domain %u Qdisk backend", domid);
-    dmss->spawn.xspath = GCSPRINTF("device-model/%u/state", domid);
+    dmss->spawn.xspath = libxl__device_model_xs_path(gc, true, 0, domid, "/state");
     dmss->spawn.timeout_ms = LIBXL_DEVICE_MODEL_START_TIMEOUT * 1000;
     /*
      * We cannot save Qemu pid anywhere in the xenstore guest dir,
@@ -1846,7 +1848,7 @@ out:
 
 int libxl__destroy_device_model(libxl__gc *gc, uint32_t domid)
 {
-    char *path = libxl__device_model_xs_path(gc, LIBXL_TOOLSTACK_DOMID,
+    char *path = libxl__device_model_xs_path(gc, false, LIBXL_TOOLSTACK_DOMID,
                                              domid, "");
     if (!xs_rm(CTX->xsh, XBT_NULL, path))
         LOG(ERROR, "xs_rm failed for %s", path);
diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index c8523da..a2bf33b 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -1016,7 +1016,7 @@ int libxl__qemu_traditional_cmd(libxl__gc *gc, uint32_t domid,
 {
     char *path = NULL;
     uint32_t dm_domid = libxl_get_stubdom_id(CTX, domid);
-    path = libxl__device_model_xs_path(gc, dm_domid, domid, "/command");
+    path = libxl__device_model_xs_path(gc, false, dm_domid, domid, "/command");
     return libxl__xs_write(gc, XBT_NULL, path, "%s", cmd);
 }
 
@@ -1034,7 +1034,7 @@ static inline char *restore_helper(libxl__gc *gc, uint32_t dm_domid,
                                    uint32_t domid,
                                    uint64_t phys_offset, char *node)
 {
-    return libxl__device_model_xs_path(gc, dm_domid, domid,
+    return libxl__device_model_xs_path(gc, false, dm_domid, domid,
                                        "/physmap/%"PRIx64"/%s",
                                        phys_offset, node);
 }
@@ -1146,9 +1146,9 @@ static void domain_suspend_switch_qemu_xen_traditional_logdirty
 
     if (!lds->cmd_path) {
         uint32_t dm_domid = libxl_get_stubdom_id(CTX, domid);
-        lds->cmd_path = libxl__device_model_xs_path(gc, dm_domid, domid,
+        lds->cmd_path = libxl__device_model_xs_path(gc, false, dm_domid, domid,
                                                     "/logdirty/cmd");
-        lds->ret_path = libxl__device_model_xs_path(gc, dm_domid, domid,
+        lds->ret_path = libxl__device_model_xs_path(gc, false, dm_domid, domid,
                                                     "/logdirty/ret");
     }
     lds->cmd = enable ? "enable" : "disable";
@@ -1672,7 +1672,7 @@ static inline char *physmap_path(libxl__gc *gc, uint32_t dm_domid,
                                  uint32_t domid,
                                  char *phys_offset, char *node)
 {
-    return libxl__device_model_xs_path(gc, dm_domid, domid,
+    return libxl__device_model_xs_path(gc, false, dm_domid, domid,
                                        "/physmap/%s/%s",
                                        phys_offset, node);
 }
@@ -1696,7 +1696,7 @@ int libxl__toolstack_save(uint32_t domid, uint8_t **buf,
     dm_domid = libxl_get_stubdom_id(CTX, domid);
 
     entries = libxl__xs_directory(gc, 0,
-                libxl__device_model_xs_path(gc, dm_domid, domid, "/physmap"),
+                libxl__device_model_xs_path(gc, false, dm_domid, domid, "/physmap"),
                 &num);
     count = num;
 
diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
index c2c67bd..5836742 100644
--- a/tools/libxl/libxl_internal.c
+++ b/tools/libxl/libxl_internal.c
@@ -567,14 +567,25 @@ void libxl__update_domain_configuration(libxl__gc *gc,
     dst->b_info.video_memkb = src->b_info.video_memkb;
 }
 
-char *libxl__device_model_xs_path(libxl__gc *gc, uint32_t dm_domid,
+char *libxl__device_model_xs_path(libxl__gc *gc, bool pvqemu, uint32_t dm_domid,
                                   uint32_t domid, const char *format,  ...)
 {
     char *s, *fmt;
     va_list ap;
-
-    fmt = GCSPRINTF("/local/domain/%u/device-model/%u%s", dm_domid,
-                    domid, format);
+    char *dm;
+    int emulator_id = 0;
+
+    dm = libxl__xs_read(gc, XBT_NULL, GCSPRINTF("/local/domain/%u/device-model", domid));
+    if (dm)
+        emulator_id = libxl__check_qemu_supported(gc, dm, "emulator_id");
+
+    if (!emulator_id) {
+        fmt = GCSPRINTF("/local/domain/%u/device-model/%u%s", dm_domid,
+                domid, format);
+    } else {
+        fmt = GCSPRINTF("/local/domain/%u/device-model/%u/%u%s", dm_domid,
+                domid, pvqemu ? QEMU_XEN_PV_ID : QEMU_XEN_DEVICE_MODEL_ID, format);
+    }
 
     va_start(ap, format);
     s = libxl__vsprintf(gc, fmt, ap);
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index b4bae2f..e15cdc7 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1830,9 +1830,8 @@ _hidden libxl__json_object *libxl__json_parse(libxl__gc *gc_opt, const char *s);
 _hidden int libxl__device_model_version_running(libxl__gc *gc, uint32_t domid);
   /* Return the system-wide default device model */
 _hidden libxl_device_model_version libxl__default_device_model(libxl__gc *gc);
-_hidden char *libxl__device_model_xs_path(libxl__gc *gc, uint32_t dm_domid,
-                                          uint32_t domid,
-                                          const char *format, ...) PRINTF_ATTRIBUTE(4, 5);
+_hidden char *libxl__device_model_xs_path(libxl__gc *gc, bool pvqemu, uint32_t dm_domid,
+                                  uint32_t domid, const char *format,  ...);
 
 /* Check how executes hotplug script currently */
 int libxl__hotplug_settings(libxl__gc *gc, xs_transaction_t t);
diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
index e0743f8..07f6209 100644
--- a/tools/libxl/libxl_pci.c
+++ b/tools/libxl/libxl_pci.c
@@ -853,9 +853,9 @@ static int qemu_pci_add_xenstore(libxl__gc *gc, uint32_t domid,
     uint32_t dm_domid;
 
     dm_domid = libxl_get_stubdom_id(CTX, domid);
-    path = libxl__device_model_xs_path(gc, dm_domid, domid, "/state");
+    path = libxl__device_model_xs_path(gc, false, dm_domid, domid, "/state");
     state = libxl__xs_read(gc, XBT_NULL, path);
-    path = libxl__device_model_xs_path(gc, dm_domid, domid, "/parameter");
+    path = libxl__device_model_xs_path(gc, false, dm_domid, domid, "/parameter");
     if (pcidev->vdevfn) {
         libxl__xs_write(gc, XBT_NULL, path, PCI_BDF_VDEVFN","PCI_OPTIONS,
                         pcidev->domain, pcidev->bus, pcidev->dev,
@@ -870,9 +870,9 @@ static int qemu_pci_add_xenstore(libxl__gc *gc, uint32_t domid,
     libxl__qemu_traditional_cmd(gc, domid, "pci-ins");
     rc = libxl__wait_for_device_model_deprecated(gc, domid, NULL, NULL,
                                       pci_ins_check, state);
-    path = libxl__device_model_xs_path(gc, dm_domid, domid, "/parameter");
+    path = libxl__device_model_xs_path(gc, false, dm_domid, domid, "/parameter");
     vdevfn = libxl__xs_read(gc, XBT_NULL, path);
-    path = libxl__device_model_xs_path(gc, dm_domid, domid, "/state");
+    path = libxl__device_model_xs_path(gc, false, dm_domid, domid, "/state");
     if ( rc < 0 )
         LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
                    "qemu refused to add device: %s", vdevfn);
@@ -1178,9 +1178,9 @@ static int qemu_pci_remove_xenstore(libxl__gc *gc, uint32_t domid,
 
     dm_domid = libxl_get_stubdom_id(CTX, domid);
 
-    path = libxl__device_model_xs_path(gc, dm_domid, domid, "/state");
+    path = libxl__device_model_xs_path(gc, false, dm_domid, domid, "/state");
     state = libxl__xs_read(gc, XBT_NULL, path);
-    path = libxl__device_model_xs_path(gc, dm_domid, domid, "/parameter");
+    path = libxl__device_model_xs_path(gc, false, dm_domid, domid, "/parameter");
     libxl__xs_write(gc, XBT_NULL, path, PCI_BDF, pcidev->domain,
                     pcidev->bus, pcidev->dev, pcidev->func);
 
@@ -1198,7 +1198,7 @@ static int qemu_pci_remove_xenstore(libxl__gc *gc, uint32_t domid,
             return ERROR_FAIL;
         }
     }
-    path = libxl__device_model_xs_path(gc, dm_domid, domid, "/state");
+    path = libxl__device_model_xs_path(gc, false, dm_domid, domid, "/state");
     xs_write(ctx->xsh, XBT_NULL, path, state, strlen(state));
 
     return 0;
-- 
1.7.10.4

[PATCH v5 5/6] libxl: change qdisk-backend-pid path on xenstore

[Stefano Stabellini]

Change the qdisk-backend-pid path on xenstore from
libxl/$DOMID/qdisk-backend-pid to /local/domain/$DOMID/image/pvqemu-pid to be
more similar to the device-model path.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

---

Changes in v4:
- update xenstore-paths.markdown
---
 docs/misc/xenstore-paths.markdown |    9 +++++----
 tools/libxl/libxl_dm.c            |    4 ++--
 2 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/docs/misc/xenstore-paths.markdown b/docs/misc/xenstore-paths.markdown
index e6ed25f..7f87f74 100644
--- a/docs/misc/xenstore-paths.markdown
+++ b/docs/misc/xenstore-paths.markdown
@@ -115,6 +115,11 @@ The domain's own ID.
 The process ID of the device model associated with this domain, if it
 has one.
 
+#### ~/image/pvqemu-pid = INTEGER [INTERNAL]
+
+The process ID of the userspace PV backends daemon associated with this
+domain, if it has one.
+
 #### ~/device-model = STRING [n,INTERNAL]
 
 The full device model binary path.
@@ -360,10 +365,6 @@ A domain writable path. Available for arbitrary domain use.
 
 Contains the status of the device models running on the domain.
 
-#### ~/libxl/$DOMID/qdisk-backend-pid [w]
-
-Contains the PIDs of the device models running on the domain.
-
 ## Virtual Machine Paths
 
 The /vm/$UUID namespace is used by toolstacks to store various
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 8bd7b82..e4d3b1c 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -1772,7 +1772,7 @@ void libxl__spawn_qdisk_backend(libxl__egc *egc, libxl__dm_spawn_state *dmss)
      * because we will call this from unprivileged driver domains,
      * so save it in the current domain libxl private dir.
      */
-    dmss->spawn.pidpath = GCSPRINTF("libxl/%u/qdisk-backend-pid", domid);
+    dmss->spawn.pidpath = GCSPRINTF("/local/domain/%d/image/pvqemu-pid", domid);
     dmss->spawn.midproc_cb = libxl__spawn_record_pid;
     dmss->spawn.confirm_cb = device_model_confirm;
     dmss->spawn.failure_cb = device_model_startup_failed;
@@ -1832,7 +1832,7 @@ int libxl__destroy_qdisk_backend(libxl__gc *gc, uint32_t domid)
     char *pid_path;
     int rc;
 
-    pid_path = GCSPRINTF("libxl/%u/qdisk-backend-pid", domid);
+    pid_path = GCSPRINTF("/local/domain/%d/image/pvqemu-pid", domid);
 
     rc = kill_device_model(gc, pid_path);
     if (rc)
-- 
1.7.10.4

[PATCH v5 6/6] libxl: spawns two QEMUs for HVM guests

[Stefano Stabellini]

Starts a second QEMU to provide PV backends in userspace to HVM guests.
Use both dcs->dmss.pvqemu and dcs->dmss.dm to keep track of the starting
QEMUs. Introduce two new fields to struct libxl__dm_spawn_state: dcs to
store the pointer to libxl__domain_create_state, and rc to store the
return code.

Only proceed when both QEMUs have started.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

---
Changes in v3:
- use dcs->dmss.pvqemu to spawn the second QEMU
- keep track of the rc of both QEMUs before proceeding
---
 tools/libxl/libxl_create.c   |   50 ++++++++++++++++++++++++++++++------------
 tools/libxl/libxl_dm.c       |    9 +++++++-
 tools/libxl/libxl_internal.h |    3 +++
 3 files changed, 47 insertions(+), 15 deletions(-)

diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index df946e2..94dd52c 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -751,6 +751,9 @@ static int store_libxl_entry(libxl__gc *gc, uint32_t domid,
 static void domcreate_devmodel_started(libxl__egc *egc,
                                        libxl__dm_spawn_state *dmss,
                                        int rc);
+static void domcreate_devmodel_callback(libxl__egc *egc,
+                                       libxl__dm_spawn_state *dmss,
+                                       int ret);
 static void domcreate_bootloader_console_available(libxl__egc *egc,
                                                    libxl__bootloader_state *bl);
 static void domcreate_bootloader_done(libxl__egc *egc,
@@ -1016,8 +1019,17 @@ static void domcreate_bootloader_done(libxl__egc *egc,
     dcs->dmss.dm.spawn.ao = ao;
     dcs->dmss.dm.guest_config = dcs->guest_config;
     dcs->dmss.dm.build_state = &dcs->build_state;
-    dcs->dmss.dm.callback = domcreate_devmodel_started;
-    dcs->dmss.callback = domcreate_devmodel_started;
+    dcs->dmss.dm.callback = domcreate_devmodel_callback;
+    dcs->dmss.dm.dcs = dcs;
+    dcs->dmss.callback = domcreate_devmodel_callback;
+
+    if (info->type == LIBXL_DOMAIN_TYPE_HVM) {
+        dcs->dmss.pvqemu.guest_domid = domid;
+        dcs->dmss.pvqemu.spawn.ao = ao;
+        dcs->dmss.pvqemu.callback = domcreate_devmodel_callback;
+        dcs->dmss.pvqemu.dcs = dcs;
+        libxl__spawn_qdisk_backend(egc, &dcs->dmss.pvqemu);
+    }
 
     if ( restore_fd < 0 ) {
         rc = libxl__domain_build(gc, d_config, domid, state);
@@ -1347,20 +1359,13 @@ static void domcreate_devmodel_started(libxl__egc *egc,
                                        libxl__dm_spawn_state *dmss,
                                        int ret)
 {
-    libxl__domain_create_state *dcs = CONTAINER_OF(dmss, *dcs, dmss.dm);
+    libxl__domain_create_state *dcs = dmss->dcs;
     STATE_AO_GC(dmss->spawn.ao);
-    libxl_ctx *ctx = CTX;
     int domid = dcs->guest_domid;
 
     /* convenience aliases */
     libxl_domain_config *const d_config = dcs->guest_config;
 
-    if (ret) {
-        LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
-                   "device model did not start: %d", ret);
-        goto error_out;
-    }
-
     if (dcs->dmss.dm.guest_domid) {
         if (d_config->b_info.device_model_version
             == LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN) {
@@ -1379,11 +1384,28 @@ static void domcreate_devmodel_started(libxl__egc *egc,
     }
 
     domcreate_attach_vtpms(egc, &dcs->multidev, 0);
-    return;
+}
 
-error_out:
-    assert(ret);
-    domcreate_complete(egc, dcs, ret);
+static void domcreate_devmodel_callback(libxl__egc *egc,
+                                       libxl__dm_spawn_state *dmss,
+                                       int ret)
+{
+    libxl__domain_create_state *dcs = dmss->dcs;
+    STATE_AO_GC(dmss->spawn.ao);
+    int worst_rc = 0;
+
+    dmss->rc = ret;
+
+    if (libxl__spawn_inuse(&dcs->dmss.dm.spawn) ||
+        libxl__spawn_inuse(&dcs->dmss.pvqemu.spawn))
+           return;
+    worst_rc = (dcs->dmss.dm.rc < dcs->dmss.pvqemu.rc) ? dcs->dmss.dm.rc : dcs->dmss.pvqemu.rc;
+
+    /* all qemus have completed */
+    if (worst_rc)
+        domcreate_complete(egc, dcs, worst_rc);
+    else
+        domcreate_devmodel_started(egc, dmss, 0);
 }
 
 static void domcreate_attach_vtpms(libxl__egc *egc,
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index e4d3b1c..986fc84 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -1848,13 +1848,20 @@ out:
 
 int libxl__destroy_device_model(libxl__gc *gc, uint32_t domid)
 {
+    int rc;
     char *path = libxl__device_model_xs_path(gc, false, LIBXL_TOOLSTACK_DOMID,
                                              domid, "");
     if (!xs_rm(CTX->xsh, XBT_NULL, path))
         LOG(ERROR, "xs_rm failed for %s", path);
+
+    kill_device_model(gc,
+            GCSPRINTF("/local/domain/%d/image/pvqemu-pid", domid));
+
     /* We should try to destroy the device model anyway. */
-    return kill_device_model(gc,
+    rc = kill_device_model(gc,
                 GCSPRINTF("/local/domain/%d/image/device-model-pid", domid));
+
+    return rc;
 }
 
 int libxl__need_xenpv_qemu(libxl__gc *gc,
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index e15cdc7..977ccca 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -3062,9 +3062,12 @@ typedef struct libxl__dm_spawn_state libxl__dm_spawn_state;
 typedef void libxl__dm_spawn_cb(libxl__egc *egc, libxl__dm_spawn_state*,
                                 int rc /* if !0, error was logged */);
 
+typedef struct libxl__domain_create_state libxl__domain_create_state;
 struct libxl__dm_spawn_state {
     /* mixed - spawn.ao must be initialised by user; rest is private: */
     libxl__spawn_state spawn;
+    int rc;
+    libxl__domain_create_state *dcs;
     /* filled in by user, must remain valid: */
     uint32_t guest_domid; /* domain being served */
     libxl_domain_config *guest_config;
-- 
1.7.10.4

Re: [PATCH v5 0/6] libxl: xs_restrict QEMU

[Fabio Fantoni]

Il 23/07/2015 19:26, Stefano Stabellini ha scritto:
> Hi all,
>
> this patch series changes libxl to start QEMU as device model with the
> new xsrestrict option (http://marc.info/?l=xen-devel&m=143341692707358).
> It also starts a second QEMU to provide PV backends in userspace (qdisk)
> to HVM guests.

Hi, I'm interested to test this serie.
xen patch "run QEMU as non-root" and qemu patch linked above are the 
only prerequisite or other are needed?
I saw that second patch is marked as [WIP], is it usable or I must wait 
to have it complete before test this serie?

Thanks for any reply and sorry for my bad english.

>
>
> Changes in v5:
> - improve commit messages with security details
>
> Changes in v4:
> - update xenstore-paths.markdown
> - add error message in case count > MAX_PHYSMAP_ENTRIES
> - add a note to xenstore-paths.markdown about the possible change in
> privilege level
> - only change permissions if xsrestrict is supported
>
> Changes in v3:
> - use LIBXL_TOOLSTACK_DOMID instead of 0 in the commit message
> - update commit message with more info on why it is safe
> - add a limit on the number of physmap entries to save and restore
> - add emulator_ids
> - mark patch #3 as WIP
> - use LIBXL_TOOLSTACK_DOMID instead of 0 in the commit message
> - change xs path to include the emulator_id
> - change qdisk-backend-pid path on xenstore
> - use dcs->dmss.pvqemu to spawn the second QEMU
> - keep track of the rc of both QEMUs before proceeding
>
>
> Stefano Stabellini (6):
>        libxl: do not add a vkb backend to hvm guests
>        [WIP] libxl: xsrestrict QEMU
>        libxl: allow /local/domain/$LIBXL_TOOLSTACK_DOMID/device-model/$DOMID to be written by $DOMID
>        libxl: change xs path for QEMU
>        libxl: change qdisk-backend-pid path on xenstore
>        libxl: spawns two QEMUs for HVM guests
>
>   docs/misc/xenstore-paths.markdown |   30 ++++++++--
>   tools/libxl/libxl.c               |    2 +-
>   tools/libxl/libxl_create.c        |   58 +++++++++++++------
>   tools/libxl/libxl_device.c        |    2 +-
>   tools/libxl/libxl_dm.c            |  115 +++++++++++++++++++++++++++++++++----
>   tools/libxl/libxl_dom.c           |   19 ++++--
>   tools/libxl/libxl_internal.c      |   19 ++++--
>   tools/libxl/libxl_internal.h      |   15 ++++-
>   tools/libxl/libxl_pci.c           |   14 ++---
>   tools/libxl/libxl_utils.c         |   10 ++++
>   10 files changed, 225 insertions(+), 59 deletions(-)
>
> Cheers,
>
> Stefano
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH v5 0/6] libxl: xs_restrict QEMU

[Stefano Stabellini]

On Mon, 27 Jul 2015, Fabio Fantoni wrote:
> Il 23/07/2015 19:26, Stefano Stabellini ha scritto:
> > Hi all,
> > 
> > this patch series changes libxl to start QEMU as device model with the
> > new xsrestrict option (http://marc.info/?l=xen-devel&m=143341692707358).
> > It also starts a second QEMU to provide PV backends in userspace (qdisk)
> > to HVM guests.
> 
> Hi, I'm interested to test this serie.
> xen patch "run QEMU as non-root" and qemu patch linked above are the only
> prerequisite or other are needed?

Yes and thank you for testing!


> I saw that second patch is marked as [WIP], is it usable or I must wait to
> have it complete before test this serie?

It is usable, but it is not recommended why to do it in libxl.


> Thanks for any reply and sorry for my bad english.
> 
> > 
> > 
> > Changes in v5:
> > - improve commit messages with security details
> > 
> > Changes in v4:
> > - update xenstore-paths.markdown
> > - add error message in case count > MAX_PHYSMAP_ENTRIES
> > - add a note to xenstore-paths.markdown about the possible change in
> > privilege level
> > - only change permissions if xsrestrict is supported
> > 
> > Changes in v3:
> > - use LIBXL_TOOLSTACK_DOMID instead of 0 in the commit message
> > - update commit message with more info on why it is safe
> > - add a limit on the number of physmap entries to save and restore
> > - add emulator_ids
> > - mark patch #3 as WIP
> > - use LIBXL_TOOLSTACK_DOMID instead of 0 in the commit message
> > - change xs path to include the emulator_id
> > - change qdisk-backend-pid path on xenstore
> > - use dcs->dmss.pvqemu to spawn the second QEMU
> > - keep track of the rc of both QEMUs before proceeding
> > 
> > 
> > Stefano Stabellini (6):
> >        libxl: do not add a vkb backend to hvm guests
> >        [WIP] libxl: xsrestrict QEMU
> >        libxl: allow /local/domain/$LIBXL_TOOLSTACK_DOMID/device-model/$DOMID
> > to be written by $DOMID
> >        libxl: change xs path for QEMU
> >        libxl: change qdisk-backend-pid path on xenstore
> >        libxl: spawns two QEMUs for HVM guests
> > 
> >   docs/misc/xenstore-paths.markdown |   30 ++++++++--
> >   tools/libxl/libxl.c               |    2 +-
> >   tools/libxl/libxl_create.c        |   58 +++++++++++++------
> >   tools/libxl/libxl_device.c        |    2 +-
> >   tools/libxl/libxl_dm.c            |  115
> > +++++++++++++++++++++++++++++++++----
> >   tools/libxl/libxl_dom.c           |   19 ++++--
> >   tools/libxl/libxl_internal.c      |   19 ++++--
> >   tools/libxl/libxl_internal.h      |   15 ++++-
> >   tools/libxl/libxl_pci.c           |   14 ++---
> >   tools/libxl/libxl_utils.c         |   10 ++++
> >   10 files changed, 225 insertions(+), 59 deletions(-)
> > 
> > Cheers,
> > 
> > Stefano
> > 
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xen.org
> > http://lists.xen.org/xen-devel
> 

Re: [PATCH v5 0/6] libxl: xs_restrict QEMU

[Fabio Fantoni]

Il 29/07/2015 11:21, Stefano Stabellini ha scritto:
> On Mon, 27 Jul 2015, Fabio Fantoni wrote:
>> Il 23/07/2015 19:26, Stefano Stabellini ha scritto:
>>> Hi all,
>>>
>>> this patch series changes libxl to start QEMU as device model with the
>>> new xsrestrict option (http://marc.info/?l=xen-devel&m=143341692707358).
>>> It also starts a second QEMU to provide PV backends in userspace (qdisk)
>>> to HVM guests.
>> Hi, I'm interested to test this serie.
>> xen patch "run QEMU as non-root" and qemu patch linked above are the only
>> prerequisite or other are needed?
> Yes and thank you for testing!
>
>
>> I saw that second patch is marked as [WIP], is it usable or I must wait to
>> have it complete before test this serie?
> It is usable, but it is not recommended why to do it in libxl.

Thanks for reply, what do you mean about "but it is not recommended why 
to do it in libxl"?

>
>
>> Thanks for any reply and sorry for my bad english.
>>
>>>
>>> Changes in v5:
>>> - improve commit messages with security details
>>>
>>> Changes in v4:
>>> - update xenstore-paths.markdown
>>> - add error message in case count > MAX_PHYSMAP_ENTRIES
>>> - add a note to xenstore-paths.markdown about the possible change in
>>> privilege level
>>> - only change permissions if xsrestrict is supported
>>>
>>> Changes in v3:
>>> - use LIBXL_TOOLSTACK_DOMID instead of 0 in the commit message
>>> - update commit message with more info on why it is safe
>>> - add a limit on the number of physmap entries to save and restore
>>> - add emulator_ids
>>> - mark patch #3 as WIP
>>> - use LIBXL_TOOLSTACK_DOMID instead of 0 in the commit message
>>> - change xs path to include the emulator_id
>>> - change qdisk-backend-pid path on xenstore
>>> - use dcs->dmss.pvqemu to spawn the second QEMU
>>> - keep track of the rc of both QEMUs before proceeding
>>>
>>>
>>> Stefano Stabellini (6):
>>>         libxl: do not add a vkb backend to hvm guests
>>>         [WIP] libxl: xsrestrict QEMU
>>>         libxl: allow /local/domain/$LIBXL_TOOLSTACK_DOMID/device-model/$DOMID
>>> to be written by $DOMID
>>>         libxl: change xs path for QEMU
>>>         libxl: change qdisk-backend-pid path on xenstore
>>>         libxl: spawns two QEMUs for HVM guests
>>>
>>>    docs/misc/xenstore-paths.markdown |   30 ++++++++--
>>>    tools/libxl/libxl.c               |    2 +-
>>>    tools/libxl/libxl_create.c        |   58 +++++++++++++------
>>>    tools/libxl/libxl_device.c        |    2 +-
>>>    tools/libxl/libxl_dm.c            |  115
>>> +++++++++++++++++++++++++++++++++----
>>>    tools/libxl/libxl_dom.c           |   19 ++++--
>>>    tools/libxl/libxl_internal.c      |   19 ++++--
>>>    tools/libxl/libxl_internal.h      |   15 ++++-
>>>    tools/libxl/libxl_pci.c           |   14 ++---
>>>    tools/libxl/libxl_utils.c         |   10 ++++
>>>    10 files changed, 225 insertions(+), 59 deletions(-)
>>>
>>> Cheers,
>>>
>>> Stefano
>>>
>>> _______________________________________________
>>> Xen-devel mailing list
>>> Xen-devel@lists.xen.org
>>> http://lists.xen.org/xen-devel

Re: [PATCH v5 0/6] libxl: xs_restrict QEMU

[Stefano Stabellini]

On Wed, 29 Jul 2015, Fabio Fantoni wrote:
> Il 29/07/2015 11:21, Stefano Stabellini ha scritto:
> > On Mon, 27 Jul 2015, Fabio Fantoni wrote:
> > > Il 23/07/2015 19:26, Stefano Stabellini ha scritto:
> > > > Hi all,
> > > > 
> > > > this patch series changes libxl to start QEMU as device model with the
> > > > new xsrestrict option (http://marc.info/?l=xen-devel&m=143341692707358).
> > > > It also starts a second QEMU to provide PV backends in userspace (qdisk)
> > > > to HVM guests.
> > > Hi, I'm interested to test this serie.
> > > xen patch "run QEMU as non-root" and qemu patch linked above are the only
> > > prerequisite or other are needed?
> > Yes and thank you for testing!
> > 
> > 
> > > I saw that second patch is marked as [WIP], is it usable or I must wait to
> > > have it complete before test this serie?
> > It is usable, but it is not recommended why to do it in libxl.
> 
> Thanks for reply, what do you mean about "but it is not recommended why to do
> it in libxl"?

Sorry, it was very badly written.

I meant that the patch spawns a QEMU instance using "fork", which is not
the way it is supposed to be done in libxl. There are helper functions
for that.


> > 
> > > Thanks for any reply and sorry for my bad english.
> > > 
> > > > 
> > > > Changes in v5:
> > > > - improve commit messages with security details
> > > > 
> > > > Changes in v4:
> > > > - update xenstore-paths.markdown
> > > > - add error message in case count > MAX_PHYSMAP_ENTRIES
> > > > - add a note to xenstore-paths.markdown about the possible change in
> > > > privilege level
> > > > - only change permissions if xsrestrict is supported
> > > > 
> > > > Changes in v3:
> > > > - use LIBXL_TOOLSTACK_DOMID instead of 0 in the commit message
> > > > - update commit message with more info on why it is safe
> > > > - add a limit on the number of physmap entries to save and restore
> > > > - add emulator_ids
> > > > - mark patch #3 as WIP
> > > > - use LIBXL_TOOLSTACK_DOMID instead of 0 in the commit message
> > > > - change xs path to include the emulator_id
> > > > - change qdisk-backend-pid path on xenstore
> > > > - use dcs->dmss.pvqemu to spawn the second QEMU
> > > > - keep track of the rc of both QEMUs before proceeding
> > > > 
> > > > 
> > > > Stefano Stabellini (6):
> > > >         libxl: do not add a vkb backend to hvm guests
> > > >         [WIP] libxl: xsrestrict QEMU
> > > >         libxl: allow
> > > > /local/domain/$LIBXL_TOOLSTACK_DOMID/device-model/$DOMID
> > > > to be written by $DOMID
> > > >         libxl: change xs path for QEMU
> > > >         libxl: change qdisk-backend-pid path on xenstore
> > > >         libxl: spawns two QEMUs for HVM guests
> > > > 
> > > >    docs/misc/xenstore-paths.markdown |   30 ++++++++--
> > > >    tools/libxl/libxl.c               |    2 +-
> > > >    tools/libxl/libxl_create.c        |   58 +++++++++++++------
> > > >    tools/libxl/libxl_device.c        |    2 +-
> > > >    tools/libxl/libxl_dm.c            |  115
> > > > +++++++++++++++++++++++++++++++++----
> > > >    tools/libxl/libxl_dom.c           |   19 ++++--
> > > >    tools/libxl/libxl_internal.c      |   19 ++++--
> > > >    tools/libxl/libxl_internal.h      |   15 ++++-
> > > >    tools/libxl/libxl_pci.c           |   14 ++---
> > > >    tools/libxl/libxl_utils.c         |   10 ++++
> > > >    10 files changed, 225 insertions(+), 59 deletions(-)
> > > > 
> > > > Cheers,
> > > > 
> > > > Stefano
> > > > 
> > > > _______________________________________________
> > > > Xen-devel mailing list
> > > > Xen-devel@lists.xen.org
> > > > http://lists.xen.org/xen-devel
>