From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
The PV backend running in other than Dom0 domain (non toolstack domain)
is not allowed to write frontend nodes. The more, the backend 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 xen_device_frontend_printf()
instances should go away completely.
Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
---
hw/block/xen-block.c | 11 +++++++----
hw/xen/xen-bus.c | 2 +-
2 files changed, 8 insertions(+), 5 deletions(-)
diff --git a/hw/block/xen-block.c b/hw/block/xen-block.c
index a07cd7eb5d..dc4d477c22 100644
--- a/hw/block/xen-block.c
+++ b/hw/block/xen-block.c
@@ -221,6 +221,7 @@ static void xen_block_realize(XenDevice *xendev, Error **errp)
XenBlockVdev *vdev = &blockdev->props.vdev;
BlockConf *conf = &blockdev->props.conf;
BlockBackend *blk = conf->blk;
+ XenBus *xenbus = XEN_BUS(qdev_get_parent_bus(DEVICE(xendev)));
if (vdev->type == XEN_BLOCK_VDEV_TYPE_INVALID) {
error_setg(errp, "vdev property not set");
@@ -280,10 +281,12 @@ static void xen_block_realize(XenDevice *xendev, Error **errp)
xen_device_backend_printf(xendev, "info", "%u", blockdev->info);
- xen_device_frontend_printf(xendev, "virtual-device", "%lu",
- vdev->number);
- xen_device_frontend_printf(xendev, "device-type", "%s",
- blockdev->device_type);
+ if (xenbus->backend_id == 0) {
+ xen_device_frontend_printf(xendev, "virtual-device", "%lu",
+ vdev->number);
+ xen_device_frontend_printf(xendev, "device-type", "%s",
+ blockdev->device_type);
+ }
xen_device_backend_printf(xendev, "sector-size", "%u",
conf->logical_block_size);
diff --git a/hw/xen/xen-bus.c b/hw/xen/xen-bus.c
index ece8ec40cd..06d5192aca 100644
--- a/hw/xen/xen-bus.c
+++ b/hw/xen/xen-bus.c
@@ -1048,7 +1048,7 @@ static void xen_device_realize(DeviceState *dev, Error **errp)
xen_device_backend_set_online(xendev, true);
xen_device_backend_set_state(xendev, XenbusStateInitWait);
- if (!xen_device_frontend_exists(xendev)) {
+ if (!xen_device_frontend_exists(xendev) && xenbus->backend_id == 0) {
xen_device_frontend_printf(xendev, "backend", "%s",
xendev->backend_path);
xen_device_frontend_printf(xendev, "backend-id", "%u",
--
2.42.0On 10/11/2023 15:42, 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 write frontend nodes. The more, the backend 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 xen_device_frontend_printf()
> instances should go away completely.
>
It is not a leftover and it is needed in the case that QEMU is
instantiating the backend unilaterally... i.e. without the involvement
of any Xen toolstack.
Agreed that, if QEMU, is running in a deprivileged context, that is not
an option. The correct way to determined this though is whether the
device is being created via the QEMU command line or whether is being
created because XenStore nodes written by a toolstack were discovered.
In the latter case there should be a XenBackendInstance that corresponds
to the XenDevice whereas in the former case there should not be. For
example, see xen_backend_try_device_destroy()
Paul
> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
> ---
> hw/block/xen-block.c | 11 +++++++----
> hw/xen/xen-bus.c | 2 +-
> 2 files changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/hw/block/xen-block.c b/hw/block/xen-block.c
> index a07cd7eb5d..dc4d477c22 100644
> --- a/hw/block/xen-block.c
> +++ b/hw/block/xen-block.c
> @@ -221,6 +221,7 @@ static void xen_block_realize(XenDevice *xendev, Error **errp)
> XenBlockVdev *vdev = &blockdev->props.vdev;
> BlockConf *conf = &blockdev->props.conf;
> BlockBackend *blk = conf->blk;
> + XenBus *xenbus = XEN_BUS(qdev_get_parent_bus(DEVICE(xendev)));
>
> if (vdev->type == XEN_BLOCK_VDEV_TYPE_INVALID) {
> error_setg(errp, "vdev property not set");
> @@ -280,10 +281,12 @@ static void xen_block_realize(XenDevice *xendev, Error **errp)
>
> xen_device_backend_printf(xendev, "info", "%u", blockdev->info);
>
> - xen_device_frontend_printf(xendev, "virtual-device", "%lu",
> - vdev->number);
> - xen_device_frontend_printf(xendev, "device-type", "%s",
> - blockdev->device_type);
> + if (xenbus->backend_id == 0) {
> + xen_device_frontend_printf(xendev, "virtual-device", "%lu",
> + vdev->number);
> + xen_device_frontend_printf(xendev, "device-type", "%s",
> + blockdev->device_type);
> + }
>
> xen_device_backend_printf(xendev, "sector-size", "%u",
> conf->logical_block_size);
> diff --git a/hw/xen/xen-bus.c b/hw/xen/xen-bus.c
> index ece8ec40cd..06d5192aca 100644
> --- a/hw/xen/xen-bus.c
> +++ b/hw/xen/xen-bus.c
> @@ -1048,7 +1048,7 @@ static void xen_device_realize(DeviceState *dev, Error **errp)
> xen_device_backend_set_online(xendev, true);
> xen_device_backend_set_state(xendev, XenbusStateInitWait);
>
> - if (!xen_device_frontend_exists(xendev)) {
> + if (!xen_device_frontend_exists(xendev) && xenbus->backend_id == 0) {
> xen_device_frontend_printf(xendev, "backend", "%s",
> xendev->backend_path);
> xen_device_frontend_printf(xendev, "backend-id", "%u",
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 write frontend nodes. The more, the backend 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 xen_device_frontend_printf() > instances should go away completely. No, this is what allows qemu to create PV devices, as opposed to just handle the ones which are created for it by the toolstack. Perhaps we should only create the frontend nodes (and likewise, only destroy those and the backend nodes on destruction) in the case where the device was instantiated directly by the QEMU command line, and refrain from doing so for the devices which were created by the toolstack and merely 'discovered' by xen_block_device_create()? (Note that we need to look at net and console devices too, now they've finally been converted to the 'new' XenBus framework in QEMU 8.2.)
On 11/11/2023 10:55 am, David Woodhouse wrote: > 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 write frontend nodes. The more, the backend 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 xen_device_frontend_printf() >> instances should go away completely. > No, this is what allows qemu to create PV devices, as opposed to just > handle the ones which are created for it by the toolstack. > > Perhaps we should only create the frontend nodes (and likewise, only > destroy those and the backend nodes on destruction) in the case where > the device was instantiated directly by the QEMU command line, and > refrain from doing so for the devices which were created by the > toolstack and merely 'discovered' by xen_block_device_create()? > > (Note that we need to look at net and console devices too, now they've > finally been converted to the 'new' XenBus framework in QEMU 8.2.) > > Furthermore, the control domain doesn't always have the domid of 0. If qemu wants/needs to make changes like this, the control domain has to arrange for qemu's domain to have appropriate permissions on the nodes. ~Andrew
On 11 November 2023 08:43:40 GMT-05:00, Andrew Cooper <andrew.cooper3@citrix.com> wrote: >Furthermore, the control domain doesn't always have the domid of 0. > >If qemu wants/needs to make changes like this, the control domain has to >arrange for qemu's domain to have appropriate permissions on the nodes. Right. And that's simple enough: if you are running QEMU in a domain which doesn't have permission to create the backend directory and/or the frontend nodes, don't ask it to *create* devices. In that case it is only able to connect as the backend for devices which were created *for* it by the toolstack. The criterion used in this patch series should be "did QEMU create this device, or discover it".
On 11/11/2023 8:18 pm, David Woodhouse wrote: > On 11 November 2023 08:43:40 GMT-05:00, Andrew Cooper <andrew.cooper3@citrix.com> wrote: >> Furthermore, the control domain doesn't always have the domid of 0. >> >> If qemu wants/needs to make changes like this, the control domain has to >> arrange for qemu's domain to have appropriate permissions on the nodes. > Right. And that's simple enough: if you are running QEMU in a domain which doesn't have permission to create the backend directory and/or the frontend nodes, don't ask it to *create* devices. In that case it is only able to connect as the backend for devices which were created *for* it by the toolstack. > > The criterion used in this patch series should be "did QEMU create this device, or discover it". > Yeah, that sounds like the right approach. ~Andrew
On 11 November 2023 16:51:22 GMT-05:00, Andrew Cooper <andrew.cooper3@citrix.com> wrote: >On 11/11/2023 8:18 pm, David Woodhouse wrote: >> On 11 November 2023 08:43:40 GMT-05:00, Andrew Cooper <andrew.cooper3@citrix.com> wrote: >>> Furthermore, the control domain doesn't always have the domid of 0. >>> >>> If qemu wants/needs to make changes like this, the control domain has to >>> arrange for qemu's domain to have appropriate permissions on the nodes. >> Right. And that's simple enough: if you are running QEMU in a domain which doesn't have permission to create the backend directory and/or the frontend nodes, don't ask it to *create* devices. In that case it is only able to connect as the backend for devices which were created *for* it by the toolstack. >> >> The criterion used in this patch series should be "did QEMU create this device, or discover it". >> > >Yeah, that sounds like the right approach. I think we want to kill the xen_backend_set_device() function and instead set the backend as a property of the XenDevice *before* realizing it.
Hi David, David Woodhouse <dwmw2@infradead.org> writes: > On 11 November 2023 16:51:22 GMT-05:00, Andrew Cooper <andrew.cooper3@citrix.com> wrote: >>On 11/11/2023 8:18 pm, David Woodhouse wrote: >>> On 11 November 2023 08:43:40 GMT-05:00, Andrew Cooper <andrew.cooper3@citrix.com> wrote: >>>> Furthermore, the control domain doesn't always have the domid of 0. >>>> >>>> If qemu wants/needs to make changes like this, the control domain has to >>>> arrange for qemu's domain to have appropriate permissions on the nodes. >>> Right. And that's simple enough: if you are running QEMU in a >>> domain which doesn't have permission to create the backend >>> directory and/or the frontend nodes, don't ask it to *create* >>> devices. In that case it is only able to connect as the backend for >>> devices which were created *for* it by the toolstack. >>> >>> The criterion used in this patch series should be "did QEMU create this device, or discover it". >>> >> >>Yeah, that sounds like the right approach. > > I think we want to kill the xen_backend_set_device() function and > instead set the backend as a property of the XenDevice *before* > realizing it. Not sure that I got this. Right now device is property of XenBackendInstance. Are you proposing to make this other way around? Right now I am looking for a place where to store the information of XenDevice creator. My plan was to add "found_in_xenbus" property to XenDevice and set it in xen_backend_device_create. -- WBR, Volodymyr
On Tue, 2023-11-14 at 21:32 +0000, Volodymyr Babchuk wrote: > > > I think we want to kill the xen_backend_set_device() function and > > instead set the backend as a property of the XenDevice *before* > > realizing it. > > Not sure that I got this. Right now device is property of > XenBackendInstance. Are you proposing to make this other way around? > > Right now I am looking for a place where to store the information of > XenDevice creator. My plan was to add "found_in_xenbus" property to > XenDevice and set it in xen_backend_device_create. A bit like this? https://lore.kernel.org/qemu-devel/916e6f41e2da700375f84a2fda7b85d4b58dfb31.camel@infradead.org
© 2016 - 2024 Red Hat, Inc.