| Message ID | 20231110204207.2927514-3-volodymyr_babchuk@epam.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v1,1/7] xen-block: Do not write frontend nodes | expand |
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;
diff --git a/hw/xen/xen-bus.c b/hw/xen/xen-bus.c index 06d5192aca..75474d4b43 100644 --- a/hw/xen/xen-bus.c +++ b/hw/xen/xen-bus.c @@ -598,8 +598,9 @@ static void xen_device_backend_destroy(XenDevice *xendev) g_assert(xenbus->xsh); - xs_node_destroy(xenbus->xsh, XBT_NULL, xendev->backend_path, - &local_err); + if (xenbus->backend_id == 0) + xs_node_destroy(xenbus->xsh, XBT_NULL, xendev->backend_path, + &local_err); g_free(xendev->backend_path); xendev->backend_path = NULL; @@ -754,8 +755,9 @@ static void xen_device_frontend_destroy(XenDevice *xendev) g_assert(xenbus->xsh); - xs_node_destroy(xenbus->xsh, XBT_NULL, xendev->frontend_path, - &local_err); + if (xenbus->backend_id == 0) + xs_node_destroy(xenbus->xsh, XBT_NULL, xendev->frontend_path, + &local_err); g_free(xendev->frontend_path); xendev->frontend_path = NULL;