| Message ID | 20231121221023.419901-2-volodymyr_babchuk@epam.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/6] hw/xen: Set XenBackendInstance in the XenDevice before realizing it | expand |
On 21/11/2023 22:10, Volodymyr Babchuk wrote: > From: David Woodhouse <dwmw@amazon.co.uk> > > 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(-) > Reviewed-by: Paul Durrant <paul@xen.org>
On 21/11/2023 22:10, Volodymyr Babchuk wrote: > From: David Woodhouse <dwmw@amazon.co.uk> > > 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(-) > 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
On Wed, 2023-11-22 at 17:05 +0000, Paul Durrant wrote: > On 21/11/2023 22:10, Volodymyr Babchuk wrote: > > From: David Woodhouse <dwmw@amazon.co.uk> > > > > 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(-) > > > > 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. 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.
Paul Durrant <xadimgnik@gmail.com> writes: > On 21/11/2023 22:10, Volodymyr Babchuk wrote: >> From: David Woodhouse <dwmw@amazon.co.uk> >> 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(-) >> > > 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.
On Wed, 2023-11-22 at 22:56 +0000, Volodymyr Babchuk wrote: > > > Paul Durrant <xadimgnik@gmail.com> writes: > > > On 21/11/2023 22:10, Volodymyr Babchuk wrote: > > > From: David Woodhouse <dwmw@amazon.co.uk> > > > 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(-) > > > > > > > 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 :)
Hi David, "Woodhouse, David" <dwmw@amazon.co.uk> writes: > On Wed, 2023-11-22 at 17:05 +0000, Paul Durrant wrote: >> On 21/11/2023 22:10, Volodymyr Babchuk wrote: >> > From: David Woodhouse <dwmw@amazon.co.uk> >> > >> > 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(-) >> > >> >> 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.
On Wed, 2023-11-22 at 23:49 +0000, Volodymyr Babchuk wrote: > > I can just pull it from this link, if you don't mind. Please do; thank you! 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.
On 22/11/2023 23:04, David Woodhouse wrote: > On Wed, 2023-11-22 at 22:56 +0000, Volodymyr Babchuk wrote: >> >> >> Paul Durrant <xadimgnik@gmail.com> writes: >> >>> On 21/11/2023 22:10, Volodymyr Babchuk wrote: >>>> From: David Woodhouse <dwmw@amazon.co.uk> >>>> 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(-) >>>> >>> >>> 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 :-)
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;