[PATCH v2 1/2] hw/xen: detect when running inside stubdomain

[PATCH v2 1/2] hw/xen: detect when running inside stubdomain

[Marek Marczykowski-Górecki]

Introduce global xen_is_stubdomain variable when qemu is running inside
a stubdomain instead of dom0. This will be relevant for subsequent
patches, as few things like accessing PCI config space need to be done
differently.

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
---
Changes in v2:
- use sigend int for domid to match xenstore_read_int() types, domid is
  in a signed range anyway
- fix code style
---
 hw/xen/xen-legacy-backend.c | 16 ++++++++++++++++
 include/hw/xen/xen.h        |  1 +
 system/globals.c            |  1 +
 3 files changed, 18 insertions(+)

diff --git a/hw/xen/xen-legacy-backend.c b/hw/xen/xen-legacy-backend.c
index 124dd5f3d6..6bd4e6eb2f 100644
--- a/hw/xen/xen-legacy-backend.c
+++ b/hw/xen/xen-legacy-backend.c
@@ -603,6 +603,20 @@ static void xen_set_dynamic_sysbus(void)
     machine_class_allow_dynamic_sysbus_dev(mc, TYPE_XENSYSDEV);
 }
 
+static bool xen_check_stubdomain(void)
+{
+    char *dm_path = g_strdup_printf("/local/domain/%d/image", xen_domid);
+    int32_t dm_domid;
+    bool is_stubdom = false;
+
+    if (!xenstore_read_int(dm_path, "device-model-domid", &dm_domid)) {
+        is_stubdom = dm_domid != 0;
+    }
+
+    g_free(dm_path);
+    return is_stubdom;
+}
+
 void xen_be_init(void)
 {
     xenstore = qemu_xen_xs_open();
@@ -616,6 +630,8 @@ void xen_be_init(void)
         exit(1);
     }
 
+    xen_is_stubdomain = xen_check_stubdomain();
+
     xen_sysdev = qdev_new(TYPE_XENSYSDEV);
     sysbus_realize_and_unref(SYS_BUS_DEVICE(xen_sysdev), &error_fatal);
     xen_sysbus = qbus_new(TYPE_XENSYSBUS, xen_sysdev, "xen-sysbus");
