Re: [PATCH v2 2/2] xen: fix stubdom PCI addr
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-a1c2ccc48...@linaro.org/ materialise. Beside the coding style, the patch looks file. Thanks, -- Anthony PERARD
Re: [PATCH v2 2/2] xen: fix stubdom PCI addr
On Tue, Mar 5, 2024 at 2:13 PM Marek Marczykowski-Górecki
wrote:
>
> From: Frédéric Pierret (fepitre)
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
> ---
> 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 0x1000 /* No side effects */
> #define IORESOURCE_MEM_64 0x0010
>
> +/*
> + * 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", );
> +if (!be_path)
error_setg() here?
> +goto out;
> +
> +if (xs_node_scanf(xenstore, 0, be_path, "num_devs", NULL, "%d",
> _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", , , , ) != 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", , , , ) != 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);
> +}
> +
[PATCH v2 2/2] xen: fix stubdom PCI addr
From: Frédéric Pierret (fepitre)
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
---
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 0x1000 /* No side effects */
#define IORESOURCE_MEM_64 0x0010
+/*
+ * 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", );
+if (!be_path)
+goto out;
+
+if (xs_node_scanf(xenstore, 0, be_path, "num_devs", NULL, "%d", _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", , , , ) != 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", , , , ) != 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
