All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v6 00/18] Add support for qemu-xen runnning in a Linux-based stubdomain
@ 2020-05-18  1:13 Jason Andryuk
  2020-05-18  1:13 ` [PATCH v6 01/18] Document ioemu MiniOS stubdomain protocol Jason Andryuk
                   ` (18 more replies)
  0 siblings, 19 replies; 28+ messages in thread
From: Jason Andryuk @ 2020-05-18  1:13 UTC (permalink / raw)
  To: xen-devel
  Cc: Stefano Stabellini, Julien Grall, Wei Liu, Jason Andryuk,
	Andrew Cooper, Ian Jackson, George Dunlap, Jan Beulich,
	Ian Jackson, Anthony PERARD, Samuel Thibault, Daniel De Graaf

Hi,

In coordination with Marek, I'm making a submission of his patches for Linux
stubdomain device-model support.  I made a few of my own additions, but Marek
did the heavy lifting.  Thank you, Marek.

Below is mostly the v5 cover leter with a few additions.

General idea is to allow freely set device_model_version and
device_model_stubdomain_override and choose the right options based on this
choice.  Also, allow to specific path to stubdomain kernel/ramdisk, for greater
flexibility.

First two patches add documentation about expected toolstack-stubdomain-qemu
interface, both for MiniOS stubdomain and Linux stubdomain.

Initial version has no QMP support - in initial patches it is completely
disabled, which means no suspend/restore and no PCI passthrough.

Later patches add QMP over libvchan connection support. The actual connection
is made in a separate process. As discussed on Xen Summit 2019, this allows to
apply some basic checks and/or filtering (not part of this series), to limit
libxl exposure for potentially malicious stubdomain.

Jason's additions ensure the qmp-proxy (vchan-socket-proxy) processes and
sockets are cleaned up and add some documentation.

The actual stubdomain implementation is here:

   https://github.com/jandryuk/qubes-vmm-xen-stubdom-linux
   (branch initramfs-tools, tag for-upstream-v6)

See readme there for build instructions.  Marek's version requires dracut.  I
have hacked up a version usable with initramfs-tools.

The v6 version is needed to be compatible with these changes in the v6 posting:

 - Mini-OS stubdoms use dmargs xenstore key as a string.  Linux stubdoms
   use dm-argv as a directory for numbered entries.

 - The hardcoded "fd:3" for the restore image is replaced with the
   placehodler string $STUBDOM_RESTORE_INCOMING_ARG.  The stubdom
   initscript needs to replace that with a an "fd:$FD" string to the
   hooked up console 2.

Few comments/questions about the stubdomain code:

1. There are extra patches for qemu that are necessary to run it in stubdomain.
While it is desirable to upstream them, I think it can be done after merging
libxl part. Stubdomain's qemu build will in most cases be separate anyway, to
limit qemu's dependencies (so the stubdomain size).

2. By default Linux hvc-xen console frontend is unreliable for data transfer
(qemu state save/restore) - it drops data sent faster than client is reading
it. To fix it, console device needs to be switched into raw mode (`stty raw
/dev/hvc1`). Especially for restoring qemu state it is tricky, as it would need
to be done before opening the device, but stty (obviously) needs to open the
device first. To solve this problem, for now the repository contains kernel
patch which changes the default for all hvc consoles. Again, this isn't
practical problem, as the kernel for stubdomain is built separately. But it
would be nice to have something working with vanilla kernel. I see those
options:
  - convert it to kernel cmdline parameter (hvc_console_raw=1 ?)
  - use channels instead of consoles (and on the kernel side change the default
    to "raw" only for channels); while in theory better design, libxl part will
    be more complex, as channels can be connected to sockets but not files, so
    libxl would need to read/write to it exactly when qemu write/read the data,
    not before/after as it is done now

Remaining parts for eliminating dom0's instance of qemu:
 - do not force QDISK backend for CDROM
 - multiple consoles support in xenconsoled

Changes in v2:
 - apply review comments by Jason Andryuk
Changes in v3:
 - rework qemu arguments handling (separate xenstore keys, instead of \x1b separator)
 - add QMP over libvchan, instead of console
 - add protocol documentation
 - a lot of minor changes, see individual patches for full changes list
 - split xenconsoled patches into separate series
Changes in v4:
 - extract vchan connection into a separate process
 - rebase on master
 - various fixes
Changes in v5:
 - Marek: apply review comments from Jason Andryuk
 - Jason: Clean up qmp-proxy processes and sockets
Changes in v6:
 - Squash vchan-proxy kill and socket cleanup into "libxl: use vchan for
   QMP access with Linux stubdomain".
 - Use dm-argv as the xenstore directory for the QEMU arguments.
 - Use $STUBDOM_RESTORE_INCOMING_ARG as a placeholder instead of
   hardcoding "fd:3".
 - Comment to re-run autotools.
 - Add Acked-by from Ian Jackson where approriate.

Eric Shelton (1):
  libxl: Handle Linux stubdomain specific QEMU options.

Jason Andryuk (3):
  libxl: Refactor kill_device_model to libxl__kill_xs_path
  docs: Add device-model-domid to xenstore-paths
  libxl: Check stubdomain kernel & ramdisk presence

Marek Marczykowski-Górecki (14):
  Document ioemu MiniOS stubdomain protocol
  Document ioemu Linux stubdomain protocol
  libxl: fix qemu-trad cmdline for no sdl/vnc case
  libxl: Allow running qemu-xen in stubdomain
  libxl: write qemu arguments into separate xenstore keys
  xl: add stubdomain related options to xl config parser
  tools/libvchan: notify server when client is connected
  libxl: add save/restore support for qemu-xen in stubdomain
  tools: add missing libxenvchan cflags
  tools: add simple vchan-socket-proxy
  libxl: use vchan for QMP access with Linux stubdomain
  libxl: require qemu in dom0 for multiple stubdomain consoles
  libxl: ignore emulated IDE disks beyond the first 4
  libxl: consider also qemu in stubdomain in libxl__dm_active check

 .gitignore                          |   1 +
 configure                           |  14 +-
 docs/configure                      |  14 +-
 docs/man/xl.cfg.5.pod.in            |  27 +-
 docs/misc/stubdom.txt               | 105 ++++++
 docs/misc/xenstore-paths.pandoc     |   5 +
 stubdom/configure                   |  14 +-
 tools/Rules.mk                      |   2 +-
 tools/config.h.in                   |   3 +
 tools/configure                     |  46 ++-
 tools/configure.ac                  |   9 +
 tools/libvchan/Makefile             |   8 +-
 tools/libvchan/init.c               |   3 +
 tools/libvchan/vchan-socket-proxy.c | 478 ++++++++++++++++++++++++++
 tools/libxl/libxl_aoutils.c         |  32 ++
 tools/libxl/libxl_create.c          |  46 ++-
 tools/libxl/libxl_dm.c              | 506 ++++++++++++++++++++++------
 tools/libxl/libxl_domain.c          |  10 +
 tools/libxl/libxl_internal.h        |  22 ++
 tools/libxl/libxl_mem.c             |   6 +-
 tools/libxl/libxl_qmp.c             |  27 +-
 tools/libxl/libxl_types.idl         |   3 +
 tools/xl/xl_parse.c                 |   7 +
 23 files changed, 1209 insertions(+), 179 deletions(-)
 create mode 100644 tools/libvchan/vchan-socket-proxy.c

-- 
2.25.1



^ permalink raw reply	[flat|nested] 28+ messages in thread

* [PATCH v6 01/18] Document ioemu MiniOS stubdomain protocol
  2020-05-18  1:13 [PATCH v6 00/18] Add support for qemu-xen runnning in a Linux-based stubdomain Jason Andryuk
@ 2020-05-18  1:13 ` Jason Andryuk
  2020-05-18  1:13 ` [PATCH v6 02/18] Document ioemu Linux " Jason Andryuk
                   ` (17 subsequent siblings)
  18 siblings, 0 replies; 28+ messages in thread
From: Jason Andryuk @ 2020-05-18  1:13 UTC (permalink / raw)
  To: xen-devel
  Cc: Stefano Stabellini, Julien Grall, Wei Liu, Jason Andryuk,
	Andrew Cooper, Ian Jackson, Marek Marczykowski-Górecki,
	George Dunlap, Jan Beulich, Ian Jackson

From: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>

Add documentation based on reverse-engineered toolstack-ioemu stubdomain
protocol.

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

---
Changes in v6:
 - Add Acked-by: Ian Jackson
---
 docs/misc/stubdom.txt | 53 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)

diff --git a/docs/misc/stubdom.txt b/docs/misc/stubdom.txt
index 882a18cab4..64c77d9b64 100644
--- a/docs/misc/stubdom.txt
+++ b/docs/misc/stubdom.txt
@@ -23,6 +23,59 @@ and https://wiki.xen.org/wiki/Device_Model_Stub_Domains for more
 information on device model stub domains
 
 
+Toolstack to MiniOS ioemu stubdomain protocol
+---------------------------------------------
+
+This section describe communication protocol between toolstack and
+qemu-traditional running in MiniOS stubdomain. The protocol include
+expectations of both qemu and stubdomain itself.
+
+Setup (done by toolstack, expected by stubdomain):
+ - Block devices for target domain are connected as PV disks to stubdomain,
+   according to configuration order, starting with xvda
+ - Network devices for target domain are connected as PV nics to stubdomain,
+   according to configuration order, starting with 0
+ - if graphics output is expected, VFB and VKB devices are set for stubdomain
+   (its backend is responsible for exposing them using appropriate protocol
+   like VNC or Spice)
+ - other target domain's devices are not connected at this point to stubdomain
+   (may be hot-plugged later)
+ - QEMU command line (space separated arguments) is stored in
+   /vm/<target-uuid>/image/dmargs xenstore path
+ - target domain id is stored in /local/domain/<stubdom-id>/target xenstore path
+?? - bios type is stored in /local/domain/<target-id>/hvmloader/bios
+ - stubdomain's console 0 is connected to qemu log file
+ - stubdomain's console 1 is connected to qemu save file (for saving state)
+ - stubdomain's console 2 is connected to qemu save file (for restoring state)
+ - next consoles are connected according to target guest's serial console configuration
+
+Startup:
+1. PV stubdomain is started with ioemu-stubdom.gz kernel and no initrd
+2. stubdomain initialize relevant devices
+3. stubdomain signal readiness by writing "running" to /local/domain/<stubdom-id>/device-model/<target-id>/state xenstore path
+4. now stubdomain is considered running
+
+Runtime control (hotplug etc):
+Toolstack can issue command through xenstore. The sequence is (from toolstack POV):
+1. Write parameter to /local/domain/<stubdom-id>/device-model/<target-id>/parameter.
+2. Write command to /local/domain/<stubdom-id>/device-model/<target-id>/command.
+3. Wait for command result in /local/domain/<stubdom-id>/device-model/<target-id>/state (command specific value).
+4. Write "running" back to /local/domain/<stubdom-id>/device-model/<target-id>/state.
+
+Defined commands:
+ - "pci-ins" - PCI hot plug, results:
+   - "pci-inserted" - success
+   - "pci-insert-failed" - failure
+ - "pci-rem" - PCI hot remove, results:
+   - "pci-removed" - success
+   - ??
+ - "save" - save domain state to console 1, results:
+   - "paused" - success
+ - "continue" - resume domain execution, after loading state from console 2 (require -loadvm command argument), results:
+   - "running" - success
+
+
+
                                    PV-GRUB
                                    =======
 
-- 
2.25.1



^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH v6 02/18] Document ioemu Linux stubdomain protocol
  2020-05-18  1:13 [PATCH v6 00/18] Add support for qemu-xen runnning in a Linux-based stubdomain Jason Andryuk
  2020-05-18  1:13 ` [PATCH v6 01/18] Document ioemu MiniOS stubdomain protocol Jason Andryuk
@ 2020-05-18  1:13 ` Jason Andryuk
  2020-05-18  1:13 ` [PATCH v6 03/18] libxl: fix qemu-trad cmdline for no sdl/vnc case Jason Andryuk
                   ` (16 subsequent siblings)
  18 siblings, 0 replies; 28+ messages in thread
From: Jason Andryuk @ 2020-05-18  1:13 UTC (permalink / raw)
  To: xen-devel
  Cc: Stefano Stabellini, Julien Grall, Wei Liu, Jason Andryuk,
	Andrew Cooper, Ian Jackson, Marek Marczykowski-Górecki,
	George Dunlap, Jan Beulich, Ian Jackson

From: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>

Add documentation for upcoming Linux stubdomain for qemu-upstream.

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

---
Changes in v6:
 - Add Acked-by: Ian Jackson
 - Replace dmargs with dm-argv for xenstore directory
 - Explain $STUBDOM_RESTORE_INCOMING_ARG for -incoming restore argument
---
 docs/misc/stubdom.txt | 52 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 52 insertions(+)

diff --git a/docs/misc/stubdom.txt b/docs/misc/stubdom.txt
index 64c77d9b64..c717a95d17 100644
--- a/docs/misc/stubdom.txt
+++ b/docs/misc/stubdom.txt
@@ -75,6 +75,58 @@ Defined commands:
    - "running" - success
 
 
+Toolstack to Linux ioemu stubdomain protocol
+--------------------------------------------
+
+This section describe communication protocol between toolstack and
+qemu-upstream running in Linux stubdomain. The protocol include
+expectations of both stubdomain, and qemu.
+
+Setup (done by toolstack, expected by stubdomain):
+ - Block devices for target domain are connected as PV disks to stubdomain,
+   according to configuration order, starting with xvda
+ - Network devices for target domain are connected as PV nics to stubdomain,
+   according to configuration order, starting with 0
+ - [not implemented] if graphics output is expected, VFB and VKB devices are set for stubdomain
+   (its backend is responsible for exposing them using appropriate protocol
+   like VNC or Spice)
+ - other target domain's devices are not connected at this point to stubdomain
+   (may be hot-plugged later)
+ - QEMU command line is stored in
+   /vm/<target-uuid>/image/dm-argv xenstore dir, each argument as separate key
+   in form /vm/<target-uuid>/image/dm-argv/NNN, where NNN is 0-padded argument
+   number
+ - target domain id is stored in /local/domain/<stubdom-id>/target xenstore path
+?? - bios type is stored in /local/domain/<target-id>/hvmloader/bios
+ - stubdomain's console 0 is connected to qemu log file
+ - stubdomain's console 1 is connected to qemu save file (for saving state)
+ - stubdomain's console 2 is connected to qemu save file (for restoring state)
+ - next consoles are connected according to target guest's serial console configuration
+
+Environment exposed by stubdomain to qemu (needed to construct appropriate qemu command line and later interact with qmp):
+ - target domain's disks are available as /dev/xvd[a-z]
+ - console 2 (incoming domain state) must be connected to an FD and the command
+   line argument $STUBDOM_RESTORE_INCOMING_ARG must be replaced with fd:$FD to
+   form "-incoming fd:$FD"
+ - console 1 (saving domain state) is added over QMP to qemu as "fdset-id 1" (done by stubdomain, toolstack doesn't need to care about it)
+ - nics are connected to relevant stubdomain PV vifs when available (qemu -netdev should specify ifname= explicitly)
+
+Startup:
+1. toolstack starts PV stubdomain with stubdom-linux-kernel kernel and stubdom-linux-initrd initrd
+2. stubdomain initialize relevant devices
+3. stubdomain starts qemu with requested command line, plus few stubdomain specific ones - including local qmp access options
+4. stubdomain starts vchan server on /local/domain/<stubdom-id>/device-model/<target-id>/qmp-vchan, exposing qmp socket to the toolstack
+5. qemu signal readiness by writing "running" to /local/domain/<stubdom-id>/device-model/<target-id>/state xenstore path
+6. now device model is considered running
+
+QEMU can be controlled using QMP over vchan at /local/domain/<stubdom-id>/device-model/<target-id>/qmp-vchan. Only one simultaneous connection is supported and toolstack needs to ensure that.
+
+Limitations:
+ - PCI passthrough require permissive mode
+ - only one nic is supported
+ - at most 26 emulated disks are supported (more are still available as PV disks)
+ - graphics output (VNC/SDL/Spice) not supported
+
 
                                    PV-GRUB
                                    =======
-- 
2.25.1



^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH v6 03/18] libxl: fix qemu-trad cmdline for no sdl/vnc case
  2020-05-18  1:13 [PATCH v6 00/18] Add support for qemu-xen runnning in a Linux-based stubdomain Jason Andryuk
  2020-05-18  1:13 ` [PATCH v6 01/18] Document ioemu MiniOS stubdomain protocol Jason Andryuk
  2020-05-18  1:13 ` [PATCH v6 02/18] Document ioemu Linux " Jason Andryuk
@ 2020-05-18  1:13 ` Jason Andryuk
  2020-05-18  1:13 ` [PATCH v6 04/18] libxl: Allow running qemu-xen in stubdomain Jason Andryuk
                   ` (15 subsequent siblings)
  18 siblings, 0 replies; 28+ messages in thread
From: Jason Andryuk @ 2020-05-18  1:13 UTC (permalink / raw)
  To: xen-devel
  Cc: Wei Liu, Wei Liu, Jason Andryuk, Ian Jackson,
	Marek Marczykowski-Górecki, Anthony PERARD, Ian Jackson

From: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>

When qemu is running in stubdomain, any attempt to initialize vnc/sdl
there will crash it (on failed attempt to load a keymap from a file). If
vfb is present, all those cases are skipped. But since
b053f0c4c9e533f3d97837cf897eb920b8355ed3 "libxl: do not start dom0 qemu
for stubdomain when not needed" it is possible to create a stubdomain
without vfb and contrary to the comment -vnc none do trigger VNC
initialization code (just skips exposing it externally).
Change the implicit SDL avoiding method to -nographics option, used when
none of SDL or VNC is enabled.

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
Reviewed-by: Jason Andryuk <jandryuk@gmail.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
Acked-by: Wei Liu <wei.liu2@citrix.com>
Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
---
Changes in v2:
 - typo in qemu option
Changes in v3:
 - add missing { }
