Re: [PATCH v1 2/7] xen-bus: Do not destroy frontend/backend directories

[David Woodhouse <dwmw2@infradead.org>]

[-- Attachment #1: Type: text/plain, Size: 5436 bytes --]

On Fri, 2023-11-10 at 20:42 +0000, Volodymyr Babchuk wrote:
> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> 
> The PV backend running in other than Dom0 domain (non toolstack domain)
> is not allowed to destroy frontend/backend directories. The more,
> it does not need to do that at all, this is purely toolstack/xl devd
> business.
> 
> I do not know for what reason the backend does that here, this is not really
> needed, probably it is just a leftover and all xs_node_destroy()
> instances should go away completely.

No, if this is a hot-unplug then the nodes should indeed be deleted.

The correct criterion to use is, "did QEMU create this device for
itself?".

Try the patch below, and then the existence of xendev->backend will
mean that it's a device that we *discovered* in XenStore (having been
created by the toolstack), and not a device which QEMU created itself.

Then your patch to which I'm replying, and other parts of the code
which create the nodes in xen_device_realize() and elsewhere, can use
'if (xendev->backend)' as the condition for whether to make the changes
in XenStore.

From 99ab85e8c766d0bb9c3ac556172208db8c69a3d7 Mon Sep 17 00:00:00 2001
From: David Woodhouse <dwmw@amazon.co.uk>
Date: Sun, 12 Nov 2023 16:49:21 -0500
Subject: [PATCH] hw/xen: Set XenBackendInstance in the XenDevice before
 realizing it

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 <dwmw@amazon.co.uk>
---
 hw/block/xen-block.c         | 3 +--
 hw/char/xen_console.c        | 2 +-
 hw/net/xen_nic.c             | 2 +-
 hw/xen/xen-bus.c             | 4 ++++
 include/hw/xen/xen-backend.h | 2 --
 include/hw/xen/xen-bus.h     | 2 ++
 6 files changed, 9 insertions(+), 6 deletions(-)

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-bus.c b/hw/xen/xen-bus.c
index fb82cc33e4..e2c5006bfb 100644
--- a/hw/xen/xen-bus.c
+++ b/hw/xen/xen-bus.c
@@ -1080,6 +1080,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 38d40afa37..5e55b984ab 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;
-- 
2.34.1






[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5965 bytes --]