Re: [PATCH v2 1/6] hw/xen: Set XenBackendInstance in the XenDevice before realizing it
On 22/11/2023 23:04, David Woodhouse wrote: On Wed, 2023-11-22 at 22:56 +, Volodymyr Babchuk wrote: Paul Durrant writes: On 21/11/2023 22:10, Volodymyr Babchuk wrote: From: David Woodhouse 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 --- 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(-) Actually, I think you should probably update xen_backend_try_device_destroy() in this patch too. It currently uses xen_backend_list_find() to check whether the specified XenDevice has an associated XenBackendInstance. Sure. Looks like it is the only user of xen_backend_list_find(), so we can get rid of it as well. I'll drop your R-b tag, because of those additional changes in the new version. I think it's fine to keep. He told me to do it :) I confirm that :-)
Re: [PATCH v2 1/6] hw/xen: Set XenBackendInstance in the XenDevice before realizing it
On Wed, 2023-11-22 at 23:49 +, Volodymyr Babchuk wrote: > > I can just pull it from this link, if you don't mind. Please do; thank you! smime.p7s Description: S/MIME cryptographic signature Amazon Development Centre (London) Ltd. Registered in England and Wales with registration number 04543232 with its registered office at 1 Principal Place, Worship Street, London EC2A 2FA, United Kingdom.
Re: [PATCH v2 1/6] hw/xen: Set XenBackendInstance in the XenDevice before realizing it
Hi David, "Woodhouse, David" writes: > On Wed, 2023-11-22 at 17:05 +, Paul Durrant wrote: >> On 21/11/2023 22:10, Volodymyr Babchuk wrote: >> > From: David Woodhouse >> > >> > 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 >> > --- >> > 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(-) >> > >> >> Actually, I think you should probably update >> xen_backend_try_device_destroy() in this patch too. It currently uses >> xen_backend_list_find() to check whether the specified XenDevice has an >> associated XenBackendInstance. > > I think I did that in > https://git.infradead.org/users/dwmw2/qemu.git/commitdiff/94f1b474478ce0ede > but didn't get round to sending it out again because of travel. I can just pull it from this link, if you don't mind. -- WBR, Volodymyr
Re: [PATCH v2 1/6] hw/xen: Set XenBackendInstance in the XenDevice before realizing it
On Wed, 2023-11-22 at 22:56 +, Volodymyr Babchuk wrote: > > > Paul Durrant writes: > > > On 21/11/2023 22:10, Volodymyr Babchuk wrote: > > > From: David Woodhouse > > > 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 > > > --- > > > 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(-) > > > > > > > Actually, I think you should probably update > > xen_backend_try_device_destroy() in this patch too. It currently uses > > xen_backend_list_find() to check whether the specified XenDevice has > > an associated XenBackendInstance. > > Sure. Looks like it is the only user of xen_backend_list_find(), so we > can get rid of it as well. > > I'll drop your R-b tag, because of those additional changes in the new > version. I think it's fine to keep. He told me to do it :) smime.p7s Description: S/MIME cryptographic signature
Re: [PATCH v2 1/6] hw/xen: Set XenBackendInstance in the XenDevice before realizing it
Paul Durrant writes: > On 21/11/2023 22:10, Volodymyr Babchuk wrote: >> From: David Woodhouse >> 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 >> --- >> 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(-) >> > > Actually, I think you should probably update > xen_backend_try_device_destroy() in this patch too. It currently uses > xen_backend_list_find() to check whether the specified XenDevice has > an associated XenBackendInstance. Sure. Looks like it is the only user of xen_backend_list_find(), so we can get rid of it as well. I'll drop your R-b tag, because of those additional changes in the new version. -- WBR, Volodymyr
Re: [PATCH v2 1/6] hw/xen: Set XenBackendInstance in the XenDevice before realizing it
On Wed, 2023-11-22 at 17:05 +, Paul Durrant wrote: > On 21/11/2023 22:10, Volodymyr Babchuk wrote: > > From: David Woodhouse > > > > 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 > > --- > > 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(-) > > > > Actually, I think you should probably update > xen_backend_try_device_destroy() in this patch too. It currently uses > xen_backend_list_find() to check whether the specified XenDevice has an > associated XenBackendInstance. I think I did that in https://git.infradead.org/users/dwmw2/qemu.git/commitdiff/94f1b474478ce0ede but didn't get round to sending it out again because of travel. smime.p7s Description: S/MIME cryptographic signature Amazon Development Centre (London) Ltd. Registered in England and Wales with registration number 04543232 with its registered office at 1 Principal Place, Worship Street, London EC2A 2FA, United Kingdom.
Re: [PATCH v2 1/6] hw/xen: Set XenBackendInstance in the XenDevice before realizing it
On 21/11/2023 22:10, Volodymyr Babchuk wrote: From: David Woodhouse 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 --- 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(-) Actually, I think you should probably update xen_backend_try_device_destroy() in this patch too. It currently uses xen_backend_list_find() to check whether the specified XenDevice has an associated XenBackendInstance. Paul
Re: [PATCH v2 1/6] hw/xen: Set XenBackendInstance in the XenDevice before realizing it
On 21/11/2023 22:10, Volodymyr Babchuk wrote: From: David Woodhouse 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 --- 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(-) Reviewed-by: Paul Durrant
[PATCH v2 1/6] hw/xen: Set XenBackendInstance in the XenDevice before realizing it
From: David Woodhouse
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
---
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 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;
--
2.42.0