---
 tools/libxl/libxl_dm.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index f4007bbe50..b91e63db6f 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -734,14 +734,15 @@ static int libxl__build_device_model_args_old(libxl__gc *gc,
         if (libxl_defbool_val(vnc->findunused)) {
             flexarray_append(dm_args, "-vncunused");
         }
-    } else
+    } else if (!sdl) {
         /*
          * VNC is not enabled by default by qemu-xen-traditional,
-         * however passing -vnc none causes SDL to not be
-         * (unexpectedly) enabled by default. This is overridden by
-         * explicitly passing -sdl below as required.
+         * however skipping -vnc causes SDL to be
+         * (unexpectedly) enabled by default. If undesired, disable graphics at
+         * all.
          */
-        flexarray_append_pair(dm_args, "-vnc", "none");
+        flexarray_append(dm_args, "-nographic");
+    }
 
     if (sdl) {
         flexarray_append(dm_args, "-sdl");
-- 
2.25.1



^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH v6 04/18] libxl: Allow running qemu-xen in stubdomain
  2020-05-18  1:13 [PATCH v6 00/18] Add support for qemu-xen runnning in a Linux-based stubdomain Jason Andryuk
                   ` (2 preceding siblings ...)
  2020-05-18  1:13 ` [PATCH v6 03/18] libxl: fix qemu-trad cmdline for no sdl/vnc case Jason Andryuk
@ 2020-05-18  1:13 ` Jason Andryuk
  2020-05-18  1:13 ` [PATCH v6 05/18] libxl: Handle Linux stubdomain specific QEMU options Jason Andryuk
                   ` (14 subsequent siblings)
  18 siblings, 0 replies; 28+ messages in thread
From: Jason Andryuk @ 2020-05-18  1:13 UTC (permalink / raw)
  To: xen-devel
  Cc: Wei Liu, Jason Andryuk, Ian Jackson,
	Marek Marczykowski-Górecki, Anthony PERARD, Ian Jackson

From: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>

Do not prohibit anymore using stubdomain with qemu-xen.
To help distingushing MiniOS and Linux stubdomain, add helper inline
functions libxl__stubdomain_is_linux() and
libxl__stubdomain_is_linux_running(). Those should be used where really
the difference is about MiniOS/Linux, not qemu-xen/qemu-xen-traditional.

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

---
Changes in v3:
 - new patch, instead of "libxl: Add "stubdomain_version" to
 domain_build_info"
 - helper functions as suggested by Ian Jackson
Changes in v6:
 - Add Acked-by: Ian Jackson
---
 tools/libxl/libxl_create.c   |  9 ---------
 tools/libxl/libxl_internal.h | 17 +++++++++++++++++
 2 files changed, 17 insertions(+), 9 deletions(-)

diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 5a043df15f..433947abab 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -171,15 +171,6 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
         }
     }
 
-    if (b_info->type == LIBXL_DOMAIN_TYPE_HVM &&
-        b_info->device_model_version !=
-            LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL &&
-        libxl_defbool_val(b_info->device_model_stubdomain)) {
-        LOG(ERROR,
-            "device model stubdomains require \"qemu-xen-traditional\"");
-        return ERROR_INVAL;
-    }
-
     if (!b_info->max_vcpus)
         b_info->max_vcpus = 1;
     if (!b_info->avail_vcpus.size) {
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index e5effd2ad1..d1ebdec8d2 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -2324,6 +2324,23 @@ _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);
 
+static inline
+bool libxl__stubdomain_is_linux_running(libxl__gc *gc, uint32_t domid)
+{
+    /* same logic as in libxl__stubdomain_is_linux */
+    return libxl__device_model_version_running(gc, domid)
+        == LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN;
+}
+
+static inline
+bool libxl__stubdomain_is_linux(libxl_domain_build_info *b_info)
+{
+    /* right now qemu-tranditional implies MiniOS stubdomain and qemu-xen
+     * implies Linux stubdomain */
+    return libxl_defbool_val(b_info->device_model_stubdomain) &&
+        b_info->device_model_version == LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN;
+}
+
 #define DEVICE_MODEL_XS_PATH(gc, dm_domid, domid, fmt, _a...)              \
     libxl__sprintf(gc, "/local/domain/%u/device-model/%u" fmt, dm_domid,   \
                    domid, ##_a)
-- 
2.25.1



^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH v6 05/18] libxl: Handle Linux stubdomain specific QEMU options.
  2020-05-18  1:13 [PATCH v6 00/18] Add support for qemu-xen runnning in a Linux-based stubdomain Jason Andryuk
                   ` (3 preceding siblings ...)
  2020-05-18  1:13 ` [PATCH v6 04/18] libxl: Allow running qemu-xen in stubdomain Jason Andryuk
@ 2020-05-18  1:13 ` Jason Andryuk
  2020-05-18  1:13 ` [PATCH v6 06/18] libxl: write qemu arguments into separate xenstore keys Jason Andryuk
                   ` (13 subsequent siblings)
  18 siblings, 0 replies; 28+ messages in thread
From: Jason Andryuk @ 2020-05-18  1:13 UTC (permalink / raw)
  To: xen-devel
  Cc: Wei Liu, Jason Andryuk, Ian Jackson,
	Marek Marczykowski-Górecki, Simon Gaiser, Anthony PERARD,
	Ian Jackson, Eric Shelton

From: Eric Shelton <eshelton@pobox.com>

This patch creates an appropriate command line for the QEMU instance
running in a Linux-based stubdomain.

NOTE: a number of items are not currently implemented for Linux-based
stubdomains, such as:
- save/restore
- QMP socket
- graphics output (e.g., VNC)

Signed-off-by: Eric Shelton <eshelton@pobox.com>

Simon:
 * fix disk path
 * fix cdrom path and "format"

Signed-off-by: Simon Gaiser <simon@invisiblethingslab.com>
[drop Qubes-specific parts]
Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>

Allow setting stubdomain_ramdisk independently from stubdomain_kernel
Add a qemu- prefix for qemu-stubdom-linux-{kernel,rootfs} since stubdom
doesn't convey device-model.  Use qemu- since this code is qemu specific.

Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
Changes in v2:
 - fix serial specified with serial=[ ... ] syntax
 - error out on multiple consoles (incompatible with stubdom)
 - drop erroneous chunk about cdrom
Changes in v3:
 - change to use libxl__stubdomain_is_linux instead of
   b_info->stubdomain_version
 - drop libxl__stubdomain_version_running, prefer
   libxl__stubdomain_is_linux_running introduced by previous patch
 - drop ifup/ifdown script - stubdomain will handle that with qemu
   events itself
 - slightly simplify -serial argument
 - add support for multiple serial consoles, do not ignore
   b_info.u.serial(_list)
 - add error checking for more than 26 emulated disks ("/dev/xvd%c"
   format string)
Changes in v5:
 - commit message fixup to match patch contents - Marek
 - file names are now qemu-stubdom-linux-{kernel,rootfs} - Jason
 - allow setting ramdisk independently of kernel - Jason
Changes in v6:
 - Add Acked-by: Ian Jackson
 - Fixes for style nits
---
 tools/libxl/libxl_create.c   |  45 ++++++++
 tools/libxl/libxl_dm.c       | 193 ++++++++++++++++++++++++-----------
 tools/libxl/libxl_internal.h |   1 +
 tools/libxl/libxl_mem.c      |   6 +-
 tools/libxl/libxl_types.idl  |   3 +
 5 files changed, 186 insertions(+), 62 deletions(-)

diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 433947abab..8614a2c241 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -171,6 +171,40 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
         }
     }
 
