xen: fix dm_restrict startup

Commit Message

Jason Andryuk March 12, 2023, 11:41 p.m. UTC

QEMU is failing to signal it start when launched by libxl with
dm_restrict=1.  When xenstore_record_dm_state() is called, the
restrictions prevent xs_open() from succeeding.  QEMU can't write
running to the xenstore, and libxl fails the VM start up.

Pass in a open xenstore connection.  Let the call back use it and the
close it.  Use the completed boolean to only allow it to be called once.
This lets the xenstore connection be closed after the startup
indication.

Fixes: ba2a92db1ff6 ("hw/xen: Add xenstore operations to allow redirection to internal emulation")
Signed-off-by: Jason Andryuk <[e-mail]>
---
I think xenstore_record_dm_state() only needs to indicate startup once,
so the completed bool makes sense.

 accel/xen/xen-all.c | 26 ++++++++++++++++----------
 1 file changed, 16 insertions(+), 10 deletions(-)

Comments

David Woodhouse March 13, 2023, 8:54 a.m. UTC | #1

On Sun, 2023-03-12 at 19:41 -0400, Jason Andryuk wrote:
> QEMU is failing to signal it start when launched by libxl with
> dm_restrict=1.  When xenstore_record_dm_state() is called, the
> restrictions prevent xs_open() from succeeding.  QEMU can't write
> running to the xenstore, and libxl fails the VM start up.
> 
> Pass in a open xenstore connection.  Let the call back use it and the
> close it.  Use the completed boolean to only allow it to be called once.
> This lets the xenstore connection be closed after the startup
> indication.
> 
> Fixes: ba2a92db1ff6 ("hw/xen: Add xenstore operations to allow redirection to internal emulation")
> Signed-off-by: Jason Andryuk <[e-mail]>

Thanks. Of the possible fixes though, I don't think this is my
favourite. I think I'd prefer to go back to using the existing global
'xenstore' from xen-legacy-backend.c as I just tried in my other email.

Or, as I said there, if the write is destined to fail anyway when
xen_domid_restrict is set, why even bother to try?

Patch

diff --git a/accel/xen/xen-all.c b/accel/xen/xen-all.c
index 00221e23c5..3843299843 100644
--- a/accel/xen/xen-all.c
+++ b/accel/xen/xen-all.c
@@ -30,17 +30,13 @@  xc_interface *xen_xc;
 xenforeignmemory_handle *xen_fmem;
 xendevicemodel_handle *xen_dmod;
 
-static void xenstore_record_dm_state(const char *state)
+static void xenstore_record_dm_state(struct xs_handle *xs, const char *state)
 {
-    struct xs_handle *xs;
+    static bool completed;
     char path[50];
 
-    /* We now have everything we need to set the xenstore entry. */
-    xs = xs_open(0);
-    if (xs == NULL) {
-        fprintf(stderr, "Could not contact XenStore\n");
-        exit(1);
-    }
+    if (completed)
+        return;
 
     snprintf(path, sizeof (path), "device-model/%u/state", xen_domid);
     /*
@@ -53,6 +49,7 @@  static void xenstore_record_dm_state(const char *state)
         exit(1);
     }
 
+    completed = true;
     xs_close(xs);
 }
 
@@ -60,9 +57,10 @@  static void xenstore_record_dm_state(const char *state)
 static void xen_change_state_handler(void *opaque, bool running,
                                      RunState state)
 {
+    struct xs_handle *xs = opaque;
     if (running) {
         /* record state running */
-        xenstore_record_dm_state("running");
+        xenstore_record_dm_state(xs, "running");
     }
 }
 
@@ -92,6 +90,7 @@  static void xen_setup_post(MachineState *ms, AccelState *accel)
 static int xen_init(MachineState *ms)
 {
     MachineClass *mc = MACHINE_GET_CLASS(ms);
+    struct xs_handle *xs;
 
     xen_xc = xc_interface_open(0, 0, 0);
     if (xen_xc == NULL) {
@@ -111,7 +110,14 @@  static int xen_init(MachineState *ms)
         xc_interface_close(xen_xc);
         return -1;
     }
-    qemu_add_vm_change_state_handler(xen_change_state_handler, NULL);
+
+    xs = xs_open(0);
+    if (xs == NULL) {
+        fprintf(stderr, "Could not contact XenStore\n");
+        exit(1);
+    }
+
+    qemu_add_vm_change_state_handler(xen_change_state_handler, xs);
     /*
      * opt out of system RAM being allocated by generic code
      */