[v4,1/6] hw/xen: Set XenBackendInstance in the XenDevice before realizing it

Commit Message

Volodymyr Babchuk Dec. 2, 2023, 1:41 a.m. UTC

From: David Woodhouse <[e-mail]>

This allows a XenDevice implementation to know whether it was created
by QEMU, or merely discovered in XenStore after the toolstack created
it. This will allow us to create frontend/backend nodes only when we
should, rather than unconditionally attempting to overwrite them from
a driver domain which doesn't have privileges to do so.

As an added benefit, it also means we no longer have to call the
xen_backend_set_device() function from the device models immediately
after calling qdev_realize_and_unref(). Even though we could make
the argument that it's safe to do so, and the pointer to the unreffed
device *will* actually still be valid, it still made my skin itch to
look at it.

Signed-off-by: David Woodhouse <[e-mail]>
Reviewed-by: Paul Durrant <[e-mail]>
---
 hw/block/xen-block.c         |  3 +--
 hw/char/xen_console.c        |  2 +-
 hw/net/xen_nic.c             |  2 +-
 hw/xen/xen-backend.c         | 15 +--------------
 hw/xen/xen-bus.c             |  4 ++++
 include/hw/xen/xen-backend.h |  2 --
 include/hw/xen/xen-bus.h     |  2 ++
 7 files changed, 10 insertions(+), 20 deletions(-)

Comments

David Woodhouse Jan. 26, 2024, 12:09 p.m. UTC | #1

On Sat, 2023-12-02 at 01:41 +0000, Volodymyr Babchuk wrote:
> From: David Woodhouse <[e-mail]>
> 
> This allows a XenDevice implementation to know whether it was created
> by QEMU, or merely discovered in XenStore after the toolstack created
> it. This will allow us to create frontend/backend nodes only when we
> should, rather than unconditionally attempting to overwrite them from
> a driver domain which doesn't have privileges to do so.
> 
> As an added benefit, it also means we no longer have to call the
> xen_backend_set_device() function from the device models immediately
> after calling qdev_realize_and_unref(). Even though we could make
> the argument that it's safe to do so, and the pointer to the unreffed
> device *will* actually still be valid, it still made my skin itch to
> look at it.
> 
> Signed-off-by: David Woodhouse <[e-mail]>
> Reviewed-by: Paul Durrant <[e-mail]>

I still have this lying around in my working tree. Shall I forget all
about it and let you take it with this patch series?

Patch

diff --git a/hw/block/xen-block.c b/hw/block/xen-block.c
index 6d64ede94f..c2ac9db4a2 100644
--- a/hw/block/xen-block.c
+++ b/hw/block/xen-block.c
@@ -1081,13 +1081,12 @@  static void xen_block_device_create(XenBackendInstance *backend,
 
     blockdev->iothread = iothread;
     blockdev->drive = drive;
+    xendev->backend = backend;
 
     if (!qdev_realize_and_unref(DEVICE(xendev), BUS(xenbus), errp)) {
         error_prepend(errp, "realization of device %s failed: ", type);
         goto fail;
     }
-
-    xen_backend_set_device(backend, xendev);
     return;
 
 fail:
diff --git a/hw/char/xen_console.c b/hw/char/xen_console.c
index 5cbee2f184..bef8a3a621 100644
--- a/hw/char/xen_console.c
+++ b/hw/char/xen_console.c
@@ -600,8 +600,8 @@  static void xen_console_device_create(XenBackendInstance *backend,
         goto fail;
     }
 
+    xendev->backend = backend;
     if (qdev_realize_and_unref(DEVICE(xendev), BUS(xenbus), errp)) {
-        xen_backend_set_device(backend, xendev);
         goto done;
     }
 
diff --git a/hw/net/xen_nic.c b/hw/net/xen_nic.c
index af4ba3f1e6..afa10c96e8 100644
--- a/hw/net/xen_nic.c
+++ b/hw/net/xen_nic.c
@@ -627,8 +627,8 @@  static void xen_net_device_create(XenBackendInstance *backend,
     net->dev = number;
     memcpy(&net->conf.macaddr, &mac, sizeof(mac));
 
+    xendev->backend = backend;
     if (qdev_realize_and_unref(DEVICE(xendev), BUS(xenbus), errp)) {
-        xen_backend_set_device(backend, xendev);
         return;
     }
 
diff --git a/hw/xen/xen-backend.c b/hw/xen/xen-backend.c
index b9bf70a9f5..b2e753ebc8 100644
--- a/hw/xen/xen-backend.c
+++ b/hw/xen/xen-backend.c
@@ -88,19 +88,6 @@  static void xen_backend_list_add(XenBackendInstance *backend)
     QLIST_INSERT_HEAD(&backend_list, backend, entry);
 }
 
-static XenBackendInstance *xen_backend_list_find(XenDevice *xendev)
-{
-    XenBackendInstance *backend;
-
-    QLIST_FOREACH(backend, &backend_list, entry) {
-        if (backend->xendev == xendev) {
-            return backend;
-        }
-    }
-
-    return NULL;
-}
-
 bool xen_backend_exists(const char *type, const char *name)
 {
     const XenBackendImpl *impl = xen_backend_table_lookup(type);
@@ -170,7 +157,7 @@  XenDevice *xen_backend_get_device(XenBackendInstance *backend)
 
 bool xen_backend_try_device_destroy(XenDevice *xendev, Error **errp)
 {
-    XenBackendInstance *backend = xen_backend_list_find(xendev);
+    XenBackendInstance *backend = xendev->backend;
     const XenBackendImpl *impl;
 
     if (!backend) {
diff --git a/hw/xen/xen-bus.c b/hw/xen/xen-bus.c
index 4973e7d9c9..dd0171ab98 100644
--- a/hw/xen/xen-bus.c
+++ b/hw/xen/xen-bus.c
@@ -1079,6 +1079,10 @@  static void xen_device_realize(DeviceState *dev, Error **errp)
         }
     }
 
+    if (xendev->backend) {
+        xen_backend_set_device(xendev->backend, xendev);
+    }
+
     xendev->exit.notify = xen_device_exit;
     qemu_add_exit_notifier(&xendev->exit);
     return;
diff --git a/include/hw/xen/xen-backend.h b/include/hw/xen/xen-backend.h
index 0f01631ae7..ea080ba7c9 100644
--- a/include/hw/xen/xen-backend.h
+++ b/include/hw/xen/xen-backend.h
@@ -10,8 +10,6 @@ 
 
 #include "hw/xen/xen-bus.h"
 
-typedef struct XenBackendInstance XenBackendInstance;
-
 typedef void (*XenBackendDeviceCreate)(XenBackendInstance *backend,
                                        QDict *opts, Error **errp);
 typedef void (*XenBackendDeviceDestroy)(XenBackendInstance *backend,
diff --git a/include/hw/xen/xen-bus.h b/include/hw/xen/xen-bus.h
index 334ddd1ff6..7647c4c38e 100644
--- a/include/hw/xen/xen-bus.h
+++ b/include/hw/xen/xen-bus.h
@@ -14,9 +14,11 @@ 
 #include "qom/object.h"
 
 typedef struct XenEventChannel XenEventChannel;
+typedef struct XenBackendInstance XenBackendInstance;
 
 struct XenDevice {
     DeviceState qdev;
+    XenBackendInstance *backend;
     domid_t frontend_id;
     char *name;
     struct qemu_xs_handle *xsh;