+    if (b_info->type == LIBXL_DOMAIN_TYPE_HVM &&
+        libxl_defbool_val(b_info->device_model_stubdomain)) {
+        if (!b_info->stubdomain_kernel) {
+            switch (b_info->device_model_version) {
+                case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL:
+                    b_info->stubdomain_kernel =
+                        libxl__abs_path(NOGC, "ioemu-stubdom.gz", libxl__xenfirmwaredir_path());
+                    break;
+                case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN:
+                    b_info->stubdomain_kernel =
+                        libxl__abs_path(NOGC,
+                                "qemu-stubdom-linux-kernel",
+                                libxl__xenfirmwaredir_path());
+                    break;
+                default:
+                    abort();
+            }
+        }
+        if (!b_info->stubdomain_ramdisk) {
+            switch (b_info->device_model_version) {
+                case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL:
+                    break;
+                case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN:
+                    b_info->stubdomain_ramdisk =
+                        libxl__abs_path(NOGC,
+                                "qemu-stubdom-linux-rootfs",
+                                libxl__xenfirmwaredir_path());
+                    break;
+                default:
+                    abort();
+            }
+        }
+    }
+
     if (!b_info->max_vcpus)
         b_info->max_vcpus = 1;
     if (!b_info->avail_vcpus.size) {
@@ -206,6 +240,17 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
     if (b_info->target_memkb == LIBXL_MEMKB_DEFAULT)
         b_info->target_memkb = b_info->max_memkb;
 
+    if (b_info->stubdomain_memkb == LIBXL_MEMKB_DEFAULT) {
+        if (libxl_defbool_val(b_info->device_model_stubdomain)) {
+            if (libxl__stubdomain_is_linux(b_info))
+                b_info->stubdomain_memkb = LIBXL_LINUX_STUBDOM_MEM * 1024;
+            else
+                b_info->stubdomain_memkb = 28 * 1024; // MiniOS
+        } else {
+            b_info->stubdomain_memkb = 0; // no stubdomain
+        }
+    }
+
     libxl_defbool_setdefault(&b_info->claim_mode, false);
 
     libxl_defbool_setdefault(&b_info->localtime, false);
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index b91e63db6f..dc1717bc12 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -1188,6 +1188,7 @@ static int libxl__build_device_model_args_new(libxl__gc *gc,
     int i, connection, devid;
     uint64_t ram_size;
     const char *path, *chardev;
+    bool is_stubdom = libxl_defbool_val(b_info->device_model_stubdomain);
 
     dm_args = flexarray_make(gc, 16, 1);
     dm_envs = flexarray_make(gc, 16, 1);
@@ -1197,39 +1198,42 @@ static int libxl__build_device_model_args_new(libxl__gc *gc,
     flexarray_vappend(dm_args, dm,
                       "-xen-domid",
                       GCSPRINTF("%d", guest_domid), NULL);
+    flexarray_append(dm_args, "-no-shutdown");
 
-    flexarray_append(dm_args, "-chardev");
-    if (state->dm_monitor_fd >= 0) {
-        flexarray_append(dm_args,
-            GCSPRINTF("socket,id=libxl-cmd,fd=%d,server,nowait",
-                      state->dm_monitor_fd));
+    /* There is currently no way to access the QMP socket in the stubdom */
+    if (!is_stubdom) {
+        flexarray_append(dm_args, "-chardev");
+        if (state->dm_monitor_fd >= 0) {
+            flexarray_append(dm_args,
+                GCSPRINTF("socket,id=libxl-cmd,fd=%d,server,nowait",
+                          state->dm_monitor_fd));
 
-        /*
-         * Start QEMU with its "CPU" paused, it will not start any emulation
-         * until the QMP command "cont" is used. This also prevent QEMU from
-         * writing "running" to the "state" xenstore node so we only use this
-         * flag when we have the QMP based startup notification.
-         * */
-        flexarray_append(dm_args, "-S");
-    } else {
-        flexarray_append(dm_args,
-                         GCSPRINTF("socket,id=libxl-cmd,"
-                                   "path=%s,server,nowait",
-                                   libxl__qemu_qmp_path(gc, guest_domid)));
-    }
+            /*
+             * Start QEMU with its "CPU" paused, it will not start any emulation
+             * until the QMP command "cont" is used. This also prevent QEMU from
+             * writing "running" to the "state" xenstore node so we only use this
+             * flag when we have the QMP based startup notification.
+             * */
+            flexarray_append(dm_args, "-S");
+        } else {
+            flexarray_append(dm_args,
+                             GCSPRINTF("socket,id=libxl-cmd,"
+                                       "path=%s,server,nowait",
+                                       libxl__qemu_qmp_path(gc, guest_domid)));
+        }
 
-    flexarray_append(dm_args, "-no-shutdown");
-    flexarray_append(dm_args, "-mon");
-    flexarray_append(dm_args, "chardev=libxl-cmd,mode=control");
+        flexarray_append(dm_args, "-mon");
+        flexarray_append(dm_args, "chardev=libxl-cmd,mode=control");
 
-    flexarray_append(dm_args, "-chardev");
-    flexarray_append(dm_args,
-                     GCSPRINTF("socket,id=libxenstat-cmd,"
-                                    "path=%s/qmp-libxenstat-%d,server,nowait",
-                                    libxl__run_dir_path(), guest_domid));
+        flexarray_append(dm_args, "-chardev");
+        flexarray_append(dm_args,
+                         GCSPRINTF("socket,id=libxenstat-cmd,"
+                                        "path=%s/qmp-libxenstat-%d,server,nowait",
+                                        libxl__run_dir_path(), guest_domid));
 
-    flexarray_append(dm_args, "-mon");
-    flexarray_append(dm_args, "chardev=libxenstat-cmd,mode=control");
+        flexarray_append(dm_args, "-mon");
+        flexarray_append(dm_args, "chardev=libxenstat-cmd,mode=control");
+    }
 
     for (i = 0; i < guest_config->num_channels; i++) {
         connection = guest_config->channels[i].connection;
@@ -1273,7 +1277,7 @@ static int libxl__build_device_model_args_new(libxl__gc *gc,
         flexarray_vappend(dm_args, "-name", c_info->name, NULL);
     }
 
-    if (vnc) {
+    if (vnc && !is_stubdom) {
         char *vncarg = NULL;
 
         flexarray_append(dm_args, "-vnc");
@@ -1312,11 +1316,12 @@ static int libxl__build_device_model_args_new(libxl__gc *gc,
         }
 
         flexarray_append(dm_args, vncarg);
-    } else
+    } else if (!is_stubdom) {
         /*
          * Ensure that by default no vnc server is created.
          */
         flexarray_append_pair(dm_args, "-vnc", "none");
+    }
 
     /*
      * Ensure that by default no display backend is created. Further
@@ -1324,7 +1329,7 @@ static int libxl__build_device_model_args_new(libxl__gc *gc,
      */
     flexarray_append_pair(dm_args, "-display", "none");
 
-    if (sdl) {
+    if (sdl && !is_stubdom) {
         flexarray_append(dm_args, "-sdl");
         if (sdl->display)
             flexarray_append_pair(dm_envs, "DISPLAY", sdl->display);
@@ -1366,18 +1371,34 @@ static int libxl__build_device_model_args_new(libxl__gc *gc,
             {
                 LOGD(ERROR, guest_domid, "Both serial and serial_list set");
                 return ERROR_INVAL;
-            }
-            if (b_info->u.hvm.serial) {
-                flexarray_vappend(dm_args,
-                                  "-serial", b_info->u.hvm.serial, NULL);
-            } else if (b_info->u.hvm.serial_list) {
-                char **p;
-                for (p = b_info->u.hvm.serial_list;
-                     *p;
-                     p++) {
-                    flexarray_vappend(dm_args,
-                                      "-serial",
-                                      *p, NULL);
+            } else {
+                if (b_info->u.hvm.serial) {
+                    if (is_stubdom) {
+                        /* see spawn_stub_launch_dm() for connecting STUBDOM_CONSOLE_SERIAL */
+                        flexarray_vappend(dm_args,
+                                          "-serial",
+                                          GCSPRINTF("/dev/hvc%d", STUBDOM_CONSOLE_SERIAL),
+                                          NULL);
+                    } else {
+                        flexarray_vappend(dm_args,
+                                          "-serial", b_info->u.hvm.serial, NULL);
+                    }
+                } else if (b_info->u.hvm.serial_list) {
+                    char **p;
+                    /* see spawn_stub_launch_dm() for connecting STUBDOM_CONSOLE_SERIAL */
+                    for (p = b_info->u.hvm.serial_list, i = 0;
+                         *p;
+                         p++, i++) {
+                        if (is_stubdom)
+                            flexarray_vappend(dm_args,
+                                              "-serial",
+                                              GCSPRINTF("/dev/hvc%d", STUBDOM_CONSOLE_SERIAL + i),
+                                              NULL);
+                        else
+                            flexarray_vappend(dm_args,
+                                              "-serial",
+                                              *p, NULL);
+                    }
                 }
             }
         }
@@ -1386,7 +1407,7 @@ static int libxl__build_device_model_args_new(libxl__gc *gc,
             flexarray_append(dm_args, "-nographic");
         }
 
-        if (libxl_defbool_val(b_info->u.hvm.spice.enable)) {
+        if (libxl_defbool_val(b_info->u.hvm.spice.enable) && !is_stubdom) {
             const libxl_spice_info *spice = &b_info->u.hvm.spice;
             char *spiceoptions = dm_spice_options(gc, spice);
             if (!spiceoptions)
@@ -1813,7 +1834,9 @@ static int libxl__build_device_model_args_new(libxl__gc *gc,
              * If qemu isn't doing the interpreting, the parameter is
              * always raw
              */
-            if (disks[i].backend == LIBXL_DISK_BACKEND_QDISK)
+            if (libxl_defbool_val(b_info->device_model_stubdomain))
+                format = "host_device";
+            else if (disks[i].backend == LIBXL_DISK_BACKEND_QDISK)
                 format = libxl__qemu_disk_format_string(disks[i].format);
             else
                 format = libxl__qemu_disk_format_string(LIBXL_DISK_FORMAT_RAW);
@@ -1824,6 +1847,16 @@ static int libxl__build_device_model_args_new(libxl__gc *gc,
                          disks[i].vdev);
                     continue;
                 }
+            } else if (libxl_defbool_val(b_info->device_model_stubdomain)) {
+                if (disk > 'z' - 'a') {
+                    LOGD(WARN, guest_domid,
+                            "Emulation of only first %d disks is supported with qemu-xen in stubdomain.\n"
+                            "Disk %d will be available via PV drivers but not as an emulated disk.",
+                            'z' - 'a',
+                            disk);
+                    continue;
+                }
+                target_path = GCSPRINTF("/dev/xvd%c", 'a' + disk);
             } else {
                 if (format == NULL) {
                     LOGD(WARN, guest_domid,
@@ -1964,7 +1997,7 @@ static int libxl__build_device_model_args(libxl__gc *gc,
                                         char ***args, char ***envs,
                                         const libxl__domain_build_state *state,
                                         int *dm_state_fd)
-/* dm_state_fd may be NULL iff caller knows we are using old stubdom
+/* dm_state_fd may be NULL iff caller knows we are using stubdom
  * and therefore will be passing a filename rather than a fd. */
 {
     switch (guest_config->b_info.device_model_version) {
@@ -1974,8 +2007,10 @@ static int libxl__build_device_model_args(libxl__gc *gc,
                                                   args, envs,
                                                   state);
     case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN:
-        assert(dm_state_fd != NULL);
-        assert(*dm_state_fd < 0);
+        if (!libxl_defbool_val(guest_config->b_info.device_model_stubdomain)) {
+            assert(dm_state_fd != NULL);
+            assert(*dm_state_fd < 0);
+        }
         return libxl__build_device_model_args_new(gc, dm,
                                                   guest_domid, guest_config,
                                                   args, envs,
@@ -2080,6 +2115,16 @@ retry_transaction:
     return 0;
 }
 
+static int libxl__store_libxl_entry(libxl__gc *gc, uint32_t domid,
+                                    const char *name, const char *value)
+{
+    char *path = NULL;
+
+    path = libxl__xs_libxl_path(gc, domid);
+    path = libxl__sprintf(gc, "%s/%s", path, name);
+    return libxl__xs_printf(gc, XBT_NULL, path, "%s", value);
+}
+
 static void dmss_init(libxl__dm_spawn_state *dmss)
 {
     libxl__ev_qmp_init(&dmss->qmp);
@@ -2138,10 +2183,14 @@ void libxl__spawn_stub_dm(libxl__egc *egc, libxl__stub_dm_spawn_state *sdss)
     dmss_init(&sdss->pvqemu);
     libxl__xswait_init(&sdss->xswait);
 
-    if (guest_config->b_info.device_model_version !=
-        LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL) {
-        ret = ERROR_INVAL;
-        goto out;
+    assert(libxl_defbool_val(guest_config->b_info.device_model_stubdomain));
+
+    if (libxl__stubdomain_is_linux(&guest_config->b_info)) {
+        if (d_state->saved_state) {
+            LOG(ERROR, "Save/Restore not supported yet with Linux Stubdom.");
+            ret = -1;
+            goto out;
+        }
     }
 
     sdss->pvqemu.guest_domid = INVALID_DOMID;
@@ -2163,8 +2212,8 @@ void libxl__spawn_stub_dm(libxl__egc *egc, libxl__stub_dm_spawn_state *sdss)
 
     dm_config->b_info.shadow_memkb = 0;
     dm_config->b_info.max_vcpus = 1;
-    dm_config->b_info.max_memkb = 28 * 1024 +
-        guest_config->b_info.video_memkb;
+    dm_config->b_info.max_memkb = guest_config->b_info.stubdomain_memkb;
+    dm_config->b_info.max_memkb += guest_config->b_info.video_memkb;
     dm_config->b_info.target_memkb = dm_config->b_info.max_memkb;
 
     dm_config->b_info.max_grant_frames = guest_config->b_info.max_grant_frames;
@@ -2203,10 +2252,8 @@ void libxl__spawn_stub_dm(libxl__egc *egc, libxl__stub_dm_spawn_state *sdss)
         dm_config->num_vkbs = 1;
     }
 
-    stubdom_state->pv_kernel.path
-        = libxl__abs_path(gc, "ioemu-stubdom.gz", libxl__xenfirmwaredir_path());
-    stubdom_state->pv_cmdline = GCSPRINTF(" -d %d", guest_domid);
-    stubdom_state->pv_ramdisk.path = "";
+    stubdom_state->pv_kernel.path = guest_config->b_info.stubdomain_kernel;
+    stubdom_state->pv_ramdisk.path = guest_config->b_info.stubdomain_ramdisk;
 
     /* fixme: this function can leak the stubdom if it fails */
     ret = libxl__domain_make(gc, dm_config, stubdom_state,
@@ -2226,6 +2273,8 @@ void libxl__spawn_stub_dm(libxl__egc *egc, libxl__stub_dm_spawn_state *sdss)
         goto out;
     }
 
+    libxl__store_libxl_entry(gc, guest_domid, "dm-version",
+        libxl_device_model_version_to_string(dm_config->b_info.device_model_version));
     libxl__write_stub_dmargs(gc, dm_domid, guest_domid, args);
     libxl__xs_printf(gc, XBT_NULL,
                      GCSPRINTF("%s/image/device-model-domid",
@@ -2235,6 +2284,15 @@ void libxl__spawn_stub_dm(libxl__egc *egc, libxl__stub_dm_spawn_state *sdss)
                      GCSPRINTF("%s/target",
                                libxl__xs_get_dompath(gc, dm_domid)),
                      "%d", guest_domid);
+    if (guest_config->b_info.device_model_version == LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN) {
+        /* qemu-xen is used as a dm in the stubdomain, so we set the bios
+         * accroding to this */
+        libxl__xs_printf(gc, XBT_NULL,
+                        libxl__sprintf(gc, "%s/hvmloader/bios",
+                                       libxl__xs_get_dompath(gc, guest_domid)),
+                        "%s",
+                        libxl_bios_type_to_string(guest_config->b_info.u.hvm.bios));
+    }
     ret = xc_domain_set_target(ctx->xch, dm_domid, guest_domid);
     if (ret<0) {
         LOGED(ERROR, guest_domid, "setting target domain %d -> %d",
@@ -2314,8 +2372,13 @@ static void spawn_stub_launch_dm(libxl__egc *egc,
         if (ret) goto out;
     }
 
-    if (guest_config->b_info.u.hvm.serial)
+    if (guest_config->b_info.u.hvm.serial) {
         num_console++;
+    } else if (guest_config->b_info.u.hvm.serial_list) {
+        char **serial = guest_config->b_info.u.hvm.serial_list;
+        while (*(serial++))
+            num_console++;
+    }
 
     console = libxl__calloc(gc, num_console, sizeof(libxl__device_console));
 
@@ -2349,8 +2412,18 @@ static void spawn_stub_launch_dm(libxl__egc *egc,
                     console[i].output =
                         GCSPRINTF("pipe:%s", d_state->saved_state);
                 break;
+            case STUBDOM_CONSOLE_SERIAL:
+                if (guest_config->b_info.u.hvm.serial) {
+                    console[i].output = guest_config->b_info.u.hvm.serial;
+                    break;
+                }
+                /* fall-through */
             default:
-                console[i].output = "pty";
+                /* Serial_list is set, as otherwise num_consoles would be
+                 * smaller and consoles 0-2 are handled above. */
+                assert(guest_config->b_info.u.hvm.serial_list);
+                console[i].output = guest_config->b_info.u.hvm.serial_list[
+                    i-STUBDOM_CONSOLE_SERIAL];
                 break;
         }
     }
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index d1ebdec8d2..f2f76439ec 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -119,6 +119,7 @@
 #define STUBDOM_CONSOLE_RESTORE 2
 #define STUBDOM_CONSOLE_SERIAL 3
 #define STUBDOM_SPECIAL_CONSOLES 3
+#define LIBXL_LINUX_STUBDOM_MEM 128
 #define TAP_DEVICE_SUFFIX "-emu"
 #define DOMID_XS_PATH "domid"
 #define PVSHIM_BASENAME "xen-shim"
diff --git a/tools/libxl/libxl_mem.c b/tools/libxl/libxl_mem.c
index bc7b95aa74..e52a9624ea 100644
--- a/tools/libxl/libxl_mem.c
+++ b/tools/libxl/libxl_mem.c
@@ -459,8 +459,10 @@ int libxl__domain_need_memory_calculate(libxl__gc *gc,
     case LIBXL_DOMAIN_TYPE_PVH:
     case LIBXL_DOMAIN_TYPE_HVM:
         *need_memkb += LIBXL_HVM_EXTRA_MEMORY;
-        if (libxl_defbool_val(b_info->device_model_stubdomain))
-            *need_memkb += 32 * 1024;
+        if (libxl_defbool_val(b_info->device_model_stubdomain)) {
+            *need_memkb += b_info->stubdomain_memkb;
+            *need_memkb += b_info->video_memkb;
+        }
         break;
     case LIBXL_DOMAIN_TYPE_PV:
         *need_memkb += LIBXL_PV_EXTRA_MEMORY;
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index f7c473be74..9d3f05f399 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -518,6 +518,9 @@ libxl_domain_build_info = Struct("domain_build_info",[
     
     ("device_model_version", libxl_device_model_version),
     ("device_model_stubdomain", libxl_defbool),
+    ("stubdomain_memkb",   MemKB),
+    ("stubdomain_kernel",  string),
+    ("stubdomain_ramdisk", string),
     # if you set device_model you must set device_model_version too
     ("device_model",     string),
     ("device_model_ssidref", uint32),
-- 
2.25.1



^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH v6 06/18] libxl: write qemu arguments into separate xenstore keys
  2020-05-18  1:13 [PATCH v6 00/18] Add support for qemu-xen runnning in a Linux-based stubdomain Jason Andryuk
                   ` (4 preceding siblings ...)
  2020-05-18  1:13 ` [PATCH v6 05/18] libxl: Handle Linux stubdomain specific QEMU options Jason Andryuk
@ 2020-05-18  1:13 ` Jason Andryuk
  2020-05-18 14:19   ` Ian Jackson
  2020-05-18  1:13 ` [PATCH v6 07/18] xl: add stubdomain related options to xl config parser Jason Andryuk
                   ` (12 subsequent siblings)
  18 siblings, 1 reply; 28+ messages in thread
From: Jason Andryuk @ 2020-05-18  1:13 UTC (permalink / raw)
  To: xen-devel
  Cc: Wei Liu, Jason Andryuk, Ian Jackson,
	Marek Marczykowski-Górecki, Anthony PERARD, Ian Jackson

From: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>

This allows using arguments with spaces, like -append, without
nominating any special "separator" character.

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
Reviewed-by: Jason Andryuk <jandryuk@gmail.com>

Re-work to use libxl_xs_* functions in a loop.  Also write arguments in
dm-argv directory instead of overloading mini-os's dmargs string.

Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
---
Changes in v3:
 - previous version of this patch "libxl: use \x1b to separate qemu
   arguments for linux stubdomain" used specific non-printable
   separator, but it was rejected as xenstore doesn't cope well with
   non-printable chars
Changes in v6:
 - Re-work to use libxl__xs_ functions in a loop.
 - Drop rtc/timeoffset
---
 tools/libxl/libxl_dm.c | 56 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 55 insertions(+), 1 deletion(-)

diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index dc1717bc12..eaed6e8ee7 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -2066,6 +2066,57 @@ static int libxl__vfb_and_vkb_from_hvm_guest_config(libxl__gc *gc,
     return 0;
 }
 
+static int libxl__write_stub_linux_dm_argv(libxl__gc *gc,
+                                           int dm_domid, int guest_domid,
+                                           char **args)
+{
+    const char *vm_path;
+    char *path;
+    struct xs_permissions roperm[2];
+    xs_transaction_t t = XBT_NULL;
+    int rc;
+
+    roperm[0].id = 0;
+    roperm[0].perms = XS_PERM_NONE;
+    roperm[1].id = dm_domid;
+    roperm[1].perms = XS_PERM_READ;
+
+    rc = libxl__xs_read_mandatory(gc, XBT_NULL,
+                                  GCSPRINTF("/local/domain/%d/vm", guest_domid),
+                                  &vm_path);
+    if (rc)
+        return rc;
+
+    path = GCSPRINTF("%s/image/dm-argv", vm_path);
+
+    for (;;) {
+        int i;
+
+        rc = libxl__xs_transaction_start(gc, &t);
+        if (rc) goto out;
+
+        rc = libxl__xs_mknod(gc, t, path, roperm, ARRAY_SIZE(roperm));
+        if (rc) goto out;
+
+        for (i=1; args[i] != NULL; i++) {
+            rc = libxl__xs_write_checked(gc, t, GCSPRINTF("%s/%03d", path, i),
+                                         args[i]);
+            if (rc) goto out;
+        }
+
+        rc = libxl__xs_transaction_commit(gc, &t);
+        if (!rc) break;
+        if (rc<0) goto out;
+    }
+
+    return 0;
+
+ out:
+    libxl__xs_transaction_abort(gc, &t);
+
+    return rc;
+}
+
 static int libxl__write_stub_dmargs(libxl__gc *gc,
                                     int dm_domid, int guest_domid,
                                     char **args)
@@ -2275,7 +2326,10 @@ void libxl__spawn_stub_dm(libxl__egc *egc, libxl__stub_dm_spawn_state *sdss)
 
     libxl__store_libxl_entry(gc, guest_domid, "dm-version",
         libxl_device_model_version_to_string(dm_config->b_info.device_model_version));
-    libxl__write_stub_dmargs(gc, dm_domid, guest_domid, args);
+    if (libxl__stubdomain_is_linux(&guest_config->b_info))
+        libxl__write_stub_linux_dm_argv(gc, dm_domid, guest_domid, args);
+    else
+        libxl__write_stub_dmargs(gc, dm_domid, guest_domid, args);
     libxl__xs_printf(gc, XBT_NULL,
                      GCSPRINTF("%s/image/device-model-domid",
                                libxl__xs_get_dompath(gc, guest_domid)),
-- 
2.25.1



^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH v6 07/18] xl: add stubdomain related options to xl config parser
  2020-05-18  1:13 [PATCH v6 00/18] Add support for qemu-xen runnning in a Linux-based stubdomain Jason Andryuk
                   ` (5 preceding siblings ...)
  2020-05-18  1:13 ` [PATCH v6 06/18] libxl: write qemu arguments into separate xenstore keys Jason Andryuk
@ 2020-05-18  1:13 ` Jason Andryuk
  2020-05-18  1:13 ` [PATCH v6 08/18] tools/libvchan: notify server when client is connected Jason Andryuk
                   ` (11 subsequent siblings)
  18 siblings, 0 replies; 28+ messages in thread
From: Jason Andryuk @ 2020-05-18  1:13 UTC (permalink / raw)
  To: xen-devel
  Cc: Wei Liu, Jason Andryuk, Ian Jackson,
	Marek Marczykowski-Górecki, Anthony PERARD, Ian Jackson

From: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
Reviewed-by: Jason Andryuk <jandryuk@gmail.com>
Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

---
Changes in v6:
 - Add Acked-by: Ian Jackson
---
 docs/man/xl.cfg.5.pod.in | 27 +++++++++++++++++++++++----
 tools/xl/xl_parse.c      |  7 +++++++
 2 files changed, 30 insertions(+), 4 deletions(-)

diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in
index 0e9e58a41a..c9bc181a95 100644
--- a/docs/man/xl.cfg.5.pod.in
+++ b/docs/man/xl.cfg.5.pod.in
@@ -2733,10 +2733,29 @@ model which they were installed with.
 
 =item B<device_model_override="PATH">
 
-Override the path to the binary to be used as the device-model. The
-binary provided here MUST be consistent with the
-B<device_model_version> which you have specified. You should not
-normally need to specify this option.
+Override the path to the binary to be used as the device-model running in
+toolstack domain. The binary provided here MUST be consistent with the
+B<device_model_version> which you have specified. You should not normally need
+to specify this option.
+
+=item B<stubdomain_kernel="PATH">
+
+Override the path to the kernel image used as device-model stubdomain.
+The binary provided here MUST be consistent with the
+B<device_model_version> which you have specified.
+In case of B<qemu-xen-traditional> it is expected to be MiniOS-based stubdomain
+image, in case of B<qemu-xen> it is expected to be Linux-based stubdomain
+kernel.
+
+=item B<stubdomain_ramdisk="PATH">
+
+Override the path to the ramdisk image used as device-model stubdomain.
+The binary provided here is to be used by a kernel pointed by B<stubdomain_kernel>.
+It is known to be used only by Linux-based stubdomain kernel.
+
+=item B<stubdomain_memory=MBYTES>
+
+Start the stubdomain with MBYTES megabytes of RAM. Default is 128.
 
 =item B<device_model_stubdomain_override=BOOLEAN>
 
diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
index 4450d59f16..61b4ef7b7e 100644
--- a/tools/xl/xl_parse.c
+++ b/tools/xl/xl_parse.c
@@ -2525,6 +2525,13 @@ skip_usbdev:
     xlu_cfg_replace_string(config, "device_model_user",
                            &b_info->device_model_user, 0);
 
+    xlu_cfg_replace_string (config, "stubdomain_kernel",
+                            &b_info->stubdomain_kernel, 0);
+    xlu_cfg_replace_string (config, "stubdomain_ramdisk",
+                            &b_info->stubdomain_ramdisk, 0);
+    if (!xlu_cfg_get_long (config, "stubdomain_memory", &l, 0))
+        b_info->stubdomain_memkb = l * 1024;
+
 #define parse_extra_args(type)                                            \
     e = xlu_cfg_get_list_as_string_list(config, "device_model_args"#type, \
                                     &b_info->extra##type, 0);            \
-- 
2.25.1



^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH v6 08/18] tools/libvchan: notify server when client is connected
  2020-05-18  1:13 [PATCH v6 00/18] Add support for qemu-xen runnning in a Linux-based stubdomain Jason Andryuk
                   ` (6 preceding siblings ...)
  2020-05-18  1:13 ` [PATCH v6 07/18] xl: add stubdomain related options to xl config parser Jason Andryuk
@ 2020-05-18  1:13 ` Jason Andryuk
  2020-05-18  1:13 ` [PATCH v6 09/18] libxl: add save/restore support for qemu-xen in stubdomain Jason Andryuk
                   ` (10 subsequent siblings)
  18 siblings, 0 replies; 28+ messages in thread
From: Jason Andryuk @ 2020-05-18  1:13 UTC (permalink / raw)
  To: xen-devel
  Cc: Wei Liu, Jason Andryuk, Ian Jackson,
	Marek Marczykowski-Górecki, Ian Jackson, Daniel De Graaf

From: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>

Let the server know when the client is connected. Otherwise server will
notice only when client send some data.
This change does not break existing clients, as libvchan user should
handle spurious notifications anyway (for example acknowledge of remote
side reading the data).

Cc: Daniel De Graaf <dgdegra@tycho.nsa.gov>
Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
Replace spaces with tabs to match the file's whitespace.
Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
Marek: I had this patch in Qubes for a long time and totally forgot it
wasn't upstream thing...

Changes in v6:
 - Add Acked-by: Ian Jackson
 - CC Daniel De Graaf
---
 tools/libvchan/init.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tools/libvchan/init.c b/tools/libvchan/init.c
index 180833dc2f..ad4b64fbe3 100644
--- a/tools/libvchan/init.c
+++ b/tools/libvchan/init.c
@@ -447,6 +447,9 @@ struct libxenvchan *libxenvchan_client_init(struct xentoollog_logger *logger,
 	ctrl->ring->cli_live = 1;
 	ctrl->ring->srv_notify = VCHAN_NOTIFY_WRITE;
 
+	/* wake up the server */
+	xenevtchn_notify(ctrl->event, ctrl->event_port);
+
  out:
 	if (xs)
 		xs_daemon_close(xs);
-- 
2.25.1



^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH v6 09/18] libxl: add save/restore support for qemu-xen in stubdomain
  2020-05-18  1:13 [PATCH v6 00/18] Add support for qemu-xen runnning in a Linux-based stubdomain Jason Andryuk
                   ` (7 preceding siblings ...)
  2020-05-18  1:13 ` [PATCH v6 08/18] tools/libvchan: notify server when client is connected Jason Andryuk
@ 2020-05-18  1:13 ` Jason Andryuk
  2020-05-18 14:24   ` Ian Jackson
  2020-05-18  1:13 ` [PATCH v6 10/18] tools: add missing libxenvchan cflags Jason Andryuk
                   ` (9 subsequent siblings)
  18 siblings, 1 reply; 28+ messages in thread
From: Jason Andryuk @ 2020-05-18  1:13 UTC (permalink / raw)
  To: xen-devel
  Cc: Wei Liu, Jason Andryuk, Ian Jackson,
	Marek Marczykowski-Górecki, Anthony PERARD, Ian Jackson

From: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>

Rely on a wrapper script in stubdomain to attach relevant consoles to
qemu.  The save console (1) must be attached to fdset/1.  When
performing a restore, $STUBDOM_RESTORE_INCOMING_ARG must be replaced on
the qemu command line by "fd:$FD", where $FD is an open file descriptor
number to the restore console (2).

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

Address TODO in dm_state_save_to_fdset: Only remove savefile for
non-stubdom.
Use $STUBDOM_RESTORE_INCOMING_ARG instead of fd:3 and update commit
message.

Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
---
Changes in v3:
 - adjust for qmp_ev*
 - assume specific fdset id in qemu set in stubdomain
Changes in v5:
 - Only remove savefile for non-stubdom
Chanres in v6:
 - Replace hardcoded fd:3 with placeholder $STUBDOM_RESTORE_INCOMING_ARG
---
 tools/libxl/libxl_dm.c  | 25 +++++++++++++------------
 tools/libxl/libxl_qmp.c | 27 +++++++++++++++++++++++++--
 2 files changed, 38 insertions(+), 14 deletions(-)

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



^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH v6 10/18] tools: add missing libxenvchan cflags
  2020-05-18  1:13 [PATCH v6 00/18] Add support for qemu-xen runnning in a Linux-based stubdomain Jason Andryuk
                   ` (8 preceding siblings ...)
  2020-05-18  1:13 ` [PATCH v6 09/18] libxl: add save/restore support for qemu-xen in stubdomain Jason Andryuk
@ 2020-05-18  1:13 ` Jason Andryuk
  2020-05-18  1:13 ` [PATCH v6 11/18] tools: add simple vchan-socket-proxy Jason Andryuk
                   ` (8 subsequent siblings)
  18 siblings, 0 replies; 28+ messages in thread
From: Jason Andryuk @ 2020-05-18  1:13 UTC (permalink / raw)
  To: xen-devel
  Cc: Ian Jackson, Ian Jackson, Marek Marczykowski-Górecki,
	Wei Liu, Jason Andryuk

From: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>

libxenvchan.h include xenevtchn.h and xengnttab.h, so applications built
with it needs applicable -I in CFLAGS too.

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
Reviewed-by: Jason Andryuk <jandryuk@gmail.com>
Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

---
Changes in v6
 - Add Acked-by: Ian Jackson
---
 tools/Rules.mk | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/Rules.mk b/tools/Rules.mk
index 5b8cf748ad..59c72e7a88 100644
--- a/tools/Rules.mk
+++ b/tools/Rules.mk
@@ -157,7 +157,7 @@ SHDEPS_libxenstat  = $(SHLIB_libxenctrl) $(SHLIB_libxenstore)
 LDLIBS_libxenstat  = $(SHDEPS_libxenstat) $(XEN_LIBXENSTAT)/libxenstat$(libextension)
 SHLIB_libxenstat   = $(SHDEPS_libxenstat) -Wl,-rpath-link=$(XEN_LIBXENSTAT)
 
-CFLAGS_libxenvchan = -I$(XEN_LIBVCHAN)
+CFLAGS_libxenvchan = -I$(XEN_LIBVCHAN) $(CFLAGS_libxengnttab) $(CFLAGS_libxenevtchn)
 SHDEPS_libxenvchan = $(SHLIB_libxentoollog) $(SHLIB_libxenstore) $(SHLIB_libxenevtchn) $(SHLIB_libxengnttab)
 LDLIBS_libxenvchan = $(SHDEPS_libxenvchan) $(XEN_LIBVCHAN)/libxenvchan$(libextension)
 SHLIB_libxenvchan  = $(SHDEPS_libxenvchan) -Wl,-rpath-link=$(XEN_LIBVCHAN)
-- 
2.25.1



^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH v6 11/18] tools: add simple vchan-socket-proxy
  2020-05-18  1:13 [PATCH v6 00/18] Add support for qemu-xen runnning in a Linux-based stubdomain Jason Andryuk
                   ` (9 preceding siblings ...)
  2020-05-18  1:13 ` [PATCH v6 10/18] tools: add missing libxenvchan cflags Jason Andryuk
@ 2020-05-18  1:13 ` Jason Andryuk
  2020-05-18  1:13 ` [PATCH v6 12/18] libxl: Refactor kill_device_model to libxl__kill_xs_path Jason Andryuk
                   ` (7 subsequent siblings)
  18 siblings, 0 replies; 28+ messages in thread
From: Jason Andryuk @ 2020-05-18  1:13 UTC (permalink / raw)
  To: xen-devel
  Cc: Stefano Stabellini, Julien Grall, Wei Liu, Jason Andryuk,
	Andrew Cooper, Ian Jackson, Marek Marczykowski-Górecki,
	George Dunlap, Jan Beulich, Ian Jackson

From: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>

Add a simple proxy for tunneling socket connection over vchan. This is
based on existing vchan-node* applications, but extended with socket
support. vchan-socket-proxy serves both as a client and as a server,
depending on parameters. It can be used to transparently communicate
with an application in another domian that normally expose UNIX socket
interface. Specifically, it's written to communicate with qemu running
within stubdom.

Server mode listens for vchan connections and when one is opened,
connects to a pointed UNIX socket.  Client mode listens on UNIX
socket and when someone connects, opens a vchan connection.  Only
a single connection at a time is supported.

Additionally, socket can be provided as a number - in which case it's
interpreted as already open FD (in case of UNIX listening socket -
listen() needs to be already called). Or "-" meaning stdin/stdout - in
which case it is reduced to vchan-node2 functionality.

Example usage:

1. (in dom0) vchan-socket-proxy --mode=client <DOMID>
    /local/domain/<DOMID>/data/vchan/1234 /run/qemu.(DOMID)

2. (in DOMID) vchan-socket-proxy --mode=server 0
   /local/domain/<DOMID>/data/vchan/1234 /run/qemu.(DOMID)

This will listen on /run/qemu.(DOMID) in dom0 and whenever connection is
made, it will connect to DOMID, where server process will connect to
/run/qemu.(DOMID) there. When client disconnects, vchan connection is
terminated and server vchan-socket-proxy process also disconnects from
qemu.

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
Reviewed-by: Jason Andryuk <jandryuk@gmail.com>
Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

---
Changes on v5:
 - Ensure bindir directory is present
 - String and comment fixes
Changes in v6
 - Add Acked-by: Ian Jackson
---
 .gitignore                          |   1 +
 tools/libvchan/Makefile             |   8 +-
 tools/libvchan/vchan-socket-proxy.c | 478 ++++++++++++++++++++++++++++
 3 files changed, 486 insertions(+), 1 deletion(-)
 create mode 100644 tools/libvchan/vchan-socket-proxy.c

diff --git a/.gitignore b/.gitignore
index bfa53723b3..7418ce9829 100644
--- a/.gitignore
+++ b/.gitignore
@@ -369,6 +369,7 @@ tools/misc/xenwatchdogd
 tools/misc/xen-hvmcrash
 tools/misc/xen-lowmemd
 tools/libvchan/vchan-node[12]
+tools/libvchan/vchan-socket-proxy
 tools/ocaml/*/.ocamldep.make
 tools/ocaml/*/*.cm[ixao]
 tools/ocaml/*/*.cmxa
diff --git a/tools/libvchan/Makefile b/tools/libvchan/Makefile
index 7892750c3e..913bcc8884 100644
--- a/tools/libvchan/Makefile
+++ b/tools/libvchan/Makefile
@@ -13,6 +13,7 @@ LIBVCHAN_PIC_OBJS = $(patsubst %.o,%.opic,$(LIBVCHAN_OBJS))
 LIBVCHAN_LIBS = $(LDLIBS_libxenstore) $(LDLIBS_libxengnttab) $(LDLIBS_libxenevtchn)
 $(LIBVCHAN_OBJS) $(LIBVCHAN_PIC_OBJS): CFLAGS += $(CFLAGS_libxenstore) $(CFLAGS_libxengnttab) $(CFLAGS_libxenevtchn)
 $(NODE_OBJS) $(NODE2_OBJS): CFLAGS += $(CFLAGS_libxengnttab) $(CFLAGS_libxenevtchn)
+vchan-socket-proxy.o: CFLAGS += $(CFLAGS_libxenstore) $(CFLAGS_libxenctrl) $(CFLAGS_libxengnttab) $(CFLAGS_libxenevtchn)
 
 MAJOR = 4.14
 MINOR = 0
@@ -39,7 +40,7 @@ $(PKG_CONFIG_LOCAL): PKG_CONFIG_LIBDIR = $(CURDIR)
 $(PKG_CONFIG_LOCAL): PKG_CONFIG_CFLAGS_LOCAL = $(CFLAGS_xeninclude)
 
 .PHONY: all
-all: libxenvchan.so vchan-node1 vchan-node2 libxenvchan.a $(PKG_CONFIG_INST) $(PKG_CONFIG_LOCAL)
+all: libxenvchan.so vchan-node1 vchan-node2 vchan-socket-proxy libxenvchan.a $(PKG_CONFIG_INST) $(PKG_CONFIG_LOCAL)
 
 libxenvchan.so: libxenvchan.so.$(MAJOR)
 	ln -sf $< $@
@@ -59,13 +60,18 @@ vchan-node1: $(NODE_OBJS) libxenvchan.so
 vchan-node2: $(NODE2_OBJS) libxenvchan.so
 	$(CC) $(LDFLAGS) -o $@ $(NODE2_OBJS) $(LDLIBS_libxenvchan) $(APPEND_LDFLAGS)
 
+vchan-socket-proxy: vchan-socket-proxy.o libxenvchan.so
+	$(CC) $(LDFLAGS) -o $@ $< $(LDLIBS_libxenvchan) $(LDLIBS_libxenstore) $(LDLIBS_libxenctrl) $(APPEND_LDFLAGS)
+
 .PHONY: install
 install: all
 	$(INSTALL_DIR) $(DESTDIR)$(libdir)
 	$(INSTALL_DIR) $(DESTDIR)$(includedir)
+	$(INSTALL_DIR) $(DESTDIR)$(bindir)
 	$(INSTALL_PROG) libxenvchan.so.$(MAJOR).$(MINOR) $(DESTDIR)$(libdir)
 	ln -sf libxenvchan.so.$(MAJOR).$(MINOR) $(DESTDIR)$(libdir)/libxenvchan.so.$(MAJOR)
 	ln -sf libxenvchan.so.$(MAJOR) $(DESTDIR)$(libdir)/libxenvchan.so
+	$(INSTALL_PROG) vchan-socket-proxy $(DESTDIR)$(bindir)
 	$(INSTALL_DATA) libxenvchan.h $(DESTDIR)$(includedir)
 	$(INSTALL_DATA) libxenvchan.a $(DESTDIR)$(libdir)
 	$(INSTALL_DATA) xenvchan.pc $(DESTDIR)$(PKG_INSTALLDIR)
diff --git a/tools/libvchan/vchan-socket-proxy.c b/tools/libvchan/vchan-socket-proxy.c
new file mode 100644
index 0000000000..13700c5d67
--- /dev/null
+++ b/tools/libvchan/vchan-socket-proxy.c
@@ -0,0 +1,478 @@
+/**
+ * @file
+ * @section AUTHORS
+ *
+ * Copyright (C) 2010  Rafal Wojtczuk  <rafal@invisiblethingslab.com>
+ *
+ *  Authors:
+ *       Rafal Wojtczuk  <rafal@invisiblethingslab.com>
+ *       Daniel De Graaf <dgdegra@tycho.nsa.gov>
+ *       Marek Marczykowski-Górecki  <marmarek@invisiblethingslab.com>
+ *
+ * @section LICENSE
+ *
+ *  This program is free software; you can redistribute it and/or
+ *  modify it under the terms of the GNU Lesser General Public
+ *  License as published by the Free Software Foundation; either
+ *  version 2.1 of the License, or (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ *  Lesser General Public License for more details.
+ *
+ *  You should have received a copy of the GNU Lesser General Public
+ *  License along with this program; If not, see <http://www.gnu.org/licenses/>.
+ *
+ * @section DESCRIPTION
+ *
+ * This is a vchan to unix socket proxy. Vchan server is set, and on client
+ * connection, local socket connection is established. Communication is bidirectional.
+ * One client is served at a time, clients needs to coordinate this themselves.
+ */
+
+#include <stdlib.h>
+#include <stdio.h>
+#include <string.h>
+#include <unistd.h>
+#include <fcntl.h>
+#include <errno.h>
+#include <sys/socket.h>
+#include <sys/un.h>
+#include <getopt.h>
+
+#include <xenstore.h>
+#include <xenctrl.h>
+#include <libxenvchan.h>
+
+static void usage(char** argv)
+{
+    fprintf(stderr, "usage:\n"
+        "\t%s [options] domainid nodepath [socket-path|file-no|-]\n"
+        "\n"
+        "options:\n"
+        "\t-m, --mode=client|server - vchan connection mode (client by default)\n"
+        "\t-s, --state-path=path - xenstore path where write \"running\" to \n"
+        "\t                        at startup\n"
+        "\t-v, --verbose - verbose logging\n"
+        "\n"
+        "client: client of a vchan connection, fourth parameter can be:\n"
+        "\tsocket-path: listen on a UNIX socket at this path and connect to vchan\n"
+        "\t             whenever new connection is accepted;\n"
+        "\t             handle multiple _subsequent_ connections, until terminated\n"
+        "\n"
+        "\tfile-no:     except open FD of a socket in listen mode;\n"
+        "\t             otherwise similar to socket-path\n"
+        "\n"
+        "\t-:           open vchan connection immediately and pass the data\n"
+        "\t             from stdin/stdout; terminate when vchan connection\n"
+        "\t             is closed\n"
+        "\n"
+        "server: server of a vchan connection, fourth parameter can be:\n"
+        "\tsocket-path: connect to this UNIX socket when new vchan connection\n"
+        "\t             is accepted;\n"
+        "\t             handle multiple _subsequent_ connections, until terminated\n"
+        "\n"
+        "\tfile-no:     pass data to/from this FD; terminate when vchan connection\n"
+        "\t             is closed\n"
+        "\n"
+        "\t-:           pass data to/from stdin/stdout; terminate when vchan\n"
+        "\t             connection is closed\n",
+        argv[0]);
+    exit(1);
+}
+
+#define BUFSIZE 8192
+char inbuf[BUFSIZE];
+char outbuf[BUFSIZE];
+int insiz = 0;
+int outsiz = 0;
+int verbose = 0;
+
+static void vchan_wr(struct libxenvchan *ctrl) {
+    int ret;
+
+    if (!insiz)
+        return;
+    ret = libxenvchan_write(ctrl, inbuf, insiz);
+    if (ret < 0) {
+        fprintf(stderr, "vchan write failed\n");
+        exit(1);
+    }
+    if (verbose)
+        fprintf(stderr, "wrote %d bytes to vchan\n", ret);
+    if (ret > 0) {
+        insiz -= ret;
+        memmove(inbuf, inbuf + ret, insiz);
+    }
+}
+
+static void socket_wr(int output_fd) {
+    int ret;
+
+    if (!outsiz)
+        return;
+    ret = write(output_fd, outbuf, outsiz);
+    if (ret < 0 && errno != EAGAIN)
+        exit(1);
+    if (ret > 0) {
+        outsiz -= ret;
+        memmove(outbuf, outbuf + ret, outsiz);
+    }
+}
+
+static int set_nonblocking(int fd, int nonblocking) {
+    int flags = fcntl(fd, F_GETFL);
+    if (flags == -1)
+        return -1;
+
+    if (nonblocking)
+        flags |= O_NONBLOCK;
+    else
+        flags &= ~O_NONBLOCK;
+
+    if (fcntl(fd, F_SETFL, flags) == -1)
+        return -1;
+
+    return 0;
+}
+
+static int connect_socket(const char *path_or_fd) {
+    int fd;
+    char *endptr;
+    struct sockaddr_un addr;
+
+    fd = strtoll(path_or_fd, &endptr, 0);
+    if (*endptr == '\0') {
+        set_nonblocking(fd, 1);
+        return fd;
+    }
+
+    fd = socket(AF_UNIX, SOCK_STREAM, 0);
+    if (fd == -1)
+        return -1;
+
+    addr.sun_family = AF_UNIX;
+    strncpy(addr.sun_path, path_or_fd, sizeof(addr.sun_path));
+    if (connect(fd, (const struct sockaddr *)&addr, sizeof(addr)) == -1) {
+        close(fd);
+        return -1;
+    }
+
+    set_nonblocking(fd, 1);
+
+    return fd;
+}
+
+static int listen_socket(const char *path_or_fd) {
+    int fd;
+    char *endptr;
+    struct sockaddr_un addr;
+
+    fd = strtoll(path_or_fd, &endptr, 0);
+    if (*endptr == '\0') {
+        return fd;
+    }
+
+    /* if not a number, assume a socket path */
+    fd = socket(AF_UNIX, SOCK_STREAM, 0);
+    if (fd == -1)
+        return -1;
+
+    addr.sun_family = AF_UNIX;
+    strncpy(addr.sun_path, path_or_fd, sizeof(addr.sun_path));
+    if (bind(fd, (const struct sockaddr *)&addr, sizeof(addr)) == -1) {
+        close(fd);
+        return -1;
+    }
+    if (listen(fd, 5) != 0) {
+        close(fd);
+        return -1;
+    }
+
+    return fd;
+}
+
+static struct libxenvchan *connect_vchan(int domid, const char *path) {
+    struct libxenvchan *ctrl = NULL;
+    struct xs_handle *xs = NULL;
+    xc_interface *xc = NULL;
+    xc_dominfo_t dominfo;
+    char **watch_ret;
+    unsigned int watch_num;
+    int ret;
+
+    xs = xs_open(XS_OPEN_READONLY);
+    if (!xs) {
+        perror("xs_open");
+        goto out;
+    }
+    xc = xc_interface_open(NULL, NULL, XC_OPENFLAG_NON_REENTRANT);
+    if (!xc) {
+        perror("xc_interface_open");
+        goto out;
+    }
+    /* wait for vchan server to create *path* */
+    xs_watch(xs, path, "path");
+    xs_watch(xs, "@releaseDomain", "release");
+    while ((watch_ret = xs_read_watch(xs, &watch_num))) {
+        /* don't care about exact which fired the watch */
+        free(watch_ret);
+        ctrl = libxenvchan_client_init(NULL, domid, path);
+        if (ctrl)
+            break;
+
+        ret = xc_domain_getinfo(xc, domid, 1, &dominfo);
+        /* break the loop if domain is definitely not there anymore, but
+         * continue if it is or the call failed (like EPERM) */
+        if (ret == -1 && errno == ESRCH)
+            break;
+        if (ret == 1 && (dominfo.domid != (uint32_t)domid || dominfo.dying))
+            break;
+    }
+
+out:
+    if (xc)
+        xc_interface_close(xc);
+    if (xs)
+        xs_close(xs);
+    return ctrl;
+}
+
+
+static void discard_buffers(struct libxenvchan *ctrl) {
+    /* discard local buffers */
+    insiz = 0;
+    outsiz = 0;
+
+    /* discard remaining incoming data */
+    while (libxenvchan_data_ready(ctrl)) {
+        if (libxenvchan_read(ctrl, inbuf, BUFSIZE) == -1) {
+            perror("vchan read");
+            exit(1);
+        }
+    }
+}
+
+int data_loop(struct libxenvchan *ctrl, int input_fd, int output_fd)
+{
+    int ret;
+    int libxenvchan_fd;
+    int max_fd;
+
+    libxenvchan_fd = libxenvchan_fd_for_select(ctrl);
+    for (;;) {
+        fd_set rfds;
+        fd_set wfds;
+        FD_ZERO(&rfds);
+        FD_ZERO(&wfds);
+
+        max_fd = -1;
+        if (input_fd != -1 && insiz != BUFSIZE) {
+            FD_SET(input_fd, &rfds);
+            if (input_fd > max_fd)
+                max_fd = input_fd;
+        }
+        if (output_fd != -1 && outsiz) {
+            FD_SET(output_fd, &wfds);
+            if (output_fd > max_fd)
+                max_fd = output_fd;
+        }
+        FD_SET(libxenvchan_fd, &rfds);
+        if (libxenvchan_fd > max_fd)
+            max_fd = libxenvchan_fd;
+        ret = select(max_fd + 1, &rfds, &wfds, NULL, NULL);
+        if (ret < 0) {
+            perror("select");
+            exit(1);
+        }
+        if (FD_ISSET(libxenvchan_fd, &rfds)) {
+            libxenvchan_wait(ctrl);
+            if (!libxenvchan_is_open(ctrl)) {
+                if (verbose)
+                    fprintf(stderr, "vchan client disconnected\n");
+                while (outsiz)
+                    socket_wr(output_fd);
+                close(output_fd);
+                close(input_fd);
+                discard_buffers(ctrl);
+                break;
+            }
+            vchan_wr(ctrl);
+        }
+
+        if (FD_ISSET(input_fd, &rfds)) {
+            ret = read(input_fd, inbuf + insiz, BUFSIZE - insiz);
+            if (ret < 0 && errno != EAGAIN)
+                exit(1);
+            if (verbose)
+                fprintf(stderr, "from-unix: %.*s\n", ret, inbuf + insiz);
+            if (ret == 0) {
+                /* EOF on socket, write everything in the buffer and close the
+                 * input_fd socket */
+                while (insiz) {
+                    vchan_wr(ctrl);
+                    libxenvchan_wait(ctrl);
+                }
+                close(input_fd);
+                input_fd = -1;
+                /* TODO: maybe signal the vchan client somehow? */
+                break;
+            }
+            if (ret)
+                insiz += ret;
+            vchan_wr(ctrl);
+        }
+        if (FD_ISSET(output_fd, &wfds))
+            socket_wr(output_fd);
+        while (libxenvchan_data_ready(ctrl) && outsiz < BUFSIZE) {
+            ret = libxenvchan_read(ctrl, outbuf + outsiz, BUFSIZE - outsiz);
+            if (ret < 0)
+                exit(1);
+            if (verbose)
+                fprintf(stderr, "from-vchan: %.*s\n", ret, outbuf + outsiz);
+            outsiz += ret;
+            socket_wr(output_fd);
+        }
+    }
+    return 0;
+}
+
+/**
+    Simple libxenvchan application, both client and server.
+    Both sides may write and read, both from the libxenvchan and from
+    stdin/stdout (just like netcat).
+*/
+
+static struct option options[] = {
+    { "mode",       required_argument, NULL, 'm' },
+    { "verbose",          no_argument, NULL, 'v' },
+    { "state-path", required_argument, NULL, 's' },
+    { }
+};
+
+int main(int argc, char **argv)
+{
+    int is_server = 0;
+    int socket_fd = -1;
+    int input_fd, output_fd;
+    struct libxenvchan *ctrl = NULL;
+    const char *socket_path;
+    int domid;
+    const char *vchan_path;
+    const char *state_path = NULL;
+    int opt;
+
+    while ((opt = getopt_long(argc, argv, "m:vs:", options, NULL)) != -1) {
+        switch (opt) {
+            case 'm':
+                if (strcmp(optarg, "server") == 0)
+                    is_server = 1;
+                else if (strcmp(optarg, "client") == 0)
+                    is_server = 0;
+                else {
+                    fprintf(stderr, "invalid argument for --mode: %s\n", optarg);
+                    usage(argv);
+                    return 1;
+                }
+                break;
+            case 'v':
+                verbose = 1;
+                break;
+            case 's':
+                state_path = optarg;
+                break;
+            case '?':
+                usage(argv);
+        }
+    }
+
+    if (argc-optind != 3)
+        usage(argv);
+
+    domid = atoi(argv[optind]);
+    vchan_path = argv[optind+1];
+    socket_path = argv[optind+2];
+
+    if (is_server) {
+        ctrl = libxenvchan_server_init(NULL, domid, vchan_path, 0, 0);
+        if (!ctrl) {
+            perror("libxenvchan_server_init");
+            exit(1);
+        }
+    } else {
+        if (strcmp(socket_path, "-") == 0) {
+            input_fd = 0;
+            output_fd = 1;
+        } else {
+            socket_fd = listen_socket(socket_path);
+            if (socket_fd == -1) {
+                perror("listen socket");
+                return 1;
+            }
+        }
+    }
+
+    if (state_path) {
+        struct xs_handle *xs;
+
+        xs = xs_open(0);
+        if (!xs) {
+            perror("xs_open");
+            return 1;
+        }
+        if (!xs_write(xs, XBT_NULL, state_path, "running", strlen("running"))) {
+            perror("xs_write");
+            return 1;
+        }
+        xs_close(xs);
+    }
+
+    for (;;) {
+        if (is_server) {
+            /* wait for vchan connection */
+            while (libxenvchan_is_open(ctrl) != 1)
+                libxenvchan_wait(ctrl);
+            /* vchan client connected, setup local FD if needed */
+            if (strcmp(socket_path, "-") == 0) {
+                input_fd = 0;
+                output_fd = 1;
+            } else {
+                input_fd = output_fd = connect_socket(socket_path);
+            }
+            if (input_fd == -1) {
+                perror("connect socket");
+                return 1;
+            }
+            if (data_loop(ctrl, input_fd, output_fd) != 0)
+                break;
+            /* keep it running only when get UNIX socket path */
+            if (socket_path[0] != '/')
+                break;
+        } else {
+            /* wait for local socket connection */
+            if (strcmp(socket_path, "-") != 0)
+                input_fd = output_fd = accept(socket_fd, NULL, NULL);
+            if (input_fd == -1) {
+                perror("accept");
+                return 1;
+            }
+            set_nonblocking(input_fd, 1);
+            set_nonblocking(output_fd, 1);
+            ctrl = connect_vchan(domid, vchan_path);
+            if (!ctrl) {
+                perror("vchan client init");
+                return 1;
+            }
+            if (data_loop(ctrl, input_fd, output_fd) != 0)
+                break;
+            /* don't reconnect if output was stdout */
+            if (strcmp(socket_path, "-") == 0)
+                break;
+
+            libxenvchan_close(ctrl);
+            ctrl = NULL;
+        }
+    }
+    return 0;
+}
-- 
2.25.1



^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH v6 12/18] libxl: Refactor kill_device_model to libxl__kill_xs_path
  2020-05-18  1:13 [PATCH v6 00/18] Add support for qemu-xen runnning in a Linux-based stubdomain Jason Andryuk
                   ` (10 preceding siblings ...)
  2020-05-18  1:13 ` [PATCH v6 11/18] tools: add simple vchan-socket-proxy Jason Andryuk
@ 2020-05-18  1:13 ` Jason Andryuk
  2020-05-18  1:13 ` [PATCH v6 13/18] libxl: use vchan for QMP access with Linux stubdomain Jason Andryuk
                   ` (6 subsequent siblings)
  18 siblings, 0 replies; 28+ messages in thread
From: Jason Andryuk @ 2020-05-18  1:13 UTC (permalink / raw)
  To: xen-devel
  Cc: Anthony PERARD, Ian Jackson, Ian Jackson, Wei Liu, Jason Andryuk

Move kill_device_model to libxl__kill_xs_path so we have a helper to
kill a process from a pid stored in xenstore.  We'll be using it to kill
vchan-qmp-proxy.

libxl__kill_xs_path takes a "what" string for use in printing error
messages.  kill_device_model is retained in libxl_dm.c to provide the
string.

Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

---
Changes in v6:
 - Add Acked-by: Ian Jackson
---
 tools/libxl/libxl_aoutils.c  | 32 ++++++++++++++++++++++++++++++++
 tools/libxl/libxl_dm.c       | 27 +--------------------------
 tools/libxl/libxl_internal.h |  3 +++
 3 files changed, 36 insertions(+), 26 deletions(-)

diff --git a/tools/libxl/libxl_aoutils.c b/tools/libxl/libxl_aoutils.c
index 1be858c93c..c4c095a5ba 100644
--- a/tools/libxl/libxl_aoutils.c
+++ b/tools/libxl/libxl_aoutils.c
@@ -626,6 +626,38 @@ void libxl__kill(libxl__gc *gc, pid_t pid, int sig, const char *what)
                 what, (unsigned long)pid, sig);
 }
 
+/* Generic function to signal (HUP) a pid stored in xenstore */
+int libxl__kill_xs_path(libxl__gc *gc, const char *xs_path_pid,
+                        const char *what)
+{
+    const char *xs_pid;
+    int ret, pid;
+
+    ret = libxl__xs_read_checked(gc, XBT_NULL, xs_path_pid, &xs_pid);
+    if (ret || !xs_pid) {
+        LOG(ERROR, "unable to find %s pid in %s", what, xs_path_pid);
+        ret = ret ? : ERROR_FAIL;
+        goto out;
+    }
+    pid = atoi(xs_pid);
+
+    ret = kill(pid, SIGHUP);
+    if (ret < 0 && errno == ESRCH) {
+        LOG(ERROR, "%s already exited", what);
+        ret = 0;
+    } else if (ret == 0) {
+        LOG(DEBUG, "%s signaled", what);
+        ret = 0;
+    } else {
+        LOGE(ERROR, "failed to kill %s [%d]", what, pid);
+        ret = ERROR_FAIL;
+        goto out;
+    }
+
+out:
+    return ret;
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index a4f8866d33..478e6540df 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -3235,32 +3235,7 @@ out:
 /* Generic function to signal a Qemu instance to exit */
 static int kill_device_model(libxl__gc *gc, const char *xs_path_pid)
 {
-    const char *xs_pid;
-    int ret, pid;
-
-    ret = libxl__xs_read_checked(gc, XBT_NULL, xs_path_pid, &xs_pid);
-    if (ret || !xs_pid) {
-        LOG(ERROR, "unable to find device model pid in %s", xs_path_pid);
-        ret = ret ? : ERROR_FAIL;
-        goto out;
-    }
-    pid = atoi(xs_pid);
-
-    ret = kill(pid, SIGHUP);
-    if (ret < 0 && errno == ESRCH) {
-        LOG(ERROR, "Device Model already exited");
-        ret = 0;
-    } else if (ret == 0) {
-        LOG(DEBUG, "Device Model signaled");
-        ret = 0;
-    } else {
-        LOGE(ERROR, "failed to kill Device Model [%d]", pid);
-        ret = ERROR_FAIL;
-        goto out;
-    }
-
-out:
-    return ret;
+    return libxl__kill_xs_path(gc, xs_path_pid, "Device Model");
 }
 
 /* Helper to destroy a Qdisk backend */
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index f2f76439ec..c939557b2e 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -2711,6 +2711,9 @@ int libxl__async_exec_start(libxl__async_exec_state *aes);
 bool libxl__async_exec_inuse(const libxl__async_exec_state *aes);
 
 _hidden void libxl__kill(libxl__gc *gc, pid_t pid, int sig, const char *what);
+/* kill SIGHUP a pid stored in xenstore */
+_hidden int libxl__kill_xs_path(libxl__gc *gc, const char *xs_path_pid,
+                                const char *what);
 
 /*----- device addition/removal -----*/
 
-- 
2.25.1



^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH v6 13/18] libxl: use vchan for QMP access with Linux stubdomain
  2020-05-18  1:13 [PATCH v6 00/18] Add support for qemu-xen runnning in a Linux-based stubdomain Jason Andryuk
                   ` (11 preceding siblings ...)
  2020-05-18  1:13 ` [PATCH v6 12/18] libxl: Refactor kill_device_model to libxl__kill_xs_path Jason Andryuk
@ 2020-05-18  1:13 ` Jason Andryuk
  2020-05-18 14:27   ` Ian Jackson
  2020-05-18  1:13 ` [PATCH v6 14/18] libxl: require qemu in dom0 for multiple stubdomain consoles Jason Andryuk
                   ` (5 subsequent siblings)
  18 siblings, 1 reply; 28+ messages in thread
From: Jason Andryuk @ 2020-05-18  1:13 UTC (permalink / raw)
  To: xen-devel
  Cc: Wei Liu, Jason Andryuk, Ian Jackson,
	Marek Marczykowski-Górecki, Ian Jackson, Anthony PERARD,
	Samuel Thibault

From: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>

Access to QMP of QEMU in Linux stubdomain is possible over vchan
connection. Handle the actual vchan connection in a separate process
(vchan-socket-proxy). This simplified integration with QMP (already
quite complex), but also allows preliminary filtering of (potentially
malicious) QMP input.
Since only one client can be connected to vchan server at the same time
and it is not enforced by the libxenvchan itself, additional client-side
locking is needed. It is implicitly implemented by vchan-socket-proxy,
as it handle only one connection at a time. Note that qemu supports only
one simultaneous client on a control socket anyway (but in UNIX socket
case, it enforce it server-side), so it doesn't add any extra
limitation.

libxl qmp client code already has locking to handle concurrent access
attempts to the same qemu qmp interface.

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

Squash in changes of regenerated autotools files.

Kill the vchan-socket-proxy so we don't leak the daemonized processes.
libxl__stubdomain_is_linux_running() works against the guest_domid, but
the xenstore path is beneath the stubdomain.  This leads to the use of
libxl_is_stubdom in addition to libxl__stubdomain_is_linux_running() so
that the stubdomain calls kill for the qmp-proxy.

Also call libxl__qmp_cleanup() to remove the unix sockets used by
vchan-socket-proxy.  vchan-socket-proxy only creates qmp-libxl-$domid,
and libxl__qmp_cleanup removes that as well as qmp-libxenstat-$domid.
However, it tolerates ENOENT, and a stray qmp-libxenstat-$domid should
not exist.

Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
---

Re-run autotools after applying.

Changes in v4:
 - new patch, in place of both "libxl: use vchan for QMP access ..."
Changes in v5:
 - Use device-model/%u/qmp-proxy-state xenstore path
 - Rephrase comment
Changes in v6
 - Commit message mention libxl locking
 - Mention re-run autotools
 - Squashed in re-generated autotools files
 - Call libxl__qmp_cleanup() to remove unix socket.
 - Cleanup in vchan-socket-proxy is dropped.

Ian, you acked the original and the squashed in "libxl: Kill
vchan-socket-proxy when cleaning up qmp".  However, I also added the
libxl__qmp_cleanup() call, so I did not retain your Ack.  That change is
at the end in dm_destroy_cb().

---
 configure                    |  14 +--
 docs/configure               |  14 +--
 stubdom/configure            |  14 +--
 tools/config.h.in            |   3 +
 tools/configure              |  46 ++++++----
 tools/configure.ac           |   9 ++
 tools/libxl/libxl_dm.c       | 163 +++++++++++++++++++++++++++++++++--
 tools/libxl/libxl_domain.c   |  10 +++
 tools/libxl/libxl_internal.h |   1 +
 9 files changed, 209 insertions(+), 65 deletions(-)

diff --git a/configure b/configure
index 9da3970cef..8af54e8a5a 100755
--- a/configure
+++ b/configure
@@ -644,7 +644,6 @@ infodir
 docdir
 oldincludedir
 includedir
-runstatedir
 localstatedir
 sharedstatedir
 sysconfdir
@@ -723,7 +722,6 @@ datadir='${datarootdir}'
 sysconfdir='${prefix}/etc'
 sharedstatedir='${prefix}/com'
 localstatedir='${prefix}/var'
-runstatedir='${localstatedir}/run'
 includedir='${prefix}/include'
 oldincludedir='/usr/include'
 docdir='${datarootdir}/doc/${PACKAGE_TARNAME}'
@@ -976,15 +974,6 @@ do
   | -silent | --silent | --silen | --sile | --sil)
     silent=yes ;;
 
-  -runstatedir | --runstatedir | --runstatedi | --runstated \
-  | --runstate | --runstat | --runsta | --runst | --runs \
-  | --run | --ru | --r)
-    ac_prev=runstatedir ;;
-  -runstatedir=* | --runstatedir=* | --runstatedi=* | --runstated=* \
-  | --runstate=* | --runstat=* | --runsta=* | --runst=* | --runs=* \
-  | --run=* | --ru=* | --r=*)
-    runstatedir=$ac_optarg ;;
-
   -sbindir | --sbindir | --sbindi | --sbind | --sbin | --sbi | --sb)
     ac_prev=sbindir ;;
   -sbindir=* | --sbindir=* | --sbindi=* | --sbind=* | --sbin=* \
@@ -1122,7 +1111,7 @@ fi
 for ac_var in	exec_prefix prefix bindir sbindir libexecdir datarootdir \
 		datadir sysconfdir sharedstatedir localstatedir includedir \
 		oldincludedir docdir infodir htmldir dvidir pdfdir psdir \
-		libdir localedir mandir runstatedir
+		libdir localedir mandir
 do
   eval ac_val=\$$ac_var
   # Remove trailing slashes.
@@ -1275,7 +1264,6 @@ Fine tuning of the installation directories:
   --sysconfdir=DIR        read-only single-machine data [PREFIX/etc]
   --sharedstatedir=DIR    modifiable architecture-independent data [PREFIX/com]
   --localstatedir=DIR     modifiable single-machine data [PREFIX/var]
-  --runstatedir=DIR       modifiable per-process data [LOCALSTATEDIR/run]
   --libdir=DIR            object code libraries [EPREFIX/lib]
   --includedir=DIR        C header files [PREFIX/include]
   --oldincludedir=DIR     C header files for non-gcc [/usr/include]
diff --git a/docs/configure b/docs/configure
index 9e3ed60462..93e9dcf404 100755
--- a/docs/configure
+++ b/docs/configure
@@ -634,7 +634,6 @@ infodir
 docdir
 oldincludedir
 includedir
-runstatedir
 localstatedir
 sharedstatedir
 sysconfdir
@@ -711,7 +710,6 @@ datadir='${datarootdir}'
 sysconfdir='${prefix}/etc'
 sharedstatedir='${prefix}/com'
 localstatedir='${prefix}/var'
-runstatedir='${localstatedir}/run'
 includedir='${prefix}/include'
 oldincludedir='/usr/include'
 docdir='${datarootdir}/doc/${PACKAGE_TARNAME}'
@@ -964,15 +962,6 @@ do
   | -silent | --silent | --silen | --sile | --sil)
     silent=yes ;;
 
-  -runstatedir | --runstatedir | --runstatedi | --runstated \
-  | --runstate | --runstat | --runsta | --runst | --runs \
-  | --run | --ru | --r)
-    ac_prev=runstatedir ;;
-  -runstatedir=* | --runstatedir=* | --runstatedi=* | --runstated=* \
-  | --runstate=* | --runstat=* | --runsta=* | --runst=* | --runs=* \
-  | --run=* | --ru=* | --r=*)
-    runstatedir=$ac_optarg ;;
-
   -sbindir | --sbindir | --sbindi | --sbind | --sbin | --sbi | --sb)
     ac_prev=sbindir ;;
   -sbindir=* | --sbindir=* | --sbindi=* | --sbind=* | --sbin=* \
@@ -1110,7 +1099,7 @@ fi
 for ac_var in	exec_prefix prefix bindir sbindir libexecdir datarootdir \
 		datadir sysconfdir sharedstatedir localstatedir includedir \
 		oldincludedir docdir infodir htmldir dvidir pdfdir psdir \
-		libdir localedir mandir runstatedir
+		libdir localedir mandir
 do
   eval ac_val=\$$ac_var
   # Remove trailing slashes.
@@ -1263,7 +1252,6 @@ Fine tuning of the installation directories:
   --sysconfdir=DIR        read-only single-machine data [PREFIX/etc]
   --sharedstatedir=DIR    modifiable architecture-independent data [PREFIX/com]
   --localstatedir=DIR     modifiable single-machine data [PREFIX/var]
-  --runstatedir=DIR       modifiable per-process data [LOCALSTATEDIR/run]
   --libdir=DIR            object code libraries [EPREFIX/lib]
   --includedir=DIR        C header files [PREFIX/include]
   --oldincludedir=DIR     C header files for non-gcc [/usr/include]
diff --git a/stubdom/configure b/stubdom/configure
index da03da535a..f7604a37f7 100755
--- a/stubdom/configure
+++ b/stubdom/configure
@@ -661,7 +661,6 @@ infodir
 docdir
 oldincludedir
 includedir
-runstatedir
 localstatedir
 sharedstatedir
 sysconfdir
@@ -751,7 +750,6 @@ datadir='${datarootdir}'
 sysconfdir='${prefix}/etc'
 sharedstatedir='${prefix}/com'
 localstatedir='${prefix}/var'
-runstatedir='${localstatedir}/run'
 includedir='${prefix}/include'
 oldincludedir='/usr/include'
 docdir='${datarootdir}/doc/${PACKAGE_TARNAME}'
@@ -1004,15 +1002,6 @@ do
   | -silent | --silent | --silen | --sile | --sil)
     silent=yes ;;
 
-  -runstatedir | --runstatedir | --runstatedi | --runstated \
-  | --runstate | --runstat | --runsta | --runst | --runs \
-  | --run | --ru | --r)
-    ac_prev=runstatedir ;;
-  -runstatedir=* | --runstatedir=* | --runstatedi=* | --runstated=* \
-  | --runstate=* | --runstat=* | --runsta=* | --runst=* | --runs=* \
-  | --run=* | --ru=* | --r=*)
-    runstatedir=$ac_optarg ;;
-
   -sbindir | --sbindir | --sbindi | --sbind | --sbin | --sbi | --sb)
     ac_prev=sbindir ;;
   -sbindir=* | --sbindir=* | --sbindi=* | --sbind=* | --sbin=* \
@@ -1150,7 +1139,7 @@ fi
 for ac_var in	exec_prefix prefix bindir sbindir libexecdir datarootdir \
 		datadir sysconfdir sharedstatedir localstatedir includedir \
 		oldincludedir docdir infodir htmldir dvidir pdfdir psdir \
-		libdir localedir mandir runstatedir
+		libdir localedir mandir
 do
   eval ac_val=\$$ac_var
   # Remove trailing slashes.
@@ -1303,7 +1292,6 @@ Fine tuning of the installation directories:
   --sysconfdir=DIR        read-only single-machine data [PREFIX/etc]
   --sharedstatedir=DIR    modifiable architecture-independent data [PREFIX/com]
   --localstatedir=DIR     modifiable single-machine data [PREFIX/var]
-  --runstatedir=DIR       modifiable per-process data [LOCALSTATEDIR/run]
   --libdir=DIR            object code libraries [EPREFIX/lib]
   --includedir=DIR        C header files [PREFIX/include]
   --oldincludedir=DIR     C header files for non-gcc [/usr/include]
diff --git a/tools/config.h.in b/tools/config.h.in
index 5a5944ebe1..5abf6092de 100644
--- a/tools/config.h.in
+++ b/tools/config.h.in
@@ -123,6 +123,9 @@
 /* Define to 1 if you have the ANSI C header files. */
 #undef STDC_HEADERS
 
+/* QMP proxy path */
+#undef STUBDOM_QMP_PROXY_PATH
+
 /* Enable large inode numbers on Mac OS X 10.5.  */
 #ifndef _DARWIN_USE_64_BIT_INODE
 # define _DARWIN_USE_64_BIT_INODE 1
diff --git a/tools/configure b/tools/configure
index 36596389b8..35036dc1db 100755
--- a/tools/configure
+++ b/tools/configure
@@ -772,7 +772,6 @@ infodir
 docdir
 oldincludedir
 includedir
-runstatedir
 localstatedir
 sharedstatedir
 sysconfdir
@@ -814,6 +813,7 @@ with_linux_backend_modules
 enable_qemu_traditional
 enable_rombios
 with_system_qemu
+with_stubdom_qmp_proxy
 with_system_seabios
 with_system_ovmf
 enable_ipxe
@@ -899,7 +899,6 @@ datadir='${datarootdir}'
 sysconfdir='${prefix}/etc'
 sharedstatedir='${prefix}/com'
 localstatedir='${prefix}/var'
-runstatedir='${localstatedir}/run'
 includedir='${prefix}/include'
 oldincludedir='/usr/include'
 docdir='${datarootdir}/doc/${PACKAGE_TARNAME}'
@@ -1152,15 +1151,6 @@ do
   | -silent | --silent | --silen | --sile | --sil)
     silent=yes ;;
 
-  -runstatedir | --runstatedir | --runstatedi | --runstated \
-  | --runstate | --runstat | --runsta | --runst | --runs \
-  | --run | --ru | --r)
-    ac_prev=runstatedir ;;
-  -runstatedir=* | --runstatedir=* | --runstatedi=* | --runstated=* \
-  | --runstate=* | --runstat=* | --runsta=* | --runst=* | --runs=* \
-  | --run=* | --ru=* | --r=*)
-    runstatedir=$ac_optarg ;;
-
   -sbindir | --sbindir | --sbindi | --sbind | --sbin | --sbi | --sb)
     ac_prev=sbindir ;;
   -sbindir=* | --sbindir=* | --sbindi=* | --sbind=* | --sbin=* \
@@ -1298,7 +1288,7 @@ fi
 for ac_var in	exec_prefix prefix bindir sbindir libexecdir datarootdir \
 		datadir sysconfdir sharedstatedir localstatedir includedir \
 		oldincludedir docdir infodir htmldir dvidir pdfdir psdir \
-		libdir localedir mandir runstatedir
+		libdir localedir mandir
 do
   eval ac_val=\$$ac_var
   # Remove trailing slashes.
@@ -1451,7 +1441,6 @@ Fine tuning of the installation directories:
   --sysconfdir=DIR        read-only single-machine data [PREFIX/etc]
   --sharedstatedir=DIR    modifiable architecture-independent data [PREFIX/com]
   --localstatedir=DIR     modifiable single-machine data [PREFIX/var]
-  --runstatedir=DIR       modifiable per-process data [LOCALSTATEDIR/run]
   --libdir=DIR            object code libraries [EPREFIX/lib]
   --includedir=DIR        C header files [PREFIX/include]
   --oldincludedir=DIR     C header files for non-gcc [/usr/include]
@@ -1535,6 +1524,9 @@ Optional Packages:
                           Use system supplied qemu PATH or qemu (taken from
                           $PATH) as qemu-xen device model instead of building
                           and installing our own version
+  --stubdom-qmp-proxy[=PATH]
+                          Use supplied binary PATH as a QMP proxy into
+                          stubdomain
   --with-system-seabios[=PATH]
                           Use system supplied seabios PATH instead of building
                           and installing our own version
@@ -3382,7 +3374,7 @@ else
     We can't simply define LARGE_OFF_T to be 9223372036854775807,
     since some C++ compilers masquerading as C compilers
     incorrectly reject 9223372036854775807.  */
-#define LARGE_OFF_T ((((off_t) 1 << 31) << 31) - 1 + (((off_t) 1 << 31) << 31))
+#define LARGE_OFF_T (((off_t) 1 << 62) - 1 + ((off_t) 1 << 62))
   int off_t_is_large[(LARGE_OFF_T % 2147483629 == 721
 		       && LARGE_OFF_T % 2147483647 == 1)
 		      ? 1 : -1];
@@ -3428,7 +3420,7 @@ else
     We can't simply define LARGE_OFF_T to be 9223372036854775807,
     since some C++ compilers masquerading as C compilers
     incorrectly reject 9223372036854775807.  */
-#define LARGE_OFF_T ((((off_t) 1 << 31) << 31) - 1 + (((off_t) 1 << 31) << 31))
+#define LARGE_OFF_T (((off_t) 1 << 62) - 1 + ((off_t) 1 << 62))
   int off_t_is_large[(LARGE_OFF_T % 2147483629 == 721
 		       && LARGE_OFF_T % 2147483647 == 1)
 		      ? 1 : -1];
@@ -3452,7 +3444,7 @@ rm -f core conftest.err conftest.$ac_objext conftest.$ac_ext
     We can't simply define LARGE_OFF_T to be 9223372036854775807,
     since some C++ compilers masquerading as C compilers
     incorrectly reject 9223372036854775807.  */
-#define LARGE_OFF_T ((((off_t) 1 << 31) << 31) - 1 + (((off_t) 1 << 31) << 31))
+#define LARGE_OFF_T (((off_t) 1 << 62) - 1 + ((off_t) 1 << 62))
   int off_t_is_large[(LARGE_OFF_T % 2147483629 == 721
 		       && LARGE_OFF_T % 2147483647 == 1)
 		      ? 1 : -1];
@@ -3497,7 +3489,7 @@ else
     We can't simply define LARGE_OFF_T to be 9223372036854775807,
     since some C++ compilers masquerading as C compilers
     incorrectly reject 9223372036854775807.  */
-#define LARGE_OFF_T ((((off_t) 1 << 31) << 31) - 1 + (((off_t) 1 << 31) << 31))
+#define LARGE_OFF_T (((off_t) 1 << 62) - 1 + ((off_t) 1 << 62))
   int off_t_is_large[(LARGE_OFF_T % 2147483629 == 721
 		       && LARGE_OFF_T % 2147483647 == 1)
 		      ? 1 : -1];
@@ -3521,7 +3513,7 @@ rm -f core conftest.err conftest.$ac_objext conftest.$ac_ext
     We can't simply define LARGE_OFF_T to be 9223372036854775807,
     since some C++ compilers masquerading as C compilers
     incorrectly reject 9223372036854775807.  */
-#define LARGE_OFF_T ((((off_t) 1 << 31) << 31) - 1 + (((off_t) 1 << 31) << 31))
+#define LARGE_OFF_T (((off_t) 1 << 62) - 1 + ((off_t) 1 << 62))
   int off_t_is_large[(LARGE_OFF_T % 2147483629 == 721
 		       && LARGE_OFF_T % 2147483647 == 1)
 		      ? 1 : -1];
@@ -4548,6 +4540,24 @@ _ACEOF
 
 
 
+# Check whether --with-stubdom-qmp-proxy was given.
+if test "${with_stubdom_qmp_proxy+set}" = set; then :
+  withval=$with_stubdom_qmp_proxy;
+    stubdom_qmp_proxy="$withval"
+
+else
+
+    stubdom_qmp_proxy="$bindir/vchan-socket-proxy"
+
+fi
+
+
+cat >>confdefs.h <<_ACEOF
+#define STUBDOM_QMP_PROXY_PATH "$stubdom_qmp_proxy"
+_ACEOF
+
+
+
 # Check whether --with-system-seabios was given.
 if test "${with_system_seabios+set}" = set; then :
   withval=$with_system_seabios;
diff --git a/tools/configure.ac b/tools/configure.ac
index b6f8882be4..a9af0a21c6 100644
--- a/tools/configure.ac
+++ b/tools/configure.ac
@@ -194,6 +194,15 @@ AC_SUBST(qemu_xen)
 AC_SUBST(qemu_xen_path)
 AC_SUBST(qemu_xen_systemd)
 
+AC_ARG_WITH([stubdom-qmp-proxy],
+    AC_HELP_STRING([--stubdom-qmp-proxy@<:@=PATH@:>@],
+        [Use supplied binary PATH as a QMP proxy into stubdomain]),[
+    stubdom_qmp_proxy="$withval"
+],[
+    stubdom_qmp_proxy="$bindir/vchan-socket-proxy"
+])
+AC_DEFINE_UNQUOTED([STUBDOM_QMP_PROXY_PATH], ["$stubdom_qmp_proxy"], [QMP proxy path])
+
 AC_ARG_WITH([system-seabios],
     AS_HELP_STRING([--with-system-seabios@<:@=PATH@:>@],
        [Use system supplied seabios PATH instead of building and installing
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 478e6540df..c66e8ebc24 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -1200,7 +1200,11 @@ static int libxl__build_device_model_args_new(libxl__gc *gc,
                       GCSPRINTF("%d", guest_domid), NULL);
     flexarray_append(dm_args, "-no-shutdown");
 
-    /* There is currently no way to access the QMP socket in the stubdom */
+    /*
+     * QMP access to qemu running in stubdomain is done over vchan. The
+     * stubdomain init script adds the appropriate monitor options for
+     * vchan-socket-proxy.
+     */
     if (!is_stubdom) {
         flexarray_append(dm_args, "-chardev");
         if (state->dm_monitor_fd >= 0) {
@@ -2214,6 +2218,23 @@ static void stubdom_pvqemu_unpaused(libxl__egc *egc,
 static void stubdom_xswait_cb(libxl__egc *egc, libxl__xswait_state *xswait,
                               int rc, const char *p);
 
+static void spawn_qmp_proxy(libxl__egc *egc,
+                            libxl__stub_dm_spawn_state *sdss);
+
+static void qmp_proxy_confirm(libxl__egc *egc, libxl__spawn_state *spawn,
+                              const char *xsdata);
+
+static void qmp_proxy_startup_failed(libxl__egc *egc,
+                                     libxl__spawn_state *spawn,
+                                     int rc);
+
+static void qmp_proxy_detached(libxl__egc *egc,
+                               libxl__spawn_state *spawn);
+
+static void qmp_proxy_spawn_outcome(libxl__egc *egc,
+                                    libxl__stub_dm_spawn_state *sdss,
+                                    int rc);
+
 char *libxl__stub_dm_name(libxl__gc *gc, const char *guest_name)
 {
     return GCSPRINTF("%s-dm", guest_name);
@@ -2496,24 +2517,150 @@ static void spawn_stub_launch_dm(libxl__egc *egc,
             goto out;
     }
 
+    sdss->qmp_proxy_spawn.ao = ao;
+    if (libxl__stubdomain_is_linux(&guest_config->b_info)) {
+        spawn_qmp_proxy(egc, sdss);
+    } else {
+        qmp_proxy_spawn_outcome(egc, sdss, 0);
+    }
+
+    return;
+
+out:
+    assert(ret);
+    qmp_proxy_spawn_outcome(egc, sdss, ret);
+}
+
+static void spawn_qmp_proxy(libxl__egc *egc,
+                            libxl__stub_dm_spawn_state *sdss)
+{
+    STATE_AO_GC(sdss->qmp_proxy_spawn.ao);
+    const uint32_t guest_domid = sdss->dm.guest_domid;
+    const uint32_t dm_domid = sdss->pvqemu.guest_domid;
+    const char *dom_path = libxl__xs_get_dompath(gc, dm_domid);
+    char **args;
+    int nr = 0;
+    int rc, logfile_w, null;
+
+    if (access(STUBDOM_QMP_PROXY_PATH, X_OK) < 0) {
+        LOGED(ERROR, guest_domid, "qmp proxy %s is not executable", STUBDOM_QMP_PROXY_PATH);
+        rc = ERROR_FAIL;
+        goto out;
+    }
+
+    sdss->qmp_proxy_spawn.what = GCSPRINTF("domain %d device model qmp proxy", guest_domid);
+    sdss->qmp_proxy_spawn.pidpath = GCSPRINTF("%s/image/qmp-proxy-pid", dom_path);
+    sdss->qmp_proxy_spawn.xspath = DEVICE_MODEL_XS_PATH(gc, LIBXL_TOOLSTACK_DOMID,
+                                                        dm_domid, "/qmp-proxy-state");
+    sdss->qmp_proxy_spawn.timeout_ms = LIBXL_DEVICE_MODEL_START_TIMEOUT * 1000;
+    sdss->qmp_proxy_spawn.midproc_cb = libxl__spawn_record_pid;
+    sdss->qmp_proxy_spawn.confirm_cb = qmp_proxy_confirm;
+    sdss->qmp_proxy_spawn.failure_cb = qmp_proxy_startup_failed;
+    sdss->qmp_proxy_spawn.detached_cb = qmp_proxy_detached;
+
+    const int arraysize = 6;
+    GCNEW_ARRAY(args, arraysize);
+    args[nr++] = STUBDOM_QMP_PROXY_PATH;
+    args[nr++] = GCSPRINTF("--state-path=%s", sdss->qmp_proxy_spawn.xspath);
+    args[nr++] = GCSPRINTF("%u", dm_domid);
+    args[nr++] = GCSPRINTF("%s/device-model/%u/qmp-vchan", dom_path, guest_domid);
+    args[nr++] = (char*)libxl__qemu_qmp_path(gc, guest_domid);
+    args[nr++] = NULL;
+    assert(nr == arraysize);
+
+    logfile_w = libxl__create_qemu_logfile(gc, GCSPRINTF("qmp-proxy-%s",
+                                                         sdss->dm_config.c_info.name));
+    if (logfile_w < 0) {
+        rc = logfile_w;
+        goto out;
+    }
+    null = open("/dev/null", O_RDWR);
+    if (null < 0) {
+        LOGED(ERROR, guest_domid, "unable to open /dev/null");
+        rc = ERROR_FAIL;
+        goto out_close;
+    }
+
+    rc = libxl__spawn_spawn(egc, &sdss->qmp_proxy_spawn);
+    if (rc < 0)
+        goto out_close;
+    if (!rc) { /* inner child */
+        setsid();
+        libxl__exec(gc, null, null, logfile_w, STUBDOM_QMP_PROXY_PATH, args, NULL);
+        /* unreachable */
+    }
+
+    rc = 0;
+
+out_close:
+    if (logfile_w >= 0)
+        close(logfile_w);
+    if (null >= 0)
+        close(null);
+out:
+    if (rc)
+        qmp_proxy_spawn_outcome(egc, sdss, rc);
+}
+
+static void qmp_proxy_confirm(libxl__egc *egc, libxl__spawn_state *spawn,
+                              const char *xsdata)
+{
+    STATE_AO_GC(spawn->ao);
+
+    if (!xsdata)
+        return;
+
+    if (strcmp(xsdata, "running"))
+        return;
+
+    libxl__spawn_initiate_detach(gc, spawn);
+}
+
+static void qmp_proxy_startup_failed(libxl__egc *egc,
+                                     libxl__spawn_state *spawn,
+                                     int rc)
+{
+    libxl__stub_dm_spawn_state *sdss = CONTAINER_OF(spawn, *sdss, qmp_proxy_spawn);
+    qmp_proxy_spawn_outcome(egc, sdss, rc);
+}
+
+static void qmp_proxy_detached(libxl__egc *egc,
+                               libxl__spawn_state *spawn)
+{
+    libxl__stub_dm_spawn_state *sdss = CONTAINER_OF(spawn, *sdss, qmp_proxy_spawn);
+    qmp_proxy_spawn_outcome(egc, sdss, 0);
+}
+
+static void qmp_proxy_spawn_outcome(libxl__egc *egc,
+                                    libxl__stub_dm_spawn_state *sdss,
+                                    int rc)
+{
+    STATE_AO_GC(sdss->qmp_proxy_spawn.ao);
+    int need_pvqemu = libxl__need_xenpv_qemu(gc, &sdss->dm_config);
+
+    if (rc) goto out;
+
+    if (need_pvqemu < 0) {
+        rc = need_pvqemu;
+        goto out;
+    }
+
     sdss->pvqemu.spawn.ao = ao;
-    sdss->pvqemu.guest_domid = dm_domid;
     sdss->pvqemu.guest_config = &sdss->dm_config;
     sdss->pvqemu.build_state = &sdss->dm_state;
     sdss->pvqemu.callback = spawn_stubdom_pvqemu_cb;
-
-    if (!need_qemu) {
+    if (need_pvqemu) {
+        libxl__spawn_local_dm(egc, &sdss->pvqemu);
+    } else {
         /* If dom0 qemu not needed, do not launch it */
         spawn_stubdom_pvqemu_cb(egc, &sdss->pvqemu, 0);
-    } else {
-        libxl__spawn_local_dm(egc, &sdss->pvqemu);
     }
 
     return;
 
 out:
-    assert(ret);
-    spawn_stubdom_pvqemu_cb(egc, &sdss->pvqemu, ret);
+    assert(rc);
+    spawn_stubdom_pvqemu_cb(egc, &sdss->pvqemu, rc);
 }
 
 static void spawn_stubdom_pvqemu_cb(libxl__egc *egc,
diff --git a/tools/libxl/libxl_domain.c b/tools/libxl/libxl_domain.c
index fef2cd4e13..c08af308fa 100644
--- a/tools/libxl/libxl_domain.c
+++ b/tools/libxl/libxl_domain.c
@@ -1260,10 +1260,20 @@ static void dm_destroy_cb(libxl__egc *egc,
     libxl__destroy_domid_state *dis = CONTAINER_OF(ddms, *dis, ddms);
     STATE_AO_GC(dis->ao);
     uint32_t domid = dis->domid;
+    uint32_t target_domid;
 
     if (rc < 0)
         LOGD(ERROR, domid, "libxl__destroy_device_model failed");
 
+    if (libxl_is_stubdom(CTX, domid, &target_domid) &&
+        libxl__stubdomain_is_linux_running(gc, target_domid)) {
+        char *path = GCSPRINTF("/local/domain/%d/image/qmp-proxy-pid", domid);
+
+        libxl__kill_xs_path(gc, path, "QMP Proxy");
+        /* qmp-proxy for stubdom registers target_domid's QMP sockets. */
+        libxl__qmp_cleanup(gc, target_domid);
+    }
+
     dis->drs.ao = ao;
     dis->drs.domid = domid;
     dis->drs.callback = devices_destroy_cb;
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index c939557b2e..41b51b07cd 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -4166,6 +4166,7 @@ typedef struct {
     libxl__destroy_domid_state dis;
     libxl__multidev multidev;
     libxl__xswait_state xswait;
+    libxl__spawn_state qmp_proxy_spawn;
 } libxl__stub_dm_spawn_state;
 
 _hidden void libxl__spawn_stub_dm(libxl__egc *egc, libxl__stub_dm_spawn_state*);
-- 
2.25.1



^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH v6 14/18] libxl: require qemu in dom0 for multiple stubdomain consoles
  2020-05-18  1:13 [PATCH v6 00/18] Add support for qemu-xen runnning in a Linux-based stubdomain Jason Andryuk
                   ` (12 preceding siblings ...)
  2020-05-18  1:13 ` [PATCH v6 13/18] libxl: use vchan for QMP access with Linux stubdomain Jason Andryuk
@ 2020-05-18  1:13 ` Jason Andryuk
  2020-05-18 14:27   ` Ian Jackson
  2020-05-18  1:13 ` [PATCH v6 15/18] libxl: ignore emulated IDE disks beyond the first 4 Jason Andryuk
                   ` (4 subsequent siblings)
  18 siblings, 1 reply; 28+ messages in thread
From: Jason Andryuk @ 2020-05-18  1:13 UTC (permalink / raw)
  To: xen-devel
  Cc: Wei Liu, Jason Andryuk, Ian Jackson,
	Marek Marczykowski-Górecki, Anthony PERARD, Ian Jackson

From: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>

Device model stubdomains (both Mini-OS + qemu-trad and linux + qemu-xen)
are always started with at least 3 consoles: log, save, and restore.
Until xenconsoled learns how to handle multiple consoles, this is needed
for save/restore support.

For Mini-OS stubdoms, this is a bug.  In practice, it works in most
cases because there is something else that triggers qemu in dom0 too:
vfb/vkb added if vnc/sdl/spice is enabled.

Additionally, Linux-based stubdomain waits for all the backends to
initialize during boot. Lack of some console backends results in
stubdomain startup timeout.

This is a temporary patch until xenconsoled will be improved.

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
[Updated commit message with Marek's explanation from mailing list.]
Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
---
Changes in v6
 - Update commit message
---
 tools/libxl/libxl_dm.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index c66e8ebc24..fb87deae91 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -2504,7 +2504,11 @@ static void spawn_stub_launch_dm(libxl__egc *egc,
         }
     }
 
-    need_qemu = libxl__need_xenpv_qemu(gc, dm_config);
+    /*
+     * Until xenconsoled learns how to handle multiple consoles, require qemu
+     * in dom0 to serve consoles for a stubdomain - it require at least 3 of them.
+     */
+    need_qemu = 1 || libxl__need_xenpv_qemu(gc, &sdss->dm_config);
 
     for (i = 0; i < num_console; i++) {
         libxl__device device;
@@ -2636,7 +2640,11 @@ static void qmp_proxy_spawn_outcome(libxl__egc *egc,
                                     int rc)
 {
     STATE_AO_GC(sdss->qmp_proxy_spawn.ao);
-    int need_pvqemu = libxl__need_xenpv_qemu(gc, &sdss->dm_config);
+    /*
+     * Until xenconsoled learns how to handle multiple consoles, require qemu
+     * in dom0 to serve consoles for a stubdomain - it require at least 3 of them.
+     */
+    int need_pvqemu = 1 || libxl__need_xenpv_qemu(gc, &sdss->dm_config);
 
     if (rc) goto out;
 
-- 
2.25.1



^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH v6 15/18] libxl: ignore emulated IDE disks beyond the first 4
  2020-05-18  1:13 [PATCH v6 00/18] Add support for qemu-xen runnning in a Linux-based stubdomain Jason Andryuk
                   ` (13 preceding siblings ...)
  2020-05-18  1:13 ` [PATCH v6 14/18] libxl: require qemu in dom0 for multiple stubdomain consoles Jason Andryuk
@ 2020-05-18  1:13 ` Jason Andryuk
  2020-05-18  1:13 ` [PATCH v6 16/18] libxl: consider also qemu in stubdomain in libxl__dm_active check Jason Andryuk
                   ` (3 subsequent siblings)
  18 siblings, 0 replies; 28+ messages in thread
From: Jason Andryuk @ 2020-05-18  1:13 UTC (permalink / raw)
  To: xen-devel
  Cc: Wei Liu, Jason Andryuk, Ian Jackson,
	Marek Marczykowski-Górecki, Anthony PERARD, Ian Jackson

From: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>

Qemu supports only 4 emulated IDE disks, when given more (or with higher
indexes), it will fail to start. Since the disks can still be accessible
using PV interface, just ignore emulated path and log a warning, instead
of rejecting the configuration altogether.

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
Reviewed-by: Jason Andryuk <jandryuk@gmail.com>
Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

---
Changes in v6:
 - Add Acked-by: Ian Jackson
---
 tools/libxl/libxl_dm.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index fb87deae91..3356880346 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -1894,6 +1894,13 @@ static int libxl__build_device_model_args_new(libxl__gc *gc,
             }
 
             if (disks[i].is_cdrom) {
+                if (disk > 4) {
+                    LOGD(WARN, guest_domid, "Emulated CDROM can be only one of the first 4 disks.\n"
+                         "Disk %s will be available via PV drivers but not as an "
+                         "emulated disk.",
+                         disks[i].vdev);
+                    continue;
+                }
                 drive = libxl__sprintf(gc,
                          "if=ide,index=%d,readonly=on,media=cdrom,id=ide-%i",
                          disk, dev_number);
@@ -1971,6 +1978,10 @@ static int libxl__build_device_model_args_new(libxl__gc *gc,
                                                        &disks[i],
                                                        colo_mode);
                 } else {
+                    LOGD(WARN, guest_domid, "Only 4 emulated IDE disks are supported.\n"
+                         "Disk %s will be available via PV drivers but not as an "
+                         "emulated disk.",
+                         disks[i].vdev);
                     continue; /* Do not emulate this disk */
                 }
 
-- 
2.25.1



^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH v6 16/18] libxl: consider also qemu in stubdomain in libxl__dm_active check
  2020-05-18  1:13 [PATCH v6 00/18] Add support for qemu-xen runnning in a Linux-based stubdomain Jason Andryuk
                   ` (14 preceding siblings ...)
  2020-05-18  1:13 ` [PATCH v6 15/18] libxl: ignore emulated IDE disks beyond the first 4 Jason Andryuk
@ 2020-05-18  1:13 ` Jason Andryuk
  2020-05-18  1:13 ` [PATCH v6 17/18] docs: Add device-model-domid to xenstore-paths Jason Andryuk
                   ` (2 subsequent siblings)
  18 siblings, 0 replies; 28+ messages in thread
From: Jason Andryuk @ 2020-05-18  1:13 UTC (permalink / raw)
  To: xen-devel
  Cc: Wei Liu, Jason Andryuk, Ian Jackson,
	Marek Marczykowski-Górecki, Anthony PERARD, Ian Jackson

From: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>

Since qemu-xen can now run in stubdomain too, handle this case when
checking it's state too.

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
Reviewed-by: Jason Andryuk <jandryuk@gmail.com>
Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

---
Changes in v6:
 - Add Acked-by: Ian Jackson
---
 tools/libxl/libxl_dm.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 3356880346..098dc49ecb 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -3744,12 +3744,18 @@ out:
 
 int libxl__dm_active(libxl__gc *gc, uint32_t domid)
 {
-    char *pid, *path;
+    char *pid, *dm_domid, *path;
 
     path = GCSPRINTF("/local/domain/%d/image/device-model-pid", domid);
     pid = libxl__xs_read(gc, XBT_NULL, path);
 
-    return pid != NULL;
+    if (pid)
+        return true;
+
+    path = GCSPRINTF("/local/domain/%d/image/device-model-domid", domid);
+    dm_domid = libxl__xs_read(gc, XBT_NULL, path);
+
+    return dm_domid != NULL;
 }
 
 int libxl__dm_check_start(libxl__gc *gc, libxl_domain_config *d_config,
-- 
2.25.1



^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH v6 17/18] docs: Add device-model-domid to xenstore-paths
  2020-05-18  1:13 [PATCH v6 00/18] Add support for qemu-xen runnning in a Linux-based stubdomain Jason Andryuk
                   ` (15 preceding siblings ...)
  2020-05-18  1:13 ` [PATCH v6 16/18] libxl: consider also qemu in stubdomain in libxl__dm_active check Jason Andryuk
@ 2020-05-18  1:13 ` Jason Andryuk
  2020-05-18  1:13 ` [PATCH v6 18/18] libxl: Check stubdomain kernel & ramdisk presence Jason Andryuk
  2020-05-18 14:28 ` [PATCH v6 00/18] Add support for qemu-xen runnning in a Linux-based stubdomain Ian Jackson
  18 siblings, 0 replies; 28+ messages in thread
From: Jason Andryuk @ 2020-05-18  1:13 UTC (permalink / raw)
  To: xen-devel
  Cc: Stefano Stabellini, Julien Grall, Wei Liu, Jason Andryuk,
	Andrew Cooper, Ian Jackson, George Dunlap, Jan Beulich,
	Ian Jackson

Document device-model-domid for when using a device model stubdomain.

Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

---
Changes in v6:
 - Add Acked-by: Ian Jackson
---
 docs/misc/xenstore-paths.pandoc | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/docs/misc/xenstore-paths.pandoc b/docs/misc/xenstore-paths.pandoc
index a152f5ea68..766e8008dc 100644
--- a/docs/misc/xenstore-paths.pandoc
+++ b/docs/misc/xenstore-paths.pandoc
@@ -148,6 +148,11 @@ The domain's own ID.
 The process ID of the device model associated with this domain, if it
 has one.
 
+#### ~/image/device-model-domid = INTEGER   [INTERNAL]
+
+The domain ID of the device model stubdomain associated with this domain,
+if it has one.
+
 #### ~/cpu/[0-9]+/availability = ("online"|"offline") [PV]
 
 One node for each virtual CPU up to the guest's configured
-- 
2.25.1



^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH v6 18/18] libxl: Check stubdomain kernel & ramdisk presence
  2020-05-18  1:13 [PATCH v6 00/18] Add support for qemu-xen runnning in a Linux-based stubdomain Jason Andryuk
                   ` (16 preceding siblings ...)
  2020-05-18  1:13 ` [PATCH v6 17/18] docs: Add device-model-domid to xenstore-paths Jason Andryuk
@ 2020-05-18  1:13 ` Jason Andryuk
  2020-05-18 14:28 ` [PATCH v6 00/18] Add support for qemu-xen runnning in a Linux-based stubdomain Ian Jackson
  18 siblings, 0 replies; 28+ messages in thread
From: Jason Andryuk @ 2020-05-18  1:13 UTC (permalink / raw)
  To: xen-devel
  Cc: Anthony PERARD, Ian Jackson, Ian Jackson, Wei Liu, Jason Andryuk

Just out of context is the following comment for libxl__domain_make:
/* fixme: this function can leak the stubdom if it fails */

When the stubdomain kernel or ramdisk is not present, the domid and
stubdomain name will indeed be leaked.  Avoid the leak by checking the
file presence and erroring out when absent.  It doesn't fix all cases,
but it avoids a big one when using a linux device model stubdomain.

Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

---
Changes in v6:
 - Add Acked-by: Ian Jackson
---
 tools/libxl/libxl_dm.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 098dc49ecb..997c4815e0 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -2336,6 +2336,22 @@ void libxl__spawn_stub_dm(libxl__egc *egc, libxl__stub_dm_spawn_state *sdss)
         dm_config->num_vkbs = 1;
     }
 
+    if (guest_config->b_info.stubdomain_kernel &&
+        access(guest_config->b_info.stubdomain_kernel, R_OK) != 0) {
+        LOGED(ERROR, guest_domid, "could not access stubdomain kernel %s",
+              guest_config->b_info.stubdomain_kernel);
+        ret = ERROR_INVAL;
+        goto out;
+    }
+
+    if (guest_config->b_info.stubdomain_ramdisk &&
+        access(guest_config->b_info.stubdomain_ramdisk, R_OK) != 0) {
+        LOGED(ERROR, guest_domid, "could not access stubdomain ramdisk %s",
+              guest_config->b_info.stubdomain_ramdisk);
+        ret = ERROR_INVAL;
+        goto out;
+    }
+
     stubdom_state->pv_kernel.path = guest_config->b_info.stubdomain_kernel;
     stubdom_state->pv_ramdisk.path = guest_config->b_info.stubdomain_ramdisk;
 
-- 
2.25.1



^ permalink raw reply related	[flat|nested] 28+ messages in thread

* Re: [PATCH v6 06/18] libxl: write qemu arguments into separate xenstore keys
  2020-05-18  1:13 ` [PATCH v6 06/18] libxl: write qemu arguments into separate xenstore keys Jason Andryuk
@ 2020-05-18 14:19   ` Ian Jackson
  2020-05-18 15:20     ` Jason Andryuk
  0 siblings, 1 reply; 28+ messages in thread
From: Ian Jackson @ 2020-05-18 14:19 UTC (permalink / raw)
  To: Jason Andryuk
  Cc: Anthony Perard, xen-devel, Marek Marczykowski-Górecki, Wei Liu

Jason Andryuk writes ("[PATCH v6 06/18] libxl: write qemu arguments into separate xenstore keys"):
> From: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
...
> +static int libxl__write_stub_linux_dm_argv(libxl__gc *gc,
> +                                           int dm_domid, int guest_domid,
> +                                           char **args)
> +{

Thanks for the changes.

> +    xs_transaction_t t = XBT_NULL;
...
> +    rc = libxl__xs_read_mandatory(gc, XBT_NULL,
> +                                  GCSPRINTF("/local/domain/%d/vm", guest_domid),
> +                                  &vm_path);
> +    if (rc)
> +        return rc;

I think this should be "goto out".  That conforms to standard libxl
error handling discipline and avoids future leak bugs etc.
libxl__xs_transaction_abort is a no-op with t==NULL.

Also, it is not clear to me why you chose to put this outside the
transaction loop.  Can you either put it inside the transaction loop,
or produce an explanation as to why this approach is race-free...

Thanks,
Ian.


^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v6 09/18] libxl: add save/restore support for qemu-xen in stubdomain
  2020-05-18  1:13 ` [PATCH v6 09/18] libxl: add save/restore support for qemu-xen in stubdomain Jason Andryuk
@ 2020-05-18 14:24   ` Ian Jackson
  2020-05-18 15:30     ` Jason Andryuk
  0 siblings, 1 reply; 28+ messages in thread
From: Ian Jackson @ 2020-05-18 14:24 UTC (permalink / raw)
  To: Jason Andryuk
  Cc: Anthony Perard, xen-devel, Marek Marczykowski-Górecki, Wei Liu

Jason Andryuk writes ("[PATCH v6 09/18] libxl: add save/restore support for qemu-xen in stubdomain"):
> From: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
...
>      if (state->saved_state) {
> -        /* This file descriptor is meant to be used by QEMU */
> -        *dm_state_fd = open(state->saved_state, O_RDONLY);
> -        flexarray_append(dm_args, "-incoming");
> -        flexarray_append(dm_args, GCSPRINTF("fd:%d",*dm_state_fd));
> +        if (is_stubdom) {
> +            /* Linux stubdomain must replace $STUBDOM_RESTORE_INCOMING_ARG
> +             * with the approriate fd:$num argument for the
> +             * STUBDOM_CONSOLE_RESTORE console 2.
> +             */
> +            flexarray_append(dm_args, "-incoming");
> +            flexarray_append(dm_args, "$STUBDOM_RESTORE_INCOMING_ARG");
> +        } else {
> +            /* This file descriptor is meant to be used by QEMU */
> +            *dm_state_fd = open(state->saved_state, O_RDONLY);
> +            flexarray_append(dm_args, "-incoming");
> +            flexarray_append(dm_args, GCSPRINTF("fd:%d",*dm_state_fd));

Hrk.  The stubdom script is expected to spot this particular value in
the dm_args array and seddery it.  OK.  This is, at leasst, sound.
I'm happy with the code and the protocol.

I think this needs a change to this doc:

  Subject: [PATCH v6 01/18] Document ioemu MiniOS stubdomain protocol

  +Toolstack to MiniOS ioemu stubdomain protocol
  +---------------------------------------------

Provided that you update the docs commit and take my ack off that,
please add my ack to this code :-).

Or you can fold the docs change into this commit, if you prefer, in
which case I'll review this patch again.

Thanks,
Ian.


^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v6 13/18] libxl: use vchan for QMP access with Linux stubdomain
  2020-05-18  1:13 ` [PATCH v6 13/18] libxl: use vchan for QMP access with Linux stubdomain Jason Andryuk
@ 2020-05-18 14:27   ` Ian Jackson
  0 siblings, 0 replies; 28+ messages in thread
From: Ian Jackson @ 2020-05-18 14:27 UTC (permalink / raw)
  To: Jason Andryuk
  Cc: Anthony Perard, xen-devel, Marek Marczykowski-Górecki,
	Wei Liu, Samuel Thibault

Jason Andryuk writes ("[PATCH v6 13/18] libxl: use vchan for QMP access with Linux stubdomain"):
> Ian, you acked the original and the squashed in "libxl: Kill
> vchan-socket-proxy when cleaning up qmp".  However, I also added the
> libxl__qmp_cleanup() call, so I did not retain your Ack.  That change is
> at the end in dm_destroy_cb().

Thanks.  That's appropriate.  Thanks for drawing my attention to it.

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>


^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v6 14/18] libxl: require qemu in dom0 for multiple stubdomain consoles
  2020-05-18  1:13 ` [PATCH v6 14/18] libxl: require qemu in dom0 for multiple stubdomain consoles Jason Andryuk
@ 2020-05-18 14:27   ` Ian Jackson
  0 siblings, 0 replies; 28+ messages in thread
From: Ian Jackson @ 2020-05-18 14:27 UTC (permalink / raw)
  To: Jason Andryuk
  Cc: Anthony Perard, xen-devel, Marek Marczykowski-Górecki, Wei Liu

Jason Andryuk writes ("[PATCH v6 14/18] libxl: require qemu in dom0 for multiple stubdomain consoles"):
> From: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> 
> Device model stubdomains (both Mini-OS + qemu-trad and linux + qemu-xen)
> are always started with at least 3 consoles: log, save, and restore.
> Until xenconsoled learns how to handle multiple consoles, this is needed
> for save/restore support.
> 
> For Mini-OS stubdoms, this is a bug.  In practice, it works in most
> cases because there is something else that triggers qemu in dom0 too:
> vfb/vkb added if vnc/sdl/spice is enabled.
> 
> Additionally, Linux-based stubdomain waits for all the backends to
> initialize during boot. Lack of some console backends results in
> stubdomain startup timeout.
> 
> This is a temporary patch until xenconsoled will be improved.

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>


^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v6 00/18] Add support for qemu-xen runnning in a Linux-based stubdomain
  2020-05-18  1:13 [PATCH v6 00/18] Add support for qemu-xen runnning in a Linux-based stubdomain Jason Andryuk
                   ` (17 preceding siblings ...)
  2020-05-18  1:13 ` [PATCH v6 18/18] libxl: Check stubdomain kernel & ramdisk presence Jason Andryuk
@ 2020-05-18 14:28 ` Ian Jackson
  18 siblings, 0 replies; 28+ messages in thread
From: Ian Jackson @ 2020-05-18 14:28 UTC (permalink / raw)
  To: Jason Andryuk
  Cc: Stefano Stabellini, Julien Grall, Wei Liu, Andrew Cooper,
	George Dunlap, Jan Beulich, Samuel Thibault, Anthony Perard,
	xen-devel, Daniel De Graaf

Jason Andryuk writes ("[PATCH v6 00/18] Add support for qemu-xen runnning in a Linux-based stubdomain"):
> In coordination with Marek, I'm making a submission of his patches for Linux
> stubdomain device-model support.  I made a few of my own additions, but Marek
> did the heavy lifting.  Thank you, Marek.

Hi.  I've gone through this version and there is little left to do.  I
look forward to committing this this week...

Ian.


^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v6 06/18] libxl: write qemu arguments into separate xenstore keys
  2020-05-18 14:19   ` Ian Jackson
@ 2020-05-18 15:20     ` Jason Andryuk
  2020-05-18 16:34       ` Ian Jackson
  0 siblings, 1 reply; 28+ messages in thread
From: Jason Andryuk @ 2020-05-18 15:20 UTC (permalink / raw)
  To: Ian Jackson
  Cc: Anthony Perard, xen-devel, Marek Marczykowski-Górecki, Wei Liu

On Mon, May 18, 2020 at 10:20 AM Ian Jackson <ian.jackson@citrix.com> wrote:
>
> Jason Andryuk writes ("[PATCH v6 06/18] libxl: write qemu arguments into separate xenstore keys"):
> > From: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> ...
> > +static int libxl__write_stub_linux_dm_argv(libxl__gc *gc,
> > +                                           int dm_domid, int guest_domid,
> > +                                           char **args)
> > +{
>
> Thanks for the changes.
>
> > +    xs_transaction_t t = XBT_NULL;
> ...
> > +    rc = libxl__xs_read_mandatory(gc, XBT_NULL,
> > +                                  GCSPRINTF("/local/domain/%d/vm", guest_domid),
> > +                                  &vm_path);
> > +    if (rc)
> > +        return rc;
>
> I think this should be "goto out".  That conforms to standard libxl
> error handling discipline and avoids future leak bugs etc.
> libxl__xs_transaction_abort is a no-op with t==NULL.
>
> Also, it is not clear to me why you chose to put this outside the
> transaction loop.  Can you either put it inside the transaction loop,
> or produce an explanation as to why this approach is race-free...

I just matched the old code's transaction only around the write.  "vm"
shouldn't change during runtime, but I can make the changes as you
suggest.

-Jason


^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v6 09/18] libxl: add save/restore support for qemu-xen in stubdomain
  2020-05-18 14:24   ` Ian Jackson
@ 2020-05-18 15:30     ` Jason Andryuk
  2020-05-18 16:29       ` Ian Jackson
  0 siblings, 1 reply; 28+ messages in thread
From: Jason Andryuk @ 2020-05-18 15:30 UTC (permalink / raw)
  To: Ian Jackson
  Cc: Anthony Perard, xen-devel, Marek Marczykowski-Górecki, Wei Liu

On Mon, May 18, 2020 at 10:24 AM Ian Jackson <ian.jackson@citrix.com> wrote:
>
> Jason Andryuk writes ("[PATCH v6 09/18] libxl: add save/restore support for qemu-xen in stubdomain"):
> > From: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> ...
> >      if (state->saved_state) {
> > -        /* This file descriptor is meant to be used by QEMU */
> > -        *dm_state_fd = open(state->saved_state, O_RDONLY);
> > -        flexarray_append(dm_args, "-incoming");
> > -        flexarray_append(dm_args, GCSPRINTF("fd:%d",*dm_state_fd));
> > +        if (is_stubdom) {
> > +            /* Linux stubdomain must replace $STUBDOM_RESTORE_INCOMING_ARG
> > +             * with the approriate fd:$num argument for the
> > +             * STUBDOM_CONSOLE_RESTORE console 2.
> > +             */
> > +            flexarray_append(dm_args, "-incoming");
> > +            flexarray_append(dm_args, "$STUBDOM_RESTORE_INCOMING_ARG");
> > +        } else {
> > +            /* This file descriptor is meant to be used by QEMU */
> > +            *dm_state_fd = open(state->saved_state, O_RDONLY);
> > +            flexarray_append(dm_args, "-incoming");
> > +            flexarray_append(dm_args, GCSPRINTF("fd:%d",*dm_state_fd));
>
> Hrk.  The stubdom script is expected to spot this particular value in
> the dm_args array and seddery it.  OK.  This is, at leasst, sound.
> I'm happy with the code and the protocol.
>
> I think this needs a change to this doc:
>
>   Subject: [PATCH v6 01/18] Document ioemu MiniOS stubdomain protocol
>
>   +Toolstack to MiniOS ioemu stubdomain protocol
>   +---------------------------------------------
>
> Provided that you update the docs commit and take my ack off that,
> please add my ack to this code :-).

I updated "[PATCH v6 02/18] Document ioemu Linux stubdomain protocol"
to mention $STUBDOM_RESTORE_INCOMING_ARG as well as the xenstore
directory change to "dm-argv" in this v6, but I left your Ack on it.
Sorry about that.  I'll remove your Ack from 02/18 when I post v7,
but I'll add the Ack to this 09/18.

-Jason


^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v6 09/18] libxl: add save/restore support for qemu-xen in stubdomain
  2020-05-18 15:30     ` Jason Andryuk
@ 2020-05-18 16:29       ` Ian Jackson
  0 siblings, 0 replies; 28+ messages in thread
From: Ian Jackson @ 2020-05-18 16:29 UTC (permalink / raw)
  To: Jason Andryuk
  Cc: Anthony Perard, xen-devel, Marek Marczykowski-Górecki, Wei Liu

Jason Andryuk writes ("Re: [PATCH v6 09/18] libxl: add save/restore support for qemu-xen in stubdomain"):
> On Mon, May 18, 2020 at 10:24 AM Ian Jackson <ian.jackson@citrix.com> wrote:
> > Provided that you update the docs commit and take my ack off that,
> > please add my ack to this code :-).
> 
> I updated "[PATCH v6 02/18] Document ioemu Linux stubdomain protocol"
> to mention $STUBDOM_RESTORE_INCOMING_ARG as well as the xenstore
> directory change to "dm-argv" in this v6, but I left your Ack on it.
> Sorry about that.  I'll remove your Ack from 02/18 when I post v7,
> but I'll add the Ack to this 09/18.

Oh, that's why I didn't see that docs change.  I went to look what you
wrote in v6 02/18.  It LGTM.  Please put my ack back :-).

Thanks,
Ian.


^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v6 06/18] libxl: write qemu arguments into separate xenstore keys
  2020-05-18 15:20     ` Jason Andryuk
@ 2020-05-18 16:34       ` Ian Jackson
  0 siblings, 0 replies; 28+ messages in thread
From: Ian Jackson @ 2020-05-18 16:34 UTC (permalink / raw)
  To: Jason Andryuk
  Cc: Anthony Perard, xen-devel, Marek Marczykowski-Górecki, Wei Liu

Jason Andryuk writes ("Re: [PATCH v6 06/18] libxl: write qemu arguments into separate xenstore keys"):
> On Mon, May 18, 2020 at 10:20 AM Ian Jackson <ian.jackson@citrix.com> wrote:
> > I think this should be "goto out".  That conforms to standard libxl
> > error handling discipline and avoids future leak bugs etc.
> > libxl__xs_transaction_abort is a no-op with t==NULL.
> >
> > Also, it is not clear to me why you chose to put this outside the
> > transaction loop.  Can you either put it inside the transaction loop,
> > or produce an explanation as to why this approach is race-free...
> 
> I just matched the old code's transaction only around the write.  "vm"
> shouldn't change during runtime, but I can make the changes as you
> suggest.

Ah I see.  I hadn't spotted this duplication.

As there is only one caller of libxl__write_stub_dmargs the messing
about with %d/vm and the transaction and so on could be factored out
and only the actual arg processing made conditional.

I would prefer that, to be honest.  But I don't want to derail this
series at this point by asking you to take on refactorings that I
ought to have asked for sooner.

So I'll take it if you make the new code the way I like it, as I
suggest above.  Maybe it will be refactored later (perhaps even by
me...)

Regards,
Ian.


^ permalink raw reply	[flat|nested] 28+ messages in thread

end of thread, other threads:[~2020-05-18 16:35 UTC | newest]

Thread overview: 28+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-18  1:13 [PATCH v6 00/18] Add support for qemu-xen runnning in a Linux-based stubdomain Jason Andryuk
2020-05-18  1:13 ` [PATCH v6 01/18] Document ioemu MiniOS stubdomain protocol Jason Andryuk
2020-05-18  1:13 ` [PATCH v6 02/18] Document ioemu Linux " Jason Andryuk
2020-05-18  1:13 ` [PATCH v6 03/18] libxl: fix qemu-trad cmdline for no sdl/vnc case Jason Andryuk
2020-05-18  1:13 ` [PATCH v6 04/18] libxl: Allow running qemu-xen in stubdomain Jason Andryuk
2020-05-18  1:13 ` [PATCH v6 05/18] libxl: Handle Linux stubdomain specific QEMU options Jason Andryuk
2020-05-18  1:13 ` [PATCH v6 06/18] libxl: write qemu arguments into separate xenstore keys Jason Andryuk
2020-05-18 14:19   ` Ian Jackson
2020-05-18 15:20     ` Jason Andryuk
2020-05-18 16:34       ` Ian Jackson
2020-05-18  1:13 ` [PATCH v6 07/18] xl: add stubdomain related options to xl config parser Jason Andryuk
2020-05-18  1:13 ` [PATCH v6 08/18] tools/libvchan: notify server when client is connected Jason Andryuk
2020-05-18  1:13 ` [PATCH v6 09/18] libxl: add save/restore support for qemu-xen in stubdomain Jason Andryuk
2020-05-18 14:24   ` Ian Jackson
2020-05-18 15:30     ` Jason Andryuk
2020-05-18 16:29       ` Ian Jackson
2020-05-18  1:13 ` [PATCH v6 10/18] tools: add missing libxenvchan cflags Jason Andryuk
2020-05-18  1:13 ` [PATCH v6 11/18] tools: add simple vchan-socket-proxy Jason Andryuk
2020-05-18  1:13 ` [PATCH v6 12/18] libxl: Refactor kill_device_model to libxl__kill_xs_path Jason Andryuk
2020-05-18  1:13 ` [PATCH v6 13/18] libxl: use vchan for QMP access with Linux stubdomain Jason Andryuk
2020-05-18 14:27   ` Ian Jackson
2020-05-18  1:13 ` [PATCH v6 14/18] libxl: require qemu in dom0 for multiple stubdomain consoles Jason Andryuk
2020-05-18 14:27   ` Ian Jackson
2020-05-18  1:13 ` [PATCH v6 15/18] libxl: ignore emulated IDE disks beyond the first 4 Jason Andryuk
2020-05-18  1:13 ` [PATCH v6 16/18] libxl: consider also qemu in stubdomain in libxl__dm_active check Jason Andryuk
2020-05-18  1:13 ` [PATCH v6 17/18] docs: Add device-model-domid to xenstore-paths Jason Andryuk
2020-05-18  1:13 ` [PATCH v6 18/18] libxl: Check stubdomain kernel & ramdisk presence Jason Andryuk
2020-05-18 14:28 ` [PATCH v6 00/18] Add support for qemu-xen runnning in a Linux-based stubdomain Ian Jackson

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.