[PATCH v1 0/7] xen-arm: add support for virtio-pci

[PATCH v1 0/7] xen-arm: add support for virtio-pci

[Volodymyr Babchuk]

Hello,

This patch series adds the basic support for virtio-pci for xen-arm
guests. The main changes are in "xen_arm: Add basic virtio-pci
support", while another 5 patches are adding groundwork. First 4
patches are required to run QEMU device model in a driver domain, not
only in Dom0.

Oleksandr Tyshchenko (6):
  xen-block: Do not write frontend nodes
  xen-bus: Do not destroy frontend/backend directories
  xen_pvdev: Do not assume Dom0 when creating a directrory
  xen-bus: Set offline if backend's state is XenbusStateClosed
  xen_arm: Set mc->max_cpus to GUEST_MAX_VCPUS in xen_arm_init()
  xen_arm: Add basic virtio-pci support

Volodymyr Babchuk (1):
  xen: xenstore: add possibility to preserve owner

 hw/arm/xen_arm.c                 | 188 ++++++++++++++++++++++++++++++-
 hw/block/xen-block.c             |  11 +-
 hw/i386/kvm/xen_xenstore.c       |  18 +++
 hw/xen/xen-bus.c                 |  16 ++-
 hw/xen/xen-hvm-common.c          |   9 +-
 hw/xen/xen-operations.c          |  12 ++
 hw/xen/xen_pvdev.c               |   3 +-
 include/hw/xen/xen_backend_ops.h |   2 +
 include/hw/xen/xen_native.h      |   8 +-
 9 files changed, 253 insertions(+), 14 deletions(-)

-- 
2.42.0

[PATCH v1 1/7] xen-block: Do not write frontend nodes

[Volodymyr Babchuk]

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.0

Re: [PATCH v1 1/7] xen-block: Do not write frontend nodes

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 1169 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 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.)



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

Re: [PATCH v1 1/7] xen-block: Do not write frontend nodes

[Andrew Cooper]

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

Re: [PATCH v1 1/7] xen-block: Do not write frontend nodes

[David Woodhouse]

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".

Re: [PATCH v1 1/7] xen-block: Do not write frontend nodes

[Andrew Cooper]

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

Re: [PATCH v1 1/7] xen-block: Do not write frontend nodes

[David Woodhouse]

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.

Re: [PATCH v1 1/7] xen-block: Do not write frontend nodes

[Volodymyr Babchuk]

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

Re: [PATCH v1 1/7] xen-block: Do not write frontend nodes

[David Woodhouse]

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

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


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

Re: [PATCH v1 1/7] xen-block: Do not write frontend nodes

[Paul Durrant]

On 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",

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

[Volodymyr Babchuk]

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.

Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
---
 hw/xen/xen-bus.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

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;
 
-- 
2.42.0

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

[David Woodhouse]

[-- 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 --]

[PATCH v1 3/7] xen: xenstore: add possibility to preserve owner

[Volodymyr Babchuk]

Add option to preserve owner when creating an entry in Xen Store. This
may be needed in cases when Qemu is working as device model in a
domain that is Domain-0, e.g. in driver domain.

"owner" parameter for qemu_xen_xs_create() function can have special
value XS_PRESERVE_OWNER, which will make specific implementation to
get original owner of an entry and pass it back to
set_permissions() call.

Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
---
 hw/i386/kvm/xen_xenstore.c       | 18 ++++++++++++++++++
 hw/xen/xen-operations.c          | 12 ++++++++++++
 include/hw/xen/xen_backend_ops.h |  2 ++
 3 files changed, 32 insertions(+)

diff --git a/hw/i386/kvm/xen_xenstore.c b/hw/i386/kvm/xen_xenstore.c
index 660d0b72f9..7b894a9884 100644
--- a/hw/i386/kvm/xen_xenstore.c
+++ b/hw/i386/kvm/xen_xenstore.c
@@ -1572,6 +1572,24 @@ static bool xs_be_create(struct qemu_xs_handle *h, xs_transaction_t t,
         return false;
     }
 
+    if (owner == XS_PRESERVE_OWNER) {
+        GList *perms;
+        char letter;
+
+        err = xs_impl_get_perms(h->impl, 0, t, path, &perms);
+        if (err) {
+            errno = err;
+            return false;
+        }
+
+        if (sscanf(perms->data, "%c%u", &letter, &owner) != 2) {
+            errno = EFAULT;
+            g_list_free_full(perms, g_free);
+            return false;
+        }
+        g_list_free_full(perms, g_free);
+    }
+
     perms_list = g_list_append(perms_list,
                                xs_perm_as_string(XS_PERM_NONE, owner));
     perms_list = g_list_append(perms_list,
diff --git a/hw/xen/xen-operations.c b/hw/xen/xen-operations.c
index e00983ec44..1df59b3c08 100644
--- a/hw/xen/xen-operations.c
+++ b/hw/xen/xen-operations.c
@@ -300,6 +300,18 @@ static bool libxenstore_create(struct qemu_xs_handle *h, xs_transaction_t t,
         return false;
     }
 
+    if (owner == XS_PRESERVE_OWNER) {
+        struct xs_permissions *tmp;
+        unsigned int num;
+
+        tmp = xs_get_permissions(h->xsh, 0, path, &num);
+        if (tmp == NULL) {
+            return false;
+        }
+        perms_list[0].id = tmp[0].id;
+        free(tmp);
+    }
+
     return xs_set_permissions(h->xsh, t, path, perms_list,
                               ARRAY_SIZE(perms_list));
 }
diff --git a/include/hw/xen/xen_backend_ops.h b/include/hw/xen/xen_backend_ops.h
index 90cca85f52..273e414559 100644
--- a/include/hw/xen/xen_backend_ops.h
+++ b/include/hw/xen/xen_backend_ops.h
@@ -266,6 +266,8 @@ typedef uint32_t xs_transaction_t;
 #define XS_PERM_READ  0x01
 #define XS_PERM_WRITE 0x02
 
+#define XS_PRESERVE_OWNER        0xFFFE
+
 struct xenstore_backend_ops {
     struct qemu_xs_handle *(*open)(void);
     void (*close)(struct qemu_xs_handle *h);
-- 
2.42.0

Re: [PATCH v1 3/7] xen: xenstore: add possibility to preserve owner

[David Woodhouse]

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

On Fri, 2023-11-10 at 20:42 +0000, Volodymyr Babchuk wrote:
> Add option to preserve owner when creating an entry in Xen Store. This
> may be needed in cases when Qemu is working as device model in a
> domain that is Domain-0, e.g. in driver domain.
> 
> "owner" parameter for qemu_xen_xs_create() function can have special
> value XS_PRESERVE_OWNER, which will make specific implementation to
> get original owner of an entry and pass it back to
> set_permissions() call.
> 
> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>

I like this, although I'd like it more if XenStore itself offered this
facility rather than making QEMU do it. Can we make it abundantly clear
that XS_PRESERVE_OWNER is a QEMU internal thing?

>  hw/i386/kvm/xen_xenstore.c       | 18 ++++++++++++++++++
>  hw/xen/xen-operations.c          | 12 ++++++++++++
>  include/hw/xen/xen_backend_ops.h |  2 ++
>  3 files changed, 32 insertions(+)
> 
> diff --git a/hw/i386/kvm/xen_xenstore.c b/hw/i386/kvm/xen_xenstore.c
> index 660d0b72f9..7b894a9884 100644
> --- a/hw/i386/kvm/xen_xenstore.c
> +++ b/hw/i386/kvm/xen_xenstore.c
> @@ -1572,6 +1572,24 @@ static bool xs_be_create(struct qemu_xs_handle *h, xs_transaction_t t,
>          return false;
>      }
>  
> +    if (owner == XS_PRESERVE_OWNER) {
> +        GList *perms;
> +        char letter;
> +
> +        err = xs_impl_get_perms(h->impl, 0, t, path, &perms);
> +        if (err) {
> +            errno = err;
> +            return false;
> +        }

I guess we get away without a race here because it's all internal and
we're holding the QEMU iothread mutex? Perhaps assert that?

> +        if (sscanf(perms->data, "%c%u", &letter, &owner) != 2) {

I'd be slightly happier if you used parse_perm() from xenstore_impl.c,
but it's static so I suppose that's fair enough.

> +            errno = EFAULT;
> +            g_list_free_full(perms, g_free);
> +            return false;
> +        }
> +        g_list_free_full(perms, g_free);
> +    }
> +
>      perms_list = g_list_append(perms_list,
>                                 xs_perm_as_string(XS_PERM_NONE, owner));
>      perms_list = g_list_append(perms_list,
> diff --git a/hw/xen/xen-operations.c b/hw/xen/xen-operations.c
> index e00983ec44..1df59b3c08 100644
> --- a/hw/xen/xen-operations.c
> +++ b/hw/xen/xen-operations.c
> @@ -300,6 +300,18 @@ static bool libxenstore_create(struct qemu_xs_handle *h, xs_transaction_t t,
>          return false;
>      }
>  
> +    if (owner == XS_PRESERVE_OWNER) {
> +        struct xs_permissions *tmp;
> +        unsigned int num;
> +
> +        tmp = xs_get_permissions(h->xsh, 0, path, &num);
> +        if (tmp == NULL) {
> +            return false;
> +        }
> +        perms_list[0].id = tmp[0].id;
> +        free(tmp);
> +    }
> +

Don't see what saves you from someone else changing it at this point on
true Xen though. Which is why I'd prefer XenStore to do it natively.

>      return xs_set_permissions(h->xsh, t, path, perms_list,
>                                ARRAY_SIZE(perms_list));
>  }
> diff --git a/include/hw/xen/xen_backend_ops.h b/include/hw/xen/xen_backend_ops.h
> index 90cca85f52..273e414559 100644
> --- a/include/hw/xen/xen_backend_ops.h
> +++ b/include/hw/xen/xen_backend_ops.h
> @@ -266,6 +266,8 @@ typedef uint32_t xs_transaction_t;
>  #define XS_PERM_READ  0x01
>  #define XS_PERM_WRITE 0x02
>  
> +#define XS_PRESERVE_OWNER        0xFFFE
> +
>  struct xenstore_backend_ops {
>      struct qemu_xs_handle *(*open)(void);
>      void (*close)(struct qemu_xs_handle *h);


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

Re: [PATCH v1 3/7] xen: xenstore: add possibility to preserve owner

[David Woodhouse]

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

On Sat, 2023-11-11 at 11:01 +0000, David Woodhouse wrote:
> 
> > --- a/hw/xen/xen-operations.c
> > +++ b/hw/xen/xen-operations.c
> > @@ -300,6 +300,18 @@ static bool libxenstore_create(struct qemu_xs_handle *h, xs_transaction_t t,
> >          return false;
> >      }
> >  
> > +    if (owner == XS_PRESERVE_OWNER) {
> > +        struct xs_permissions *tmp;
> > +        unsigned int num;
> > +
> > +        tmp = xs_get_permissions(h->xsh, 0, path, &num);
> > +        if (tmp == NULL) {
> > +            return false;
> > +        }
> > +        perms_list[0].id = tmp[0].id;
> > +        free(tmp);
> > +    }
> > +
> 
> Don't see what saves you from someone else changing it at this point on
> true Xen though. Which is why I'd prefer XenStore to do it natively.

I suppose maybe you could do it in a transaction *if* the transaction_t
you're passed in isn't already XBT_NULL?

One might argue that the mkdir+set_perms in libxenstore_create() ought
to have been within the same transaction *anyway*? 

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

Re: [PATCH v1 3/7] xen: xenstore: add possibility to preserve owner

[Volodymyr Babchuk]

Hi David,

David Woodhouse <dwmw2@infradead.org> writes:

> [[S/MIME Signed Part:Undecided]]
> On Sat, 2023-11-11 at 11:01 +0000, David Woodhouse wrote:
>> 
>> > --- a/hw/xen/xen-operations.c
>> > +++ b/hw/xen/xen-operations.c
>> > @@ -300,6 +300,18 @@ static bool libxenstore_create(struct qemu_xs_handle *h, xs_transaction_t t,
>> >          return false;
>> >      }
>> >  
>> > +    if (owner == XS_PRESERVE_OWNER) {
>> > +        struct xs_permissions *tmp;
>> > +        unsigned int num;
>> > +
>> > +        tmp = xs_get_permissions(h->xsh, 0, path, &num);
>> > +        if (tmp == NULL) {
>> > +            return false;
>> > +        }
>> > +        perms_list[0].id = tmp[0].id;
>> > +        free(tmp);
>> > +    }
>> > +
>> 
>> Don't see what saves you from someone else changing it at this point on
>> true Xen though. Which is why I'd prefer XenStore to do it natively.
>
> I suppose maybe you could do it in a transaction *if* the transaction_t
> you're passed in isn't already XBT_NULL?

Yes, I need to pass "t" to xs_get_permissions(), of course.

> One might argue that the mkdir+set_perms in libxenstore_create() ought
> to have been within the same transaction *anyway*? 

Yes, all operations should be performed inside one transaction.

-- 
WBR, Volodymyr

Re: [PATCH v1 3/7] xen: xenstore: add possibility to preserve owner

[Volodymyr Babchuk]

Hi David,

David Woodhouse <dwmw2@infradead.org> writes:

> [[S/MIME Signed Part:Undecided]]
> On Fri, 2023-11-10 at 20:42 +0000, Volodymyr Babchuk wrote:
>> Add option to preserve owner when creating an entry in Xen Store. This
>> may be needed in cases when Qemu is working as device model in a
>> domain that is Domain-0, e.g. in driver domain.
>> 
>> "owner" parameter for qemu_xen_xs_create() function can have special
>> value XS_PRESERVE_OWNER, which will make specific implementation to
>> get original owner of an entry and pass it back to
>> set_permissions() call.
>> 
>> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
>
> I like this, although I'd like it more if XenStore itself offered this
> facility rather than making QEMU do it.

XenStore itself ensures that access rights are inherited. The problem
is with qemu_xen_xs_create() function that does two things at a time:
creates a new entry and then assigns permissions, overwriting any
permissions that existed before.

> Can we make it abundantly clear
> that XS_PRESERVE_OWNER is a QEMU internal thing?

It is defined in xen_backend_ops.h which is internal QEMU interface for
XenStore. Do you have any suggestions how to make it even clearer?

>>  hw/i386/kvm/xen_xenstore.c       | 18 ++++++++++++++++++
>>  hw/xen/xen-operations.c          | 12 ++++++++++++
>>  include/hw/xen/xen_backend_ops.h |  2 ++
>>  3 files changed, 32 insertions(+)
>> 
>> diff --git a/hw/i386/kvm/xen_xenstore.c b/hw/i386/kvm/xen_xenstore.c
>> index 660d0b72f9..7b894a9884 100644
>> --- a/hw/i386/kvm/xen_xenstore.c
>> +++ b/hw/i386/kvm/xen_xenstore.c
>> @@ -1572,6 +1572,24 @@ static bool xs_be_create(struct qemu_xs_handle *h, xs_transaction_t t,
>>          return false;
>>      }
>>  
>> +    if (owner == XS_PRESERVE_OWNER) {
>> +        GList *perms;
>> +        char letter;
>> +
>> +        err = xs_impl_get_perms(h->impl, 0, t, path, &perms);
>> +        if (err) {
>> +            errno = err;
>> +            return false;
>> +        }
>
> I guess we get away without a race here because it's all internal and
> we're holding the QEMU iothread mutex? Perhaps assert that?
>

I am not quite familiar with QEMU internals, but why we do we need
assert here? xe_be_create() calls xs_impl* function before and after
this part. Is this piece of code special in some way?


>> +        if (sscanf(perms->data, "%c%u", &letter, &owner) != 2) {
>
> I'd be slightly happier if you used parse_perm() from xenstore_impl.c,
> but it's static so I suppose that's fair enough.
>

Yes, I wanted to use that function, but it is internal for
xenstore_impl.c

I can rename it to xs_impl_parse_perm() and make it public, if you
believe that this is a better approach.

>> +            errno = EFAULT;
>> +            g_list_free_full(perms, g_free);
>> +            return false;
>> +        }
>> +        g_list_free_full(perms, g_free);
>> +    }
>> +
>>      perms_list = g_list_append(perms_list,
>>                                 xs_perm_as_string(XS_PERM_NONE, owner));
>>      perms_list = g_list_append(perms_list,
>> diff --git a/hw/xen/xen-operations.c b/hw/xen/xen-operations.c
>> index e00983ec44..1df59b3c08 100644
>> --- a/hw/xen/xen-operations.c
>> +++ b/hw/xen/xen-operations.c
>> @@ -300,6 +300,18 @@ static bool libxenstore_create(struct qemu_xs_handle *h, xs_transaction_t t,
>>          return false;
>>      }
>>  
>> +    if (owner == XS_PRESERVE_OWNER) {
>> +        struct xs_permissions *tmp;
>> +        unsigned int num;
>> +
>> +        tmp = xs_get_permissions(h->xsh, 0, path, &num);
>> +        if (tmp == NULL) {
>> +            return false;
>> +        }
>> +        perms_list[0].id = tmp[0].id;
>> +        free(tmp);
>> +    }
>> +
>
> Don't see what saves you from someone else changing it at this point on
> true Xen though. Which is why I'd prefer XenStore to do it natively.
>

Oh, I missed the transaction parameter. My bad. Will fix in the next
version.

>>      return xs_set_permissions(h->xsh, t, path, perms_list,
>>                                ARRAY_SIZE(perms_list));
>>  }
>> diff --git a/include/hw/xen/xen_backend_ops.h b/include/hw/xen/xen_backend_ops.h
>> index 90cca85f52..273e414559 100644
>> --- a/include/hw/xen/xen_backend_ops.h
>> +++ b/include/hw/xen/xen_backend_ops.h
>> @@ -266,6 +266,8 @@ typedef uint32_t xs_transaction_t;
>>  #define XS_PERM_READ  0x01
>>  #define XS_PERM_WRITE 0x02
>>  
>> +#define XS_PRESERVE_OWNER        0xFFFE
>> +
>>  struct xenstore_backend_ops {
>>      struct qemu_xs_handle *(*open)(void);
>>      void (*close)(struct qemu_xs_handle *h);
>
> [[End of S/MIME Signed Part]]


-- 
WBR, Volodymyr

Re: [PATCH v1 3/7] xen: xenstore: add possibility to preserve owner

[Paul Durrant]

On 10/11/2023 15:42, Volodymyr Babchuk wrote:
> Add option to preserve owner when creating an entry in Xen Store. This
> may be needed in cases when Qemu is working as device model in a
> domain that is Domain-0, e.g. in driver domain.
> 
> "owner" parameter for qemu_xen_xs_create() function can have special
> value XS_PRESERVE_OWNER, which will make specific implementation to
> get original owner of an entry and pass it back to
> set_permissions() call.
> 

If QEMU is running in a driver domain then it should know whether the 
domid of the domain it is running in and use that. Yes, it is hardcoded 
to 0 at the moment but surely a backend domid (which defaults to 0) 
could be passed on the command line?

   Paul

> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
> ---
>   hw/i386/kvm/xen_xenstore.c       | 18 ++++++++++++++++++
>   hw/xen/xen-operations.c          | 12 ++++++++++++
>   include/hw/xen/xen_backend_ops.h |  2 ++
>   3 files changed, 32 insertions(+)
> 
> diff --git a/hw/i386/kvm/xen_xenstore.c b/hw/i386/kvm/xen_xenstore.c
> index 660d0b72f9..7b894a9884 100644
> --- a/hw/i386/kvm/xen_xenstore.c
> +++ b/hw/i386/kvm/xen_xenstore.c
> @@ -1572,6 +1572,24 @@ static bool xs_be_create(struct qemu_xs_handle *h, xs_transaction_t t,
>           return false;
>       }
>   
> +    if (owner == XS_PRESERVE_OWNER) {
> +        GList *perms;
> +        char letter;
> +
> +        err = xs_impl_get_perms(h->impl, 0, t, path, &perms);
> +        if (err) {
> +            errno = err;
> +            return false;
> +        }
> +
> +        if (sscanf(perms->data, "%c%u", &letter, &owner) != 2) {
> +            errno = EFAULT;
> +            g_list_free_full(perms, g_free);
> +            return false;
> +        }
> +        g_list_free_full(perms, g_free);
> +    }
> +
>       perms_list = g_list_append(perms_list,
>                                  xs_perm_as_string(XS_PERM_NONE, owner));
>       perms_list = g_list_append(perms_list,
> diff --git a/hw/xen/xen-operations.c b/hw/xen/xen-operations.c
> index e00983ec44..1df59b3c08 100644
> --- a/hw/xen/xen-operations.c
> +++ b/hw/xen/xen-operations.c
> @@ -300,6 +300,18 @@ static bool libxenstore_create(struct qemu_xs_handle *h, xs_transaction_t t,
>           return false;
>       }
>   
> +    if (owner == XS_PRESERVE_OWNER) {
> +        struct xs_permissions *tmp;
> +        unsigned int num;
> +
> +        tmp = xs_get_permissions(h->xsh, 0, path, &num);
> +        if (tmp == NULL) {
> +            return false;
> +        }
> +        perms_list[0].id = tmp[0].id;
> +        free(tmp);
> +    }
> +
>       return xs_set_permissions(h->xsh, t, path, perms_list,
>                                 ARRAY_SIZE(perms_list));
>   }
> diff --git a/include/hw/xen/xen_backend_ops.h b/include/hw/xen/xen_backend_ops.h
> index 90cca85f52..273e414559 100644
> --- a/include/hw/xen/xen_backend_ops.h
> +++ b/include/hw/xen/xen_backend_ops.h
> @@ -266,6 +266,8 @@ typedef uint32_t xs_transaction_t;
>   #define XS_PERM_READ  0x01
>   #define XS_PERM_WRITE 0x02
>   
> +#define XS_PRESERVE_OWNER        0xFFFE
> +
>   struct xenstore_backend_ops {
>       struct qemu_xs_handle *(*open)(void);
>       void (*close)(struct qemu_xs_handle *h);

[PATCH v1 4/7] xen_pvdev: Do not assume Dom0 when creating a directrory

[Volodymyr Babchuk]

From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>

Instead of forcing the owner to domid 0, use XS_PRESERVE_OWNER to save
the previous owner of the directory.

Note that for other than Dom0 domain (non toolstack domain) the
"driver_domain" property should be set in domain config file for the
toolstack to create required directories in advance.

Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
---
 hw/xen/xen_pvdev.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/xen/xen_pvdev.c b/hw/xen/xen_pvdev.c
index c5ad71e8dc..42bdd4f6c8 100644
--- a/hw/xen/xen_pvdev.c
+++ b/hw/xen/xen_pvdev.c
@@ -60,7 +60,8 @@ void xen_config_cleanup(void)
 
 int xenstore_mkdir(char *path, int p)
 {
-    if (!qemu_xen_xs_create(xenstore, 0, 0, xen_domid, p, path)) {
+    if (!qemu_xen_xs_create(xenstore, 0, XS_PRESERVE_OWNER,
+                            xen_domid, p, path)) {
         xen_pv_printf(NULL, 0, "xs_mkdir %s: failed\n", path);
         return -1;
     }
-- 
2.42.0

Re: [PATCH v1 4/7] xen_pvdev: Do not assume Dom0 when creating a directrory

[David Woodhouse]

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

On Fri, 2023-11-10 at 20:42 +0000, Volodymyr Babchuk wrote:
> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> 
> Instead of forcing the owner to domid 0, use XS_PRESERVE_OWNER to save
> the previous owner of the directory.
> 

You're missing the words "... if it already exists" from that sentence.

If the directory *didn't* already exist, it gets created with dom0 as
the owner still, right? Assuming XenStore allows QEMU to do that.

Strictly, the node gets created (if permitted) and *then*
xs_set_permissions() attempts to set dom0 as the owner (if permitted).

> Note that for other than Dom0 domain (non toolstack domain) the
> "driver_domain" property should be set in domain config file for the
> toolstack to create required directories in advance.
> 
> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
> ---
>  hw/xen/xen_pvdev.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/xen/xen_pvdev.c b/hw/xen/xen_pvdev.c
> index c5ad71e8dc..42bdd4f6c8 100644
> --- a/hw/xen/xen_pvdev.c
> +++ b/hw/xen/xen_pvdev.c
> @@ -60,7 +60,8 @@ void xen_config_cleanup(void)
>  
>  int xenstore_mkdir(char *path, int p)
>  {
> -    if (!qemu_xen_xs_create(xenstore, 0, 0, xen_domid, p, path)) {
> +    if (!qemu_xen_xs_create(xenstore, 0, XS_PRESERVE_OWNER,
> +                            xen_domid, p, path)) {
>          xen_pv_printf(NULL, 0, "xs_mkdir %s: failed\n", path);
>          return -1;
>      }

Why bother with xenstore_mkdir()? AFAICT it's *only* used from the
legacy XenLegacyDevice stuff, and can't we just finish the job of
moving from that to the XenDevice model? I've done console and net
recently; want to keep going?

And even then... the xenstore_mkdir() function is called twice from
xen_config_dev_dirs() in hw/xen/xen_devconfig.c to create the frontend
and backend directories — which is what the rest of your patch series
is trying to eliminate because a driver domain doesn't have permissions
to do that anyway.

It's also called from xen_be_register() in hw/xen/xen_devconfig.c to
create device-model/${GUEST_DOMID}/backends/${DEVICE_TYPE} (using a
relative path, so in the driver domain's XenStore). That one presumably
*won't* exist already, and so XS_PRESERVE_OWNER won't even have any
effect?

What practical difference does this even make? Am I missing something?

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

Re: [PATCH v1 4/7] xen_pvdev: Do not assume Dom0 when creating a directrory

[Volodymyr Babchuk]

Hi David,

David Woodhouse <dwmw2@infradead.org> writes:

> [[S/MIME Signed Part:Undecided]]
> On Fri, 2023-11-10 at 20:42 +0000, Volodymyr Babchuk wrote:
>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>> 
>> Instead of forcing the owner to domid 0, use XS_PRESERVE_OWNER to save
>> the previous owner of the directory.
>> 
>
> You're missing the words "... if it already exists" from that sentence.
>
> If the directory *didn't* already exist, it gets created with dom0 as
> the owner still, right? Assuming XenStore allows QEMU to do that.

If it didn't already exist, it is created and it inherits access rights
from the parent node.

> Strictly, the node gets created (if permitted) and *then*
> xs_set_permissions() attempts to set dom0 as the owner (if permitted).

Yes. I'll rephrase this as "Instead of forcing the owner to domid 0, use
 XS_PRESERVE_OWNER to save the inherited owner of the directory."

will it be okay?

>
>> Note that for other than Dom0 domain (non toolstack domain) the
>> "driver_domain" property should be set in domain config file for the
>> toolstack to create required directories in advance.
>> 
>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
>> ---
>>  hw/xen/xen_pvdev.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>> 
>> diff --git a/hw/xen/xen_pvdev.c b/hw/xen/xen_pvdev.c
>> index c5ad71e8dc..42bdd4f6c8 100644
>> --- a/hw/xen/xen_pvdev.c
>> +++ b/hw/xen/xen_pvdev.c
>> @@ -60,7 +60,8 @@ void xen_config_cleanup(void)
>>  
>>  int xenstore_mkdir(char *path, int p)
>>  {
>> -    if (!qemu_xen_xs_create(xenstore, 0, 0, xen_domid, p, path)) {
>> +    if (!qemu_xen_xs_create(xenstore, 0, XS_PRESERVE_OWNER,
>> +                            xen_domid, p, path)) {
>>          xen_pv_printf(NULL, 0, "xs_mkdir %s: failed\n", path);
>>          return -1;
>>      }
>
> Why bother with xenstore_mkdir()? AFAICT it's *only* used from the
> legacy XenLegacyDevice stuff, and can't we just finish the job of
> moving from that to the XenDevice model? I've done console and net
> recently; want to keep going?

Well, I am not so familiar with QEMU to accomplish this in a short
time. If you really need help, I can take alook at 9p driver, it looks
simplest of them all...

>
> And even then... the xenstore_mkdir() function is called twice from
> xen_config_dev_dirs() in hw/xen/xen_devconfig.c to create the frontend
> and backend directories — which is what the rest of your patch series
> is trying to eliminate because a driver domain doesn't have permissions
> to do that anyway.
>
> It's also called from xen_be_register() in hw/xen/xen_devconfig.c to
> create device-model/${GUEST_DOMID}/backends/${DEVICE_TYPE} (using a
> relative path, so in the driver domain's XenStore). That one presumably
> *won't* exist already, and so XS_PRESERVE_OWNER won't even have any
> effect?

As I said, it will inherit driver's domain access rights. So yeah,
Oleksandr's patch covers this case, mostly.

I agree, it would be better to drop xenstore_mkdir() at all, but this is
tangent to my task of adding virtio-pci support for ARM guests. Those
Oleksandr's patches for drive domain, that you are seeing, come to life
only because our system happens to use a separate driver domain.

-- 
WBR, Volodymyr

[PATCH v1 6/7] xen_arm: Set mc->max_cpus to GUEST_MAX_VCPUS in xen_arm_init()

[Volodymyr Babchuk]

From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>

The number of vCPUs used for the IOREQ configuration (machine->smp.cpus)
should really match the system value as for each vCPU we setup a dedicated
evtchn for the communication with Xen at the runtime. This is needed
for the IOREQ to be properly configured and work if the involved domain
has more than one vCPU assigned.

Set the number of current supported guest vCPUs here (128) which is
defined in public header arch-arm.h. And the toolstack should then
pass max_vcpus using "-smp" arg.

Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
---
 hw/arm/xen_arm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/arm/xen_arm.c b/hw/arm/xen_arm.c
index f83b983ec5..a2b41dc2de 100644
--- a/hw/arm/xen_arm.c
+++ b/hw/arm/xen_arm.c
@@ -229,7 +229,7 @@ static void xen_arm_machine_class_init(ObjectClass *oc, void *data)
     MachineClass *mc = MACHINE_CLASS(oc);
     mc->desc = "Xen Para-virtualized PC";
     mc->init = xen_arm_init;
-    mc->max_cpus = 1;
+    mc->max_cpus = GUEST_MAX_VCPUS;
     mc->default_machine_opts = "accel=xen";
     /* Set explicitly here to make sure that real ram_size is passed */
     mc->default_ram_size = 0;
-- 
2.42.0

[PATCH v1 5/7] xen-bus: Set offline if backend's state is XenbusStateClosed

[Volodymyr Babchuk]

From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>

Both state (XenbusStateClosed) and online (0) are expected by
toolstack/xl devd to completely destroy the device. But "offline"
is never being set by the backend resulting in timeout during
domain destruction, garbage in Xestore and still running Qemu
instance.

Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
---
 hw/xen/xen-bus.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/xen/xen-bus.c b/hw/xen/xen-bus.c
index 75474d4b43..6e7ec3af64 100644
--- a/hw/xen/xen-bus.c
+++ b/hw/xen/xen-bus.c
@@ -519,6 +519,10 @@ static void xen_device_backend_changed(void *opaque, const char *path)
         xen_device_backend_set_state(xendev, XenbusStateClosed);
     }
 
+    if (xen_device_backend_get_state(xendev) == XenbusStateClosed) {
+        xen_device_backend_set_online(xendev, false);
+    }
+
     /*
      * If a backend is still 'online' then we should leave it alone but,
      * if a backend is not 'online', then the device is a candidate
-- 
2.42.0

Re: [PATCH v1 5/7] xen-bus: Set offline if backend's state is XenbusStateClosed

[David Woodhouse]

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

(When sending a series, if you Cc someone on one message please could you Cc them on the whole thread for context? Thanks.)

> Both state (XenbusStateClosed) and online (0) are expected by
> toolstack/xl devd to completely destroy the device. But "offline"
> is never being set by the backend resulting in timeout during
> domain destruction, garbage in Xestore and still running Qemu
> instance.

I would dearly love to see a clear state diagram showing all the possible state transitions during device creation, fe/be reconnecting, and hot plug/unplug. Does anything like that exist? Any test cases for the common and the less common transitions, and even the illegal ones which need to be handled gracefully?

Fantasy aside, can you please confirm that this patch series was tested with hotplug (device_add in the monitor) *and* unplug in QEMU, both under real Xen and ideally also emulated Xen in KVM? 

Thanks.

[-- Attachment #2: Type: text/html, Size: 1029 bytes --]

Re: [PATCH v1 5/7] xen-bus: Set offline if backend's state is XenbusStateClosed

[Paul Durrant]

On 10/11/2023 15:42, Volodymyr Babchuk wrote:
> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> 
> Both state (XenbusStateClosed) and online (0) are expected by
> toolstack/xl devd to completely destroy the device. But "offline"
> is never being set by the backend resulting in timeout during
> domain destruction, garbage in Xestore and still running Qemu
> instance.
> 
> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
> ---
>   hw/xen/xen-bus.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/hw/xen/xen-bus.c b/hw/xen/xen-bus.c
> index 75474d4b43..6e7ec3af64 100644
> --- a/hw/xen/xen-bus.c
> +++ b/hw/xen/xen-bus.c
> @@ -519,6 +519,10 @@ static void xen_device_backend_changed(void *opaque, const char *path)
>           xen_device_backend_set_state(xendev, XenbusStateClosed);
>       }
>   
> +    if (xen_device_backend_get_state(xendev) == XenbusStateClosed) {
> +        xen_device_backend_set_online(xendev, false);
> +    }
> +
>       /*
>        * If a backend is still 'online' then we should leave it alone but,
>        * if a backend is not 'online', then the device is a candidate

I don't understand what you're trying to do here. Just a few lines up 
from this hunk there is:

  506    if (xen_device_backend_scanf(xendev, "online", "%u", &online) 
!= 1) {
  507        online = 0;
  508    }
  509
  510    xen_device_backend_set_online(xendev, !!online);

Why is this not sufficient? What happens if the frontend decides to stop 
and start (e.g. for a driver update)? I'm guessing the backend will be 
destroyed... which is not very friendly.

   Paul

[PATCH v1 7/7] xen_arm: Add basic virtio-pci support

[Volodymyr Babchuk]

From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>

This patch adds basic virtio-pci support for xen_arm machine.

It provides a flexible way to configure virtio-pci resources with
xenstore. We made this for several reasons:

- We don't want to clash with vPCI devices, so we need information
  from Xen toolstack on which PCI bus to use.
- The guest memory layout that describes these resources is not stable
  and may vary between guests, so we cannot rely on static resources
  to be always the same for both ends.
- Also the device-models which run in different domains and serve
  virtio-pci devices for the same guest should use different host
  bridge resources for Xen to distinguish. The rule for the guest
  device-tree generation is one PCI host bridge per backend domain.

Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
---
 hw/arm/xen_arm.c            | 186 ++++++++++++++++++++++++++++++++++++
 hw/xen/xen-hvm-common.c     |   9 +-
 include/hw/xen/xen_native.h |   8 +-
 3 files changed, 200 insertions(+), 3 deletions(-)

diff --git a/hw/arm/xen_arm.c b/hw/arm/xen_arm.c
index a2b41dc2de..4a0a6726a8 100644
--- a/hw/arm/xen_arm.c
+++ b/hw/arm/xen_arm.c
@@ -22,6 +22,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "qemu/cutils.h"
 #include "qemu/error-report.h"
 #include "qapi/qapi-commands-migration.h"
 #include "qapi/visitor.h"
@@ -34,6 +35,9 @@
 #include "hw/xen/xen-hvm-common.h"
 #include "sysemu/tpm.h"
 #include "hw/xen/arch_hvm.h"
+#include "exec/address-spaces.h"
+#include "hw/pci-host/gpex.h"
+#include "hw/virtio/virtio-pci.h"
 
 #define TYPE_XEN_ARM  MACHINE_TYPE_NAME("xenpvh")
 OBJECT_DECLARE_SIMPLE_TYPE(XenArmState, XEN_ARM)
@@ -58,6 +62,11 @@ struct XenArmState {
     struct {
         uint64_t tpm_base_addr;
     } cfg;
+
+    MemMapEntry pcie_mmio;
+    MemMapEntry pcie_ecam;
+    MemMapEntry pcie_mmio_high;
+    int         pcie_irq;
 };
 
 static MemoryRegion ram_lo, ram_hi;
@@ -73,6 +82,7 @@ static MemoryRegion ram_lo, ram_hi;
 #define NR_VIRTIO_MMIO_DEVICES   \
    (GUEST_VIRTIO_MMIO_SPI_LAST - GUEST_VIRTIO_MMIO_SPI_FIRST)
 
+/* TODO It should be xendevicemodel_set_pci_intx_level() for PCI interrupts. */
 static void xen_set_irq(void *opaque, int irq, int level)
 {
     xendevicemodel_set_irq_level(xen_dmod, xen_domid, irq, level);
@@ -127,6 +137,176 @@ static void xen_init_ram(MachineState *machine)
     }
 }
 
+static void xen_create_pcie(XenArmState *xam)
+{
+    MemoryRegion *mmio_alias, *mmio_alias_high, *mmio_reg;
+    MemoryRegion *ecam_alias, *ecam_reg;
+    DeviceState *dev;
+    int i;
+
+    dev = qdev_new(TYPE_GPEX_HOST);
+    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
+
+    /* Map ECAM space */
+    ecam_alias = g_new0(MemoryRegion, 1);
+    ecam_reg = sysbus_mmio_get_region(SYS_BUS_DEVICE(dev), 0);
+    memory_region_init_alias(ecam_alias, OBJECT(dev), "pcie-ecam",
+                             ecam_reg, 0, xam->pcie_ecam.size);
+    memory_region_add_subregion(get_system_memory(), xam->pcie_ecam.base,
+                                ecam_alias);
+
+    /* Map the MMIO space */
+    mmio_alias = g_new0(MemoryRegion, 1);
+    mmio_reg = sysbus_mmio_get_region(SYS_BUS_DEVICE(dev), 1);
+    memory_region_init_alias(mmio_alias, OBJECT(dev), "pcie-mmio",
+                             mmio_reg,
+                             xam->pcie_mmio.base,
+                             xam->pcie_mmio.size);
+    memory_region_add_subregion(get_system_memory(), xam->pcie_mmio.base,
+                                mmio_alias);
+
+    /* Map the MMIO_HIGH space */
+    mmio_alias_high = g_new0(MemoryRegion, 1);
+    memory_region_init_alias(mmio_alias_high, OBJECT(dev), "pcie-mmio-high",
+                             mmio_reg,
+                             xam->pcie_mmio_high.base,
+                             xam->pcie_mmio_high.size);
+    memory_region_add_subregion(get_system_memory(), xam->pcie_mmio_high.base,
+                                mmio_alias_high);
+
+    /* Legacy PCI interrupts (#INTA - #INTD) */
+    for (i = 0; i < GPEX_NUM_IRQS; i++) {
+        qemu_irq irq = qemu_allocate_irq(xen_set_irq, NULL,
+                                         xam->pcie_irq + i);
+
+        sysbus_connect_irq(SYS_BUS_DEVICE(dev), i, irq);
+        gpex_set_irq_num(GPEX_HOST(dev), i, xam->pcie_irq + i);
+    }
+
+    DPRINTF("Created PCIe host bridge\n");
+}
+
+static bool xen_read_pcie_prop(XenArmState *xam, unsigned int xen_domid,
+                               const char *prop_name, unsigned long *data)
+{
+    char path[128], *value = NULL;
+    unsigned int len;
+    bool ret = true;
+
+    snprintf(path, sizeof(path), "device-model/%d/virtio_pci_host/%s",
+             xen_domid, prop_name);
+    value = xs_read(xam->state->xenstore, XBT_NULL, path, &len);
+
+    if (qemu_strtou64(value, NULL, 16, data)) {
+        error_report("xenpv: Failed to get 'virtio_pci_host/%s' prop",
+                     prop_name);
+        ret = false;
+    }
+
+    free(value);
+
+    return ret;
+}
+
+static int xen_get_pcie_params(XenArmState *xam)
+{
+    char path[128], *value = NULL, **entries = NULL;
+    unsigned int len, tmp;
+    int rc = -1;
+
+    snprintf(path, sizeof(path), "device-model/%d/virtio_pci_host",
+             xen_domid);
+    entries = xs_directory(xam->state->xenstore, XBT_NULL, path, &len);
+    if (entries == NULL) {
+        error_report("xenpv: 'virtio_pci_host' dir is not present");
+        return -1;
+    }
+    free(entries);
+    if (len != 9) {
+        error_report("xenpv: Unexpected number of entries in 'virtio_pci_host' dir");
+        goto out;
+    }
+
+    snprintf(path, sizeof(path), "device-model/%d/virtio_pci_host/id",
+             xen_domid);
+    value = xs_read(xam->state->xenstore, XBT_NULL, path, &len);
+    if (qemu_strtoui(value, NULL, 10, &tmp)) {
+        error_report("xenpv: Failed to get 'virtio_pci_host/id' prop");
+        goto out;
+    }
+    free(value);
+    value = NULL;
+    if (tmp > 0xffff) {
+        error_report("xenpv: Wrong 'virtio_pci_host/id' value %u", tmp);
+        goto out;
+    }
+    xen_pci_segment = tmp;
+
+    if (!xen_read_pcie_prop(xam, xen_domid, "ecam_base",
+                            &xam->pcie_ecam.base)) {
+        goto out;
+    }
+
+    if (!xen_read_pcie_prop(xam, xen_domid, "ecam_size",
+                            &xam->pcie_ecam.size)) {
+        goto out;
+    }
+
+    if (!xen_read_pcie_prop(xam, xen_domid, "mem_base",
+                            &xam->pcie_mmio.base)) {
+        goto out;
+    }
+
+    if (!xen_read_pcie_prop(xam, xen_domid, "mem_size",
+                            &xam->pcie_mmio.base)) {
+        goto out;
+    }
+
+    if (!xen_read_pcie_prop(xam, xen_domid, "prefetch_mem_base",
+                            &xam->pcie_mmio_high.base)) {
+        goto out;
+    }
+
+    if (!xen_read_pcie_prop(xam, xen_domid, "prefetch_mem_size",
+                            &xam->pcie_mmio_high.size)) {
+        goto out;
+    }
+
+    snprintf(path, sizeof(path), "device-model/%d/virtio_pci_host/irq_first",
+             xen_domid);
+    value = xs_read(xam->state->xenstore, XBT_NULL, path, &len);
+    if (qemu_strtoi(value, NULL, 10, &xam->pcie_irq)) {
+        error_report("xenpv: Failed to get 'virtio_pci_host/irq_first' prop");
+        goto out;
+    }
+    free(value);
+    value = NULL;
+    DPRINTF("PCIe host bridge: irq_first %u\n", xam->pcie_irq);
+
+    snprintf(path, sizeof(path), "device-model/%d/virtio_pci_host/num_irqs",
+             xen_domid);
+    value = xs_read(xam->state->xenstore, XBT_NULL, path, &len);
+    if (qemu_strtoui(value, NULL, 10, &tmp)) {
+        error_report("xenpv: Failed to get 'virtio_pci_host/num_irqs' prop");
+        goto out;
+    }
+    free(value);
+    value = NULL;
+    if (tmp != GPEX_NUM_IRQS) {
+        error_report("xenpv: Wrong 'virtio_pci_host/num_irqs' value %u", tmp);
+        goto out;
+    }
+    DPRINTF("PCIe host bridge: num_irqs %u\n", tmp);
+
+    rc = 0;
+out:
+    if (value) {
+        free(value);
+    }
+
+    return rc;
+}
+
 void arch_handle_ioreq(XenIOState *state, ioreq_t *req)
 {
     hw_error("Invalid ioreq type 0x%x\n", req->type);
@@ -187,6 +367,12 @@ static void xen_arm_init(MachineState *machine)
     xen_register_ioreq(xam->state, machine->smp.cpus, &xen_memory_listener);
 
     xen_create_virtio_mmio_devices(xam);
+    if (!xen_get_pcie_params(xam)) {
+        xen_create_pcie(xam);
+    } else {
+        DPRINTF("PCIe host bridge is not available,"
+                "only virtio-mmio can be used\n");
+    }
 
 #ifdef CONFIG_TPM
     if (xam->cfg.tpm_base_addr) {
diff --git a/hw/xen/xen-hvm-common.c b/hw/xen/xen-hvm-common.c
index b7255977a5..b1eb2972d5 100644
--- a/hw/xen/xen-hvm-common.c
+++ b/hw/xen/xen-hvm-common.c
@@ -47,6 +47,8 @@ void xen_ram_alloc(ram_addr_t ram_addr, ram_addr_t size, MemoryRegion *mr,
     g_free(pfn_list);
 }
 
+uint16_t xen_pci_segment;
+
 static void xen_set_memory(struct MemoryListener *listener,
                            MemoryRegionSection *section,
                            bool add)
@@ -382,7 +384,12 @@ static void cpu_ioreq_config(XenIOState *state, ioreq_t *req)
     }
 
     QLIST_FOREACH(xendev, &state->dev_list, entry) {
-        if (xendev->sbdf != sbdf) {
+        /*
+         * As we append xen_pci_segment just before forming dm_op in
+         * xen_map_pcidev() we need to check with appended xen_pci_segment
+         * here as well.
+         */
+        if ((xendev->sbdf | (xen_pci_segment << 16)) != sbdf) {
             continue;
         }
 
diff --git a/include/hw/xen/xen_native.h b/include/hw/xen/xen_native.h
index 5d2718261f..a8fac902f1 100644
--- a/include/hw/xen/xen_native.h
+++ b/include/hw/xen/xen_native.h
@@ -431,6 +431,8 @@ static inline void xen_unmap_io_section(domid_t dom,
                                                     0, start_addr, end_addr);
 }
 
+extern uint16_t xen_pci_segment;
+
 static inline void xen_map_pcidev(domid_t dom,
                                   ioservid_t ioservid,
                                   PCIDevice *pci_dev)
@@ -441,7 +443,8 @@ static inline void xen_map_pcidev(domid_t dom,
 
     trace_xen_map_pcidev(ioservid, pci_dev_bus_num(pci_dev),
                          PCI_SLOT(pci_dev->devfn), PCI_FUNC(pci_dev->devfn));
-    xendevicemodel_map_pcidev_to_ioreq_server(xen_dmod, dom, ioservid, 0,
+    xendevicemodel_map_pcidev_to_ioreq_server(xen_dmod, dom, ioservid,
+                                              xen_pci_segment,
                                               pci_dev_bus_num(pci_dev),
                                               PCI_SLOT(pci_dev->devfn),
                                               PCI_FUNC(pci_dev->devfn));
@@ -457,7 +460,8 @@ static inline void xen_unmap_pcidev(domid_t dom,
 
     trace_xen_unmap_pcidev(ioservid, pci_dev_bus_num(pci_dev),
                            PCI_SLOT(pci_dev->devfn), PCI_FUNC(pci_dev->devfn));
-    xendevicemodel_unmap_pcidev_from_ioreq_server(xen_dmod, dom, ioservid, 0,
+    xendevicemodel_unmap_pcidev_from_ioreq_server(xen_dmod, dom, ioservid,
+                                                  xen_pci_segment,
                                                   pci_dev_bus_num(pci_dev),
                                                   PCI_SLOT(pci_dev->devfn),
                                                   PCI_FUNC(pci_dev->devfn));
-- 
2.42.0

Re: [PATCH v1 7/7] xen_arm: Add basic virtio-pci support

[David Woodhouse]

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

On Fri, 2023-11-10 at 20:42 +0000, Volodymyr Babchuk wrote:
> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> 
> This patch adds basic virtio-pci support for xen_arm machine.

Why only xen_arm? Couldn't this be a fairly generic device which can be
instantiated on x86 too, both for real and emulated Xen guests? And
riscv/ppc too?

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

Re: [PATCH v1 7/7] xen_arm: Add basic virtio-pci support

[Volodymyr Babchuk]

Hi David,

David Woodhouse <dwmw2@infradead.org> writes:

> [[S/MIME Signed Part:Undecided]]
> On Fri, 2023-11-10 at 20:42 +0000, Volodymyr Babchuk wrote:
>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>> 
>> This patch adds basic virtio-pci support for xen_arm machine.
>
> Why only xen_arm? Couldn't this be a fairly generic device which can be
> instantiated on x86 too, both for real and emulated Xen guests? And
> riscv/ppc too?

This is the architecture-specific code. Actually, from QEMU point of
view, this code just adds a virtual PCI bridge. For example, on x86 this
is done in a some other way, so they already have virtio-pci working.

-- 
WBR, Volodymyr