[PATCH v2] libxl: wait for QEMU exit before restarting a domain

[PATCH v2] libxl: wait for QEMU exit before restarting a domain

[Jiaqing Zhao]

The PID-based device model destruction path returns immediately after
sending SIGHUP, before QEMU has necessarily exited. When an HVM guest
reboots with VNC enabled, the new QEMU process may fail to bind the VNC
port because the old process still holds it.

Wait for QEMU to exit in a child process after signaling it for at most
10 seconds. If waiting fails or times out, log the error and retain the
existing domain destruction behaviour.

Signed-off-by: Jiaqing Zhao <Zhao.Jiaqing@amd.com>
---
v2:
* Share the fork, error handling and child exit handling between UID and
  PID-based destruction paths.
* Reduce the device-model stop timeout from 60 to 10 seconds.
* Handle unexpected errors from kill(pid, 0) explicitly.
* Fix misspelled "signaling" in commit message.
* Coding style fixes against tools/libs/light/CODING_STYLE.
---
Cc: Anthony PERARD <anthony.perard@vates.tech>
Cc: Juergen Gross <jgross@suse.com>
Cc: Roger Pau Monné <roger@xenproject.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Penny Zheng <penny.zheng@amd.com>
---
 tools/libs/light/libxl_aoutils.c  |  8 ++-
 tools/libs/light/libxl_dm.c       | 98 +++++++++++++++++++++----------
 tools/libs/light/libxl_domain.c   |  2 +-
 tools/libs/light/libxl_internal.h |  3 +-
 4 files changed, 75 insertions(+), 36 deletions(-)

diff --git a/tools/libs/light/libxl_aoutils.c b/tools/libs/light/libxl_aoutils.c
index c2d42e7cac..977a2705a8 100644
--- a/tools/libs/light/libxl_aoutils.c
+++ b/tools/libs/light/libxl_aoutils.c
@@ -628,10 +628,11 @@ void libxl__kill(libxl__gc *gc, pid_t pid, int sig, const char *what)
 
 /* 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 *what, pid_t *pid_r)
 {
     const char *xs_pid;
-    int ret, pid;
+    int ret;
+    pid_t pid;
 
     ret = libxl__xs_read_checked(gc, XBT_NULL, xs_path_pid, &xs_pid);
     if (ret || !xs_pid) {
@@ -640,6 +641,9 @@ int libxl__kill_xs_path(libxl__gc *gc, const char *xs_path_pid,
         goto out;
     }
     pid = atoi(xs_pid);
+    if (pid_r) {
+        *pid_r = pid;
+    }
 
     ret = kill(pid, SIGHUP);
     if (ret < 0 && errno == ESRCH) {
diff --git a/tools/libs/light/libxl_dm.c b/tools/libs/light/libxl_dm.c
index 511ec76a65..104c48a07d 100644
--- a/tools/libs/light/libxl_dm.c
+++ b/tools/libs/light/libxl_dm.c
@@ -2859,7 +2859,7 @@ static void device_model_probe_spawn_outcome(libxl__egc *egc,
 
     /* Ensure our QEMU command line probe is killed. */
     rc = libxl__kill_xs_path(gc, dmss->spawn.pidpath,
-                             "qemu command-line probe");
+                             "qemu command-line probe", NULL);
     if (rc) {
         LOGD(WARN, dmss->guest_domid,
              "Killing qemu command-line probe pid from path %s",
@@ -3420,9 +3420,10 @@ out:
 }
 
 /* Generic function to signal a Qemu instance to exit */
-static int kill_device_model(libxl__gc *gc, const char *xs_path_pid)
+static int kill_device_model(libxl__gc *gc, const char *xs_path_pid,
+                             pid_t *pid_r)
 {
-    return libxl__kill_xs_path(gc, xs_path_pid, "Device Model");
+    return libxl__kill_xs_path(gc, xs_path_pid, "Device Model", pid_r);
 }
 
 /* Helper to destroy a Qdisk backend */
@@ -3433,7 +3434,7 @@ int libxl__destroy_qemu_xenpv_backend(libxl__gc *gc, uint32_t domid)
 
     pid_path = GCSPRINTF("libxl/%u/qdisk-backend-pid", domid);
 
-    rc = kill_device_model(gc, pid_path);
+    rc = kill_device_model(gc, pid_path, NULL);
     if (rc)
         goto out;
 
@@ -3454,6 +3455,30 @@ static void kill_device_model_uid_cb(libxl__egc *egc,
                                      libxl__ev_child *destroyer,
                                      pid_t pid, int status);
 
+/* Wait in a child so the libxl event loop is not blocked. */
+static int wait_device_model_pid_child(libxl__gc *gc, uint32_t domid,
+                                       pid_t pid)
+{
+    unsigned int i;
+    int r;
+
+    for (i = 0; i < LIBXL_DEVICE_MODEL_STOP_TIMEOUT * 10; i++) {
+        r = kill(pid, 0);
+        if (r) {
+            if (errno == ESRCH)
+                return 0;
+            LOGED(ERROR, domid,
+                  "Failed checking device model [%d] state", pid);
+            return ERROR_FAIL;
+        }
+        usleep(100000);
+    }
+
+    LOGD(ERROR, domid,
+         "Timed out waiting for device model [%d] to exit", pid);
+    return ERROR_TIMEDOUT;
+}
+
 /*
  * If we have a uid, we shouldn't kill by pid.  This is because a
  * hostile QEMU might have exited, in which case the pid we have may
@@ -3477,6 +3502,7 @@ void libxl__destroy_device_model(libxl__egc *egc,
     int domid = ddms->domid;
     char *path;
     const char *dm_kill_uid_str = NULL;
+    pid_t dm_pid;
     int reaper_pid;
 
     ddms->rc = 0;
@@ -3513,46 +3539,54 @@ void libxl__destroy_device_model(libxl__egc *egc,
     if (dm_kill_uid_str) {
         LOGD(DEBUG, domid, "Found DM uid %s, destroying by uid",
              dm_kill_uid_str);
+    } else {
+        /*
+         * No uid to kill; attempt to kill by pid.
+         */
+        LOGD(DEBUG, domid, "Didn't find dm UID; destroying by pid");
 
-        reaper_pid = libxl__ev_child_fork(gc, &ddms->destroyer,
-                                          kill_device_model_uid_cb);
-        if (reaper_pid < 0) {
-            rc = ERROR_FAIL;
+        path = GCSPRINTF("/local/domain/%d/image/device-model-pid", domid);
+        rc = kill_device_model(gc, path, &dm_pid);
+        if (rc) {
             ACCUMULATE_RC(ddms->rc);
-            /*
-             * Note that if this fails, we still don't kill by pid, to
-             * make sure that an untrusted DM has not "maliciously"
-             * exited (potentially causing us to kill an unrelated
-             * process which happened to get the same pid).
-             */
+            LOGD(ERROR, domid,
+                 "Killing device model pid from path %s", path);
             goto out;
         }
 
-        if (!reaper_pid) {  /* child */
-            rc = kill_device_model_uid_child(ddms, dm_kill_uid_str);
-            assert(rc <= 0 && rc >= -125);
-            _exit(-rc);
-        }
+        LOGD(DEBUG, domid,
+             "Waiting for device model [%d] to exit", dm_pid);
+    }
 
+    reaper_pid = libxl__ev_child_fork(gc, &ddms->destroyer,
+                                      kill_device_model_uid_cb);
+    if (reaper_pid < 0) {
+        rc = ERROR_FAIL;
+        ACCUMULATE_RC(ddms->rc);
+        LOGD(ERROR, domid, "Unable to fork device model destroyer");
         /*
-         * Parent of successful fork; execution will pick up in
-         * kill_device_model_uid_cb when child exits
+         * Note that if uid fails, we still don't kill by pid, to
+         * make sure that an untrusted DM has not "maliciously"
+         * exited (potentially causing us to kill an unrelated
+         * process which happened to get the same pid).
          */
-        return;
+        goto out;
+    }
+
+    if (!reaper_pid) {  /* child */
+        if (dm_kill_uid_str)
+            rc = kill_device_model_uid_child(ddms, dm_kill_uid_str);
+        else
+            rc = wait_device_model_pid_child(gc, domid, dm_pid);
+        assert(rc <= 0 && rc >= -125);
+        _exit(-rc);
     }
 
     /*
-     * No uid to kill; attept to kill by pid.
+     * Parent of successful fork; execution will pick up in
+     * kill_device_model_uid_cb when child exits
      */
-    LOGD(DEBUG, domid, "Didn't find dm UID; destroying by pid");
-
-    path = GCSPRINTF("/local/domain/%d/image/device-model-pid", domid);
-    rc = kill_device_model(gc, path);
-
-    if (rc) {
-        ACCUMULATE_RC(ddms->rc);
-        LOGD(ERROR, domid, "Killing device model pid from path %s", path);
-    }
+    return;
 
 out:
     /*
diff --git a/tools/libs/light/libxl_domain.c b/tools/libs/light/libxl_domain.c
index 37fcd92871..6e332cfed7 100644
--- a/tools/libs/light/libxl_domain.c
+++ b/tools/libs/light/libxl_domain.c
@@ -1270,7 +1270,7 @@ static void dm_destroy_cb(libxl__egc *egc,
         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");
+        libxl__kill_xs_path(gc, path, "QMP Proxy", NULL);
         /* qmp-proxy for stubdom registers target_domid's QMP sockets. */
         libxl__qmp_cleanup(gc, target_domid);
     }
diff --git a/tools/libs/light/libxl_internal.h b/tools/libs/light/libxl_internal.h
index b65e0064b9..b4a8273b3e 100644
--- a/tools/libs/light/libxl_internal.h
+++ b/tools/libs/light/libxl_internal.h
@@ -97,6 +97,7 @@
 /* QEMU may be slow to load and start due to a bug in Linux where the I/O
  * subsystem sometime produce high latency under load. */
 #define LIBXL_DEVICE_MODEL_START_TIMEOUT 60
+#define LIBXL_DEVICE_MODEL_STOP_TIMEOUT 10
 #define LIBXL_DEVICE_MODEL_SAVE_FILE XEN_LIB_DIR "/qemu-save" /* .$domid */
 #define LIBXL_DEVICE_MODEL_RESTORE_FILE XEN_LIB_DIR "/qemu-resume" /* .$domid */
 #define LIBXL_QMP_CMD_TIMEOUT 10
@@ -2659,7 +2660,7 @@ 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);
+                                const char *what, pid_t *pid_r);
 
 /*----- device addition/removal -----*/
 
-- 
2.53.0

Re: [PATCH v2] libxl: wait for QEMU exit before restarting a domain

[Jan Beulich]

On 10.11.2026 11:02, Jiaqing Zhao wrote:
> The PID-based device model destruction path returns immediately after
> sending SIGHUP, before QEMU has necessarily exited. When an HVM guest
> reboots with VNC enabled, the new QEMU process may fail to bind the VNC
> port because the old process still holds it.
> 
> Wait for QEMU to exit in a child process after signaling it for at most
> 10 seconds. If waiting fails or times out, log the error and retain the
> existing domain destruction behaviour.
> 
> Signed-off-by: Jiaqing Zhao <Zhao.Jiaqing@amd.com>
> ---
> v2:
> * Share the fork, error handling and child exit handling between UID and
>   PID-based destruction paths.
> * Reduce the device-model stop timeout from 60 to 10 seconds.

As per my v1 comments I continue to question this.

> * Handle unexpected errors from kill(pid, 0) explicitly.
> * Fix misspelled "signaling" in commit message.
> * Coding style fixes against tools/libs/light/CODING_STYLE.
> ---
> Cc: Anthony PERARD <anthony.perard@vates.tech>
> Cc: Juergen Gross <jgross@suse.com>
> Cc: Roger Pau Monné <roger@xenproject.com>

You will want to use a correct address here (.org rather than .com).

Jan

Re: [PATCH v2] libxl: wait for QEMU exit before restarting a domain

[Jiaqing Zhao]

On 2026-11-10 18:22, Jan Beulich wrote:
> On 10.11.2026 11:02, Jiaqing Zhao wrote:
>> The PID-based device model destruction path returns immediately after
>> sending SIGHUP, before QEMU has necessarily exited. When an HVM guest
>> reboots with VNC enabled, the new QEMU process may fail to bind the VNC
>> port because the old process still holds it.
>>
>> Wait for QEMU to exit in a child process after signaling it for at most
>> 10 seconds. If waiting fails or times out, log the error and retain the
>> existing domain destruction behaviour.
>>
>> Signed-off-by: Jiaqing Zhao <Zhao.Jiaqing@amd.com>
>> ---
>> v2:
>> * Share the fork, error handling and child exit handling between UID and
>>   PID-based destruction paths.
>> * Reduce the device-model stop timeout from 60 to 10 seconds.
> 
> As per my v1 comments I continue to question this.

I looked further into QEMU's shutdown path. After receiving SIGHUP,
QEMU calls bdrv_drain_all() to wait for outstanding block I/O requests
and bdrv_flush_all() to flush its block devices. These could take
longer than 10 seconds on slow storage.

While this does not identify the timeout you observed with SR-IOV,
my earlier measurements are insufficient to prove that 10s is enough.
I'll restore the 60s timeout in v3.

>> * Handle unexpected errors from kill(pid, 0) explicitly.
>> * Fix misspelled "signaling" in commit message.
>> * Coding style fixes against tools/libs/light/CODING_STYLE.
>> ---
>> Cc: Anthony PERARD <anthony.perard@vates.tech>
>> Cc: Juergen Gross <jgross@suse.com>
>> Cc: Roger Pau Monné <roger@xenproject.com>
> 
> You will want to use a correct address here (.org rather than .com).
> 
> Jan

That was my mistake, sorry.

By the way, on MAINTAINERS:390, Roger's address uses .com, maybe it need to
be fixed?

Re: [PATCH v2] libxl: wait for QEMU exit before restarting a domain

[Jan Beulich]

On 11.11.2026 06:51, Jiaqing Zhao wrote:
> On 2026-11-10 18:22, Jan Beulich wrote:
>> On 10.11.2026 11:02, Jiaqing Zhao wrote:
>>> ---
>>> Cc: Anthony PERARD <anthony.perard@vates.tech>
>>> Cc: Juergen Gross <jgross@suse.com>
>>> Cc: Roger Pau Monné <roger@xenproject.com>
>>
>> You will want to use a correct address here (.org rather than .com).
> 
> That was my mistake, sorry.
> 
> By the way, on MAINTAINERS:390, Roger's address uses .com, maybe it need to
> be fixed?

Indeed - Roger?

Jan