diff --git a/include/hw/xen/xen.h b/include/hw/xen/xen.h
index 37ecc91fc3..ecb89ecfc1 100644
--- a/include/hw/xen/xen.h
+++ b/include/hw/xen/xen.h
@@ -36,6 +36,7 @@ enum xen_mode {
 extern uint32_t xen_domid;
 extern enum xen_mode xen_mode;
 extern bool xen_domid_restrict;
+extern bool xen_is_stubdomain;
 
 int xen_pci_slot_get_pirq(PCIDevice *pci_dev, int irq_num);
 int xen_set_pci_link_route(uint8_t link, uint8_t irq);
diff --git a/system/globals.c b/system/globals.c
index b6d4e72530..ac27d88bd4 100644
--- a/system/globals.c
+++ b/system/globals.c
@@ -62,6 +62,7 @@ bool qemu_uuid_set;
 uint32_t xen_domid;
 enum xen_mode xen_mode = XEN_DISABLED;
 bool xen_domid_restrict;
+bool xen_is_stubdomain;
 struct evtchn_backend_ops *xen_evtchn_ops;
 struct gnttab_backend_ops *xen_gnttab_ops;
 struct foreignmem_backend_ops *xen_foreignmem_ops;
-- 
2.43.0

[PATCH v2 2/2] xen: fix stubdom PCI addr

[Marek Marczykowski-Górecki]

From: Frédéric Pierret (fepitre) <frederic.pierret@qubes-os.org>

When running in a stubdomain, the config space access via sysfs needs to
use BDF as seen inside stubdomain (connected via xen-pcifront), which is
different from the real BDF. For other purposes (hypercall parameters
etc), the real BDF needs to be used.
Get the in-stubdomain BDF by looking up relevant PV PCI xenstore
entries.

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
---
Changes in v2:
- use xs_node_scanf
- use %d instead of %u to read values written as %d
- add a comment from another iteration of this patch by Jason Andryuk
---
 hw/xen/xen-host-pci-device.c | 69 +++++++++++++++++++++++++++++++++++-
 hw/xen/xen-host-pci-device.h |  6 ++++
 2 files changed, 74 insertions(+), 1 deletion(-)

diff --git a/hw/xen/xen-host-pci-device.c b/hw/xen/xen-host-pci-device.c
index 8c6e9a1716..8ea2a5a4af 100644
--- a/hw/xen/xen-host-pci-device.c
+++ b/hw/xen/xen-host-pci-device.c
@@ -9,6 +9,8 @@
 #include "qemu/osdep.h"
 #include "qapi/error.h"
 #include "qemu/cutils.h"
+#include "hw/xen/xen-legacy-backend.h"
+#include "hw/xen/xen-bus-helper.h"
 #include "xen-host-pci-device.h"
 
 #define XEN_HOST_PCI_MAX_EXT_CAP \
@@ -33,13 +35,67 @@
 #define IORESOURCE_PREFETCH     0x00001000      /* No side effects */
 #define IORESOURCE_MEM_64       0x00100000
 
+/*
+ * Non-passthrough (dom0) accesses are local PCI devices and use the given BDF
+ * Passthough (stubdom) accesses are through PV frontend PCI device.  Those
+ * either have a BDF identical to the backend's BDF (xen-backend.passthrough=1)
+ * or a local virtual BDF (xen-backend.passthrough=0)
+ *
+ * We are always given the backend's BDF and need to lookup the appropriate
+ * local BDF for sysfs access.
+ */
+static void xen_host_pci_fill_local_addr(XenHostPCIDevice *d, Error **errp)
+{
+    unsigned int num_devs, len, i;
+    unsigned int domain, bus, dev, func;
+    char *be_path = NULL;
+    char path[80];
+
+    be_path = qemu_xen_xs_read(xenstore, 0, "device/pci/0/backend", &len);
+    if (!be_path)
+        goto out;
+
+    if (xs_node_scanf(xenstore, 0, be_path, "num_devs", NULL, "%d", &num_devs) != 1) {
+        error_setg(errp, "Failed to read or parse %s/num_devs\n", be_path);
+        goto out;
+    }
+
+    for (i = 0; i < num_devs; i++) {
+        snprintf(path, sizeof(path), "dev-%d", i);
+        if (xs_node_scanf(xenstore, 0, be_path, path, NULL,
+                          "%x:%x:%x.%x", &domain, &bus, &dev, &func) != 4) {
+            error_setg(errp, "Failed to read or parse %s/%s\n", be_path, path);
+            goto out;
+        }
+        if (domain != d->domain ||
+                bus != d->bus ||
+                dev != d->dev ||
+                func!= d->func)
+            continue;
+        snprintf(path, sizeof(path), "vdev-%d", i);
+        if (xs_node_scanf(xenstore, 0, be_path, path, NULL,
+                          "%x:%x:%x.%x", &domain, &bus, &dev, &func) != 4) {
+            error_setg(errp, "Failed to read or parse %s/%s\n", be_path, path);
+            goto out;
+        }
+        d->local_domain = domain;
+        d->local_bus = bus;
+        d->local_dev = dev;
+        d->local_func = func;
+        goto out;
+    }
+
+out:
+    free(be_path);
+}
+
 static void xen_host_pci_sysfs_path(const XenHostPCIDevice *d,
                                     const char *name, char *buf, ssize_t size)
 {
     int rc;
 
     rc = snprintf(buf, size, "/sys/bus/pci/devices/%04x:%02x:%02x.%d/%s",
-                  d->domain, d->bus, d->dev, d->func, name);
+                  d->local_domain, d->local_bus, d->local_dev, d->local_func, name);
     assert(rc >= 0 && rc < size);
 }
 
