tools/libs/light/libxl_aoutils.c | 8 ++++-- tools/libs/light/libxl_dm.c | 45 ++++++++++++++++++++++++++----- tools/libs/light/libxl_domain.c | 2 +- tools/libs/light/libxl_internal.h | 3 ++- 4 files changed, 48 insertions(+), 10 deletions(-)
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 singaling it for at most
60 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>
---
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 | 45 ++++++++++++++++++++++++++-----
tools/libs/light/libxl_domain.c | 2 +-
tools/libs/light/libxl_internal.h | 3 ++-
4 files changed, 48 insertions(+), 10 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..ac1db9fe6a 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,21 @@ static void kill_device_model_uid_cb(libxl__egc *egc,
libxl__ev_child *destroyer,
pid_t pid, int status);
+/* Run in a child so waiting for QEMU does not block the libxl event loop. */
+static int wait_device_model_pid_child(libxl__gc *gc, uint32_t domid, pid_t pid)
+{
+ unsigned int i;
+
+ for (i = 0; i < LIBXL_DEVICE_MODEL_STOP_TIMEOUT * 10; i++) {
+ if (kill(pid, 0) && errno == ESRCH)
+ return 0;
+ 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 +3493,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;
@@ -3547,12 +3564,28 @@ void libxl__destroy_device_model(libxl__egc *egc,
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);
-
+ rc = kill_device_model(gc, path, &dm_pid);
if (rc) {
ACCUMULATE_RC(ddms->rc);
LOGD(ERROR, domid, "Killing device model pid from path %s", path);
+ goto out;
+ }
+
+ 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 exit waiter");
+ goto out;
}
+ if (!reaper_pid) {
+ rc = wait_device_model_pid_child(gc, domid, dm_pid);
+ assert(rc <= 0 && rc >= -125);
+ _exit(-rc);
+ }
+ 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..f4fde8009e 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 60
#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
On 2026-11-04 01:46, 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 singaling it for at most
"signaling"
> 60 seconds. If waiting fails or times out, log the error and retain the
> existing domain destruction behaviour.
60 seconds seems like a lot. Did you measure the time to see how long
it takes? It's an upper bound, but maybe 10 seconds would be better?
> Signed-off-by: Jiaqing Zhao <Zhao.Jiaqing@amd.com>
> @@ -3547,12 +3564,28 @@ void libxl__destroy_device_model(libxl__egc *egc,
> 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);
> -
> + rc = kill_device_model(gc, path, &dm_pid);
If you just read the pid directly here, you don't need to change
kill_device_model(). But your approach is only a single read, so maybe
it's better. I'm undecided, but maybe someone else has an opinion.
> if (rc) {
> ACCUMULATE_RC(ddms->rc);
> LOGD(ERROR, domid, "Killing device model pid from path %s", path);
> + goto out;
> + }
> +
> + 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);
The reaper_pid handling below is duplicated from
`if (dm_kill_uid_str) {`
Put the above pid code in an else, and use a single implementation of
the reaper_pid handling.
> + if (reaper_pid < 0) {
> + rc = ERROR_FAIL;
> + ACCUMULATE_RC(ddms->rc);
> + LOGD(ERROR, domid, "Unable to fork device model exit waiter");
> + goto out;
> }
> + if (!reaper_pid) {
> + rc = wait_device_model_pid_child(gc, domid, dm_pid);
Though this would need an additional conditional.
Thanks,
Jason
> + assert(rc <= 0 && rc >= -125);
> + _exit(-rc);
> + }
> + return;
>
> out:
> /*
On 2026-11-05 01:33, Jason Andryuk wrote:
> On 2026-11-04 01:46, 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 singaling it for at most
>
> "signaling"
>
>> 60 seconds. If waiting fails or times out, log the error and retain the
>> existing domain destruction behaviour.
>
> 60 seconds seems like a lot. Did you measure the time to see how long it takes? It's an upper bound, but maybe 10 seconds would be better?
Typically it costs less than 3s. Most time it only takes 1s. Will change
to 10s in v2.
>> Signed-off-by: Jiaqing Zhao <Zhao.Jiaqing@amd.com>
>
>> @@ -3547,12 +3564,28 @@ void libxl__destroy_device_model(libxl__egc *egc,
>> 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);
>> -
>> + rc = kill_device_model(gc, path, &dm_pid);
>
> If you just read the pid directly here, you don't need to change kill_device_model(). But your approach is only a single read, so maybe it's better. I'm undecided, but maybe someone else has an opinion.
Yes keeping old kill_device_model() requires an extra libxl__xs_read_checked()
to read the pid. Let's wait for more comments.
>> if (rc) {
>> ACCUMULATE_RC(ddms->rc);
>> LOGD(ERROR, domid, "Killing device model pid from path %s", path);
>> + goto out;
>> + }
>> +
>> + 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);
>
> The reaper_pid handling below is duplicated from
> `if (dm_kill_uid_str) {`
>
> Put the above pid code in an else, and use a single implementation of the reaper_pid handling.
>
>> + if (reaper_pid < 0) {
>> + rc = ERROR_FAIL;
>> + ACCUMULATE_RC(ddms->rc);
>> + LOGD(ERROR, domid, "Unable to fork device model exit waiter");
>> + goto out;
>> }
>> + if (!reaper_pid) {
>> + rc = wait_device_model_pid_child(gc, domid, dm_pid);
>
> Though this would need an additional conditional.
>
> Thanks,
> Jason
Sure, will do in v2.
>> + assert(rc <= 0 && rc >= -125);
>> + _exit(-rc);
>> + }
>> + return;
>> out:
>> /*
© 2016 - 2026 Red Hat, Inc.