@@ -342,6 +398,17 @@ void xen_host_pci_device_get(XenHostPCIDevice *d, uint16_t domain,
     d->dev = dev;
     d->func = func;
 
+    if (xen_is_stubdomain) {
+        xen_host_pci_fill_local_addr(d, errp);
+        if (*errp)
+            goto error;
+    } else {
+        d->local_domain = d->domain;
+        d->local_bus = d->bus;
+        d->local_dev = d->dev;
+        d->local_func = d->func;
+    }
+
     xen_host_pci_config_open(d, errp);
     if (*errp) {
         goto error;
diff --git a/hw/xen/xen-host-pci-device.h b/hw/xen/xen-host-pci-device.h
index 4d8d34ecb0..270dcb27f7 100644
--- a/hw/xen/xen-host-pci-device.h
+++ b/hw/xen/xen-host-pci-device.h
@@ -23,6 +23,12 @@ typedef struct XenHostPCIDevice {
     uint8_t dev;
     uint8_t func;
 
+    /* different from the above in case of stubdomain */
+    uint16_t local_domain;
+    uint8_t local_bus;
+    uint8_t local_dev;
+    uint8_t local_func;
+
     uint16_t vendor_id;
     uint16_t device_id;
     uint32_t class_code;
-- 
2.43.0

Re: [PATCH v2 2/2] xen: fix stubdom PCI addr

[Jason Andryuk]

On Tue, Mar 5, 2024 at 2:13 PM Marek Marczykowski-Górecki
<marmarek@invisiblethingslab.com> wrote:
>
> From: Frédéric Pierret (fepitre) <frederic.pierret@qubes-os.org>

Needs to be changed to Marek.

> When running in a stubdomain, the config space access via sysfs needs to
> use BDF as seen inside stubdomain (connected via xen-pcifront), which is
> different from the real BDF. For other purposes (hypercall parameters
> etc), the real BDF needs to be used.
> Get the in-stubdomain BDF by looking up relevant PV PCI xenstore
> entries.
>
> Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> ---
> Changes in v2:
> - use xs_node_scanf
> - use %d instead of %u to read values written as %d
> - add a comment from another iteration of this patch by Jason Andryuk
> ---
>  hw/xen/xen-host-pci-device.c | 69 +++++++++++++++++++++++++++++++++++-
>  hw/xen/xen-host-pci-device.h |  6 ++++
>  2 files changed, 74 insertions(+), 1 deletion(-)
>
> diff --git a/hw/xen/xen-host-pci-device.c b/hw/xen/xen-host-pci-device.c
> index 8c6e9a1716..8ea2a5a4af 100644
> --- a/hw/xen/xen-host-pci-device.c
> +++ b/hw/xen/xen-host-pci-device.c
> @@ -9,6 +9,8 @@
>  #include "qemu/osdep.h"
>  #include "qapi/error.h"
>  #include "qemu/cutils.h"
> +#include "hw/xen/xen-legacy-backend.h"
> +#include "hw/xen/xen-bus-helper.h"
>  #include "xen-host-pci-device.h"
>
>  #define XEN_HOST_PCI_MAX_EXT_CAP \
> @@ -33,13 +35,67 @@
>  #define IORESOURCE_PREFETCH     0x00001000      /* No side effects */
>  #define IORESOURCE_MEM_64       0x00100000
>
> +/*
> + * Non-passthrough (dom0) accesses are local PCI devices and use the given BDF
> + * Passthough (stubdom) accesses are through PV frontend PCI device.  Those
> + * either have a BDF identical to the backend's BDF (xen-backend.passthrough=1)
> + * or a local virtual BDF (xen-backend.passthrough=0)
> + *
> + * We are always given the backend's BDF and need to lookup the appropriate
> + * local BDF for sysfs access.
> + */
> +static void xen_host_pci_fill_local_addr(XenHostPCIDevice *d, Error **errp)
> +{
> +    unsigned int num_devs, len, i;
> +    unsigned int domain, bus, dev, func;
> +    char *be_path = NULL;
> +    char path[80];

path is now only used for dev/vdev-%d, so 80 could be reduced.

> +
> +    be_path = qemu_xen_xs_read(xenstore, 0, "device/pci/0/backend", &len);
> +    if (!be_path)

error_setg() here?

> +        goto out;
> +
> +    if (xs_node_scanf(xenstore, 0, be_path, "num_devs", NULL, "%d", &num_devs) != 1) {
> +        error_setg(errp, "Failed to read or parse %s/num_devs\n", be_path);
> +        goto out;
> +    }
> +
> +    for (i = 0; i < num_devs; i++) {
> +        snprintf(path, sizeof(path), "dev-%d", i);
> +        if (xs_node_scanf(xenstore, 0, be_path, path, NULL,
> +                          "%x:%x:%x.%x", &domain, &bus, &dev, &func) != 4) {
> +            error_setg(errp, "Failed to read or parse %s/%s\n", be_path, path);
> +            goto out;
> +        }
> +        if (domain != d->domain ||
> +                bus != d->bus ||
> +                dev != d->dev ||
> +                func!= d->func)
> +            continue;
> +        snprintf(path, sizeof(path), "vdev-%d", i);
> +        if (xs_node_scanf(xenstore, 0, be_path, path, NULL,
> +                          "%x:%x:%x.%x", &domain, &bus, &dev, &func) != 4) {
> +            error_setg(errp, "Failed to read or parse %s/%s\n", be_path, path);
> +            goto out;
> +        }
> +        d->local_domain = domain;
> +        d->local_bus = bus;
> +        d->local_dev = dev;
> +        d->local_func = func;
> +        goto out;
> +    }

error_setg here in case we exited the loop without finding a match?

Thanks,
Jason

> +
> +out:
> +    free(be_path);
> +}
> +

Re: [PATCH v2 2/2] xen: fix stubdom PCI addr

[Anthony PERARD]

First things first, could you fix the coding style?

Run something like `./scripts/checkpatch.pl @^..` or
`./scripts/checkpatch.pl master..`. Patchew might have run that for you
if the patch series had a cover letter.

On Tue, Mar 05, 2024 at 08:12:30PM +0100, Marek Marczykowski-Górecki wrote:
> diff --git a/hw/xen/xen-host-pci-device.c b/hw/xen/xen-host-pci-device.c
> index 8c6e9a1716..8ea2a5a4af 100644
> --- a/hw/xen/xen-host-pci-device.c
> +++ b/hw/xen/xen-host-pci-device.c
> @@ -9,6 +9,8 @@
>  #include "qemu/osdep.h"
>  #include "qapi/error.h"
>  #include "qemu/cutils.h"
> +#include "hw/xen/xen-legacy-backend.h"

I'd like to avoid this header here, that would be complicated at the
moment, as the global variable `xenstore` would be missing. So for now,
that's fine. I guess that could be rework if something like Philippe
talked about at
https://lore.kernel.org/qemu-devel/429a5a27-21b9-45bd-a1a6-a1c2ccc484c9@linaro.org/
materialise.


Beside the coding style, the patch looks file.

Thanks,

-- 
Anthony PERARD

Re: [PATCH v2 1/2] hw/xen: detect when running inside stubdomain

[Jason Andryuk]

On Tue, Mar 5, 2024 at 2:13 PM Marek Marczykowski-Górecki
<marmarek@invisiblethingslab.com> wrote:
>
> Introduce global xen_is_stubdomain variable when qemu is running inside
> a stubdomain instead of dom0. This will be relevant for subsequent
> patches, as few things like accessing PCI config space need to be done
> differently.
>
> Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>

Reviewed-by: Jason Andryuk <jandryuk@gmail.com>

Re: [PATCH v2 1/2] hw/xen: detect when running inside stubdomain

[Anthony PERARD]

On Tue, Mar 05, 2024 at 08:12:29PM +0100, Marek Marczykowski-Górecki wrote:
> diff --git a/hw/xen/xen-legacy-backend.c b/hw/xen/xen-legacy-backend.c
> index 124dd5f3d6..6bd4e6eb2f 100644
> --- a/hw/xen/xen-legacy-backend.c
> +++ b/hw/xen/xen-legacy-backend.c
> @@ -603,6 +603,20 @@ static void xen_set_dynamic_sysbus(void)
>      machine_class_allow_dynamic_sysbus_dev(mc, TYPE_XENSYSDEV);
>  }
>  
> +static bool xen_check_stubdomain(void)
> +{
> +    char *dm_path = g_strdup_printf("/local/domain/%d/image", xen_domid);
> +    int32_t dm_domid;
> +    bool is_stubdom = false;
> +
> +    if (!xenstore_read_int(dm_path, "device-model-domid", &dm_domid)) {
> +        is_stubdom = dm_domid != 0;
> +    }
> +
> +    g_free(dm_path);
> +    return is_stubdom;
> +}
> +
>  void xen_be_init(void)
>  {
>      xenstore = qemu_xen_xs_open();
> @@ -616,6 +630,8 @@ void xen_be_init(void)
>          exit(1);
>      }
>  
> +    xen_is_stubdomain = xen_check_stubdomain();

This isn't really a backend specific information, and xen_be_init() is
all about old backend implementation support. (qdisk which have been the
first to be rewritten doesn't need xen_be_init(), or shouldn't). Could
we move the initialisation elsewhere?

Is this relevant PV guests? If not, we could move the initialisation to
xen_hvm_init_pc().

Also, avoid having xen_check_stubdomain() depending on
"xen-legacy-backend", if possible.

(In xen_hvm_init_pc(), a call to xen_register_ioreq() opens another
xenstore, as `state->xenstore`.)

(There's already been effort to build QEMU without legacy backends, that
stubdom check would break in this scenario.)

Thanks,

-- 
Anthony PERARD

Re: [PATCH v2 1/2] hw/xen: detect when running inside stubdomain

[Marek Marczykowski-Górecki]

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

On Tue, Mar 26, 2024 at 05:06:50PM +0000, Anthony PERARD wrote:
> On Tue, Mar 05, 2024 at 08:12:29PM +0100, Marek Marczykowski-Górecki wrote:
> > diff --git a/hw/xen/xen-legacy-backend.c b/hw/xen/xen-legacy-backend.c
> > index 124dd5f3d6..6bd4e6eb2f 100644
> > --- a/hw/xen/xen-legacy-backend.c
> > +++ b/hw/xen/xen-legacy-backend.c
> > @@ -603,6 +603,20 @@ static void xen_set_dynamic_sysbus(void)
> >      machine_class_allow_dynamic_sysbus_dev(mc, TYPE_XENSYSDEV);
> >  }
> >  
> > +static bool xen_check_stubdomain(void)
> > +{
> > +    char *dm_path = g_strdup_printf("/local/domain/%d/image", xen_domid);
> > +    int32_t dm_domid;
> > +    bool is_stubdom = false;
> > +
> > +    if (!xenstore_read_int(dm_path, "device-model-domid", &dm_domid)) {
> > +        is_stubdom = dm_domid != 0;
> > +    }
> > +
> > +    g_free(dm_path);
> > +    return is_stubdom;
> > +}
> > +
> >  void xen_be_init(void)
> >  {
> >      xenstore = qemu_xen_xs_open();
> > @@ -616,6 +630,8 @@ void xen_be_init(void)
> >          exit(1);
> >      }
> >  
> > +    xen_is_stubdomain = xen_check_stubdomain();
> 
> This isn't really a backend specific information, and xen_be_init() is
> all about old backend implementation support. (qdisk which have been the
> first to be rewritten doesn't need xen_be_init(), or shouldn't). Could
> we move the initialisation elsewhere?

I can try to move it, sure.

> Is this relevant PV guests? If not, we could move the initialisation to
> xen_hvm_init_pc().
> 
> Also, avoid having xen_check_stubdomain() depending on
> "xen-legacy-backend", if possible.
> 
> (In xen_hvm_init_pc(), a call to xen_register_ioreq() opens another
> xenstore, as `state->xenstore`.)

And xen_register_ioreq() calls xen_be_init() anyway, so it wouldn't
change much in practice (at least for now)...

> (There's already been effort to build QEMU without legacy backends, that
> stubdom check would break in this scenario.)

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]