[PATCH v3 0/5] xen-arm: add support for virtio-pci

[PATCH v3 0/5] 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 virtual PCIe host bridge
support", while most of other patches are required to make QEMU work
as device model in a non-privileged domains like driver domain.

New in version 3:

 - Use commandline/properties instead of xenstore entries to configure
   PCIe bridge
 - Instead of trying to fix legacy backends, just add option to disable
   them

David Woodhouse (1):
  hw/xen: Set XenBackendInstance in the XenDevice before realizing it

Oleksandr Tyshchenko (2):
  xen_arm: set mc->max_cpus to GUEST_MAX_VCPUS
  xen_arm: Add virtual PCIe host bridge support

Volodymyr Babchuk (2):
  xen: backends: don't overwrite XenStore nodes created by toolstack
  xen: add option to disable legacy backends

 accel/xen/xen-all.c           |  13 +-
 hw/9pfs/meson.build           |   4 +-
 hw/arm/xen_arm.c              | 228 +++++++++++++++++++++++++++++++++-
 hw/block/xen-block.c          |  19 +--
 hw/char/xen_console.c         |   4 +-
 hw/display/meson.build        |   4 +-
 hw/i386/pc.c                  |   2 +
 hw/net/xen_nic.c              |  20 +--
 hw/usb/meson.build            |   5 +-
 hw/xen/meson.build            |  11 +-
 hw/xen/xen-backend.c          |  15 +--
 hw/xen/xen-bus.c              |  18 ++-
 hw/xen/xen-hvm-common.c       |  11 +-
 hw/xen/xen-legacy-backend.c   |   7 --
 hw/xenpv/xen_machine_pv.c     |   2 +
 include/hw/xen/xen-backend.h  |   2 -
 include/hw/xen/xen-bus.h      |   2 +
 include/hw/xen/xen_native.h   |   8 +-
 meson.build                   |   5 +
 meson_options.txt             |   2 +
 scripts/meson-buildoptions.sh |   4 +
 21 files changed, 325 insertions(+), 61 deletions(-)

-- 
2.42.0

[PATCH v3 2/5] xen: backends: don't overwrite XenStore nodes created by toolstack

[Volodymyr Babchuk]

Xen PV devices in QEMU can be created in two ways: either by QEMU
itself, if they were passed via command line, or by Xen toolstack. In
the latter case, QEMU scans XenStore entries and configures devices
accordingly.

In the second case we don't want QEMU to write/delete front-end
entries for two reasons: it might have no access to those entries if
it is running in un-privileged domain and it is just incorrect to
overwrite entries already provided by Xen toolstack, because toolstack
manages those nodes. For example, it might read backend- or frontend-
state to be sure that they are both disconnected and it is safe to
destroy a domain.

This patch checks presence of xendev->backend to check if Xen PV
device was configured by Xen toolstack to decide if it should touch
frontend entries in XenStore. Also, when we need to remove XenStore
entries during device teardown only if they weren't created by Xen
toolstack. If they were created by toolstack, then it is toolstack's
job to do proper clean-up.

Suggested-by: Paul Durrant <xadimgnik@gmail.com>
Suggested-by: David Woodhouse <dwmw@amazon.co.uk>
Co-Authored-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>

---

Changes in v3:

 - Rephrased the commit message
---
 hw/block/xen-block.c  | 16 +++++++++-------
 hw/char/xen_console.c |  2 +-
 hw/net/xen_nic.c      | 18 ++++++++++--------
 hw/xen/xen-bus.c      | 14 +++++++++-----
 4 files changed, 29 insertions(+), 21 deletions(-)

diff --git a/hw/block/xen-block.c b/hw/block/xen-block.c
index c2ac9db4a2..dac519a6d3 100644
--- a/hw/block/xen-block.c
+++ b/hw/block/xen-block.c
@@ -390,13 +390,15 @@ 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);
-
-    xen_device_backend_printf(xendev, "sector-size", "%u",
-                              conf->logical_block_size);
+    if (!xendev->backend) {
+        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);
+    }
 
     xen_block_set_size(blockdev);
 
diff --git a/hw/char/xen_console.c b/hw/char/xen_console.c
index bef8a3a621..b52ddddabf 100644
--- a/hw/char/xen_console.c
+++ b/hw/char/xen_console.c
@@ -450,7 +450,7 @@ static void xen_console_realize(XenDevice *xendev, Error **errp)
 
     trace_xen_console_realize(con->dev, object_get_typename(OBJECT(cs)));
 
-    if (CHARDEV_IS_PTY(cs)) {
+    if (CHARDEV_IS_PTY(cs) && !xendev->backend) {
         /* Strip the leading 'pty:' */
         xen_device_frontend_printf(xendev, "tty", "%s", cs->filename + 4);
     }
diff --git a/hw/net/xen_nic.c b/hw/net/xen_nic.c
index afa10c96e8..27442bef38 100644
--- a/hw/net/xen_nic.c
+++ b/hw/net/xen_nic.c
@@ -315,14 +315,16 @@ static void xen_netdev_realize(XenDevice *xendev, Error **errp)
 
     qemu_macaddr_default_if_unset(&netdev->conf.macaddr);
 
-    xen_device_frontend_printf(xendev, "mac", "%02x:%02x:%02x:%02x:%02x:%02x",
-                               netdev->conf.macaddr.a[0],
-                               netdev->conf.macaddr.a[1],
-                               netdev->conf.macaddr.a[2],
-                               netdev->conf.macaddr.a[3],
-                               netdev->conf.macaddr.a[4],
-                               netdev->conf.macaddr.a[5]);
-
+    if (!xendev->backend) {
+        xen_device_frontend_printf(xendev, "mac",
+                                   "%02x:%02x:%02x:%02x:%02x:%02x",
+                                   netdev->conf.macaddr.a[0],
+                                   netdev->conf.macaddr.a[1],
+                                   netdev->conf.macaddr.a[2],
+                                   netdev->conf.macaddr.a[3],
+                                   netdev->conf.macaddr.a[4],
+                                   netdev->conf.macaddr.a[5]);
+    }
     netdev->nic = qemu_new_nic(&net_xen_info, &netdev->conf,
                                object_get_typename(OBJECT(xendev)),
                                DEVICE(xendev)->id, netdev);
diff --git a/hw/xen/xen-bus.c b/hw/xen/xen-bus.c
index dd0171ab98..d0f17aeb27 100644
--- a/hw/xen/xen-bus.c
+++ b/hw/xen/xen-bus.c
@@ -599,8 +599,10 @@ 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 (!xendev->backend) {
+        xs_node_destroy(xenbus->xsh, XBT_NULL, xendev->backend_path,
+                        &local_err);
+    }
     g_free(xendev->backend_path);
     xendev->backend_path = NULL;
 
@@ -764,8 +766,10 @@ 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 (!xendev->backend) {
+        xs_node_destroy(xenbus->xsh, XBT_NULL, xendev->frontend_path,
+                        &local_err);
+    }
     g_free(xendev->frontend_path);
     xendev->frontend_path = NULL;
 
@@ -1063,7 +1067,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) && !xendev->backend) {
         xen_device_frontend_printf(xendev, "backend", "%s",
                                    xendev->backend_path);
         xen_device_frontend_printf(xendev, "backend-id", "%u",
-- 
2.42.0

[PATCH v3 1/5] hw/xen: Set XenBackendInstance in the XenDevice before realizing it

[Volodymyr Babchuk]

From: David Woodhouse <dwmw@amazon.co.uk>

This allows a XenDevice implementation to know whether it was created
by QEMU, or merely discovered in XenStore after the toolstack created
it. This will allow us to create frontend/backend nodes only when we
should, rather than unconditionally attempting to overwrite them from
a driver domain which doesn't have privileges to do so.

As an added benefit, it also means we no longer have to call the
xen_backend_set_device() function from the device models immediately
after calling qdev_realize_and_unref(). Even though we could make
the argument that it's safe to do so, and the pointer to the unreffed
device *will* actually still be valid, it still made my skin itch to
look at it.

Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
Reviewed-by: Paul Durrant <paul@xen.org>
---
 hw/block/xen-block.c         |  3 +--
 hw/char/xen_console.c        |  2 +-
 hw/net/xen_nic.c             |  2 +-
 hw/xen/xen-backend.c         | 15 +--------------
 hw/xen/xen-bus.c             |  4 ++++
 include/hw/xen/xen-backend.h |  2 --
 include/hw/xen/xen-bus.h     |  2 ++
 7 files changed, 10 insertions(+), 20 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-backend.c b/hw/xen/xen-backend.c
index b9bf70a9f5..b2e753ebc8 100644
--- a/hw/xen/xen-backend.c
+++ b/hw/xen/xen-backend.c
@@ -88,19 +88,6 @@ static void xen_backend_list_add(XenBackendInstance *backend)
     QLIST_INSERT_HEAD(&backend_list, backend, entry);
 }
 
-static XenBackendInstance *xen_backend_list_find(XenDevice *xendev)
-{
-    XenBackendInstance *backend;
-
-    QLIST_FOREACH(backend, &backend_list, entry) {
-        if (backend->xendev == xendev) {
-            return backend;
-        }
-    }
-
-    return NULL;
-}
-
 bool xen_backend_exists(const char *type, const char *name)
 {
     const XenBackendImpl *impl = xen_backend_table_lookup(type);
@@ -170,7 +157,7 @@ XenDevice *xen_backend_get_device(XenBackendInstance *backend)
 
 bool xen_backend_try_device_destroy(XenDevice *xendev, Error **errp)
 {
-    XenBackendInstance *backend = xen_backend_list_find(xendev);
+    XenBackendInstance *backend = xendev->backend;
     const XenBackendImpl *impl;
 
     if (!backend) {
diff --git a/hw/xen/xen-bus.c b/hw/xen/xen-bus.c
index 4973e7d9c9..dd0171ab98 100644
--- a/hw/xen/xen-bus.c
+++ b/hw/xen/xen-bus.c
@@ -1079,6 +1079,10 @@ static void xen_device_realize(DeviceState *dev, Error **errp)
         }
     }
 
+    if (xendev->backend) {
+        xen_backend_set_device(xendev->backend, xendev);
+    }
+
     xendev->exit.notify = xen_device_exit;
     qemu_add_exit_notifier(&xendev->exit);
     return;
diff --git a/include/hw/xen/xen-backend.h b/include/hw/xen/xen-backend.h
index 0f01631ae7..ea080ba7c9 100644
--- a/include/hw/xen/xen-backend.h
+++ b/include/hw/xen/xen-backend.h
@@ -10,8 +10,6 @@
 
 #include "hw/xen/xen-bus.h"
 
-typedef struct XenBackendInstance XenBackendInstance;
-
 typedef void (*XenBackendDeviceCreate)(XenBackendInstance *backend,
                                        QDict *opts, Error **errp);
 typedef void (*XenBackendDeviceDestroy)(XenBackendInstance *backend,
diff --git a/include/hw/xen/xen-bus.h b/include/hw/xen/xen-bus.h
index 334ddd1ff6..7647c4c38e 100644
--- a/include/hw/xen/xen-bus.h
+++ b/include/hw/xen/xen-bus.h
@@ -14,9 +14,11 @@
 #include "qom/object.h"
 
 typedef struct XenEventChannel XenEventChannel;
+typedef struct XenBackendInstance XenBackendInstance;
 
 struct XenDevice {
     DeviceState qdev;
+    XenBackendInstance *backend;
     domid_t frontend_id;
     char *name;
     struct qemu_xs_handle *xsh;
-- 
2.42.0

[RFC PATCH v3 3/5] xen: add option to disable legacy backends

[Volodymyr Babchuk]

This patch makes legacy backends optional. As was discussed at [1]
this is a solution to a problem when we can't run QEMU as a device
model in a non-privileged domain. This is because legacy backends
assume that they are always running in domain with ID = 0. Actually,
this may prevent running QEMU in a privileged domain with ID not equal
to zero.

To be able to disable legacy backends we need to alter couple of
source files that unintentionally depend on them. For example
xen-all.c used xen_pv_printf to report errors, while not providing any
additional like xendev pointer. Also, we need to move xenstore
structure from xen-legacy-backend.c, because it is apparently used in
xen-all.c.

With this patch it is possible to provide
"--disable-xen-legacy-backends" configure option to get QEMU binary
that can run in a driver domain. With price of not be able to use
legacy backends of course.

[1]
https://lists.gnu.org/archive/html/qemu-devel/2023-11/msg05022.html

Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>

---

I am not sure if I made correct changes to the build system, thus this
patch is tagged as RFC.

Changes in v3:
 - New patch in v3
---
 accel/xen/xen-all.c           | 13 ++++++++++---
 hw/9pfs/meson.build           |  4 +++-
 hw/display/meson.build        |  4 +++-
 hw/i386/pc.c                  |  2 ++
 hw/usb/meson.build            |  5 ++++-
 hw/xen/meson.build            | 11 ++++++++---
 hw/xen/xen-hvm-common.c       |  2 ++
 hw/xen/xen-legacy-backend.c   |  7 -------
 hw/xenpv/xen_machine_pv.c     |  2 ++
 meson.build                   |  5 +++++
 meson_options.txt             |  2 ++
 scripts/meson-buildoptions.sh |  4 ++++
 12 files changed, 45 insertions(+), 16 deletions(-)

diff --git a/accel/xen/xen-all.c b/accel/xen/xen-all.c
index 5ff0cb8bd9..188b29597f 100644
--- a/accel/xen/xen-all.c
+++ b/accel/xen/xen-all.c
@@ -29,6 +29,7 @@ bool xen_allowed;
 xc_interface *xen_xc;
 xenforeignmemory_handle *xen_fmem;
 xendevicemodel_handle *xen_dmod;
+struct qemu_xs_handle *xenstore;
 
 static void xenstore_record_dm_state(const char *state)
 {
@@ -78,20 +79,26 @@ static int xen_init(MachineState *ms)
 {
     MachineClass *mc = MACHINE_GET_CLASS(ms);
 
+    xenstore = qemu_xen_xs_open();
+    if (!xenstore) {
+        error_report("can't connect to xenstored\n");
+        exit(1);
+    }
+
     xen_xc = xc_interface_open(0, 0, 0);
     if (xen_xc == NULL) {
-        xen_pv_printf(NULL, 0, "can't open xen interface\n");
+        error_report("can't open xen interface\n");
         return -1;
     }
     xen_fmem = xenforeignmemory_open(0, 0);
     if (xen_fmem == NULL) {
-        xen_pv_printf(NULL, 0, "can't open xen fmem interface\n");
+        error_report("can't open xen fmem interface\n");
         xc_interface_close(xen_xc);
         return -1;
     }
     xen_dmod = xendevicemodel_open(0, 0);
     if (xen_dmod == NULL) {
-        xen_pv_printf(NULL, 0, "can't open xen devicemodel interface\n");
+        error_report("can't open xen devicemodel interface\n");
         xenforeignmemory_close(xen_fmem);
         xc_interface_close(xen_xc);
         return -1;
diff --git a/hw/9pfs/meson.build b/hw/9pfs/meson.build
index 2944ea63c3..e8306ba8d2 100644
--- a/hw/9pfs/meson.build
+++ b/hw/9pfs/meson.build
@@ -15,7 +15,9 @@ fs_ss.add(files(
 ))
 fs_ss.add(when: 'CONFIG_LINUX', if_true: files('9p-util-linux.c'))
 fs_ss.add(when: 'CONFIG_DARWIN', if_true: files('9p-util-darwin.c'))
-fs_ss.add(when: 'CONFIG_XEN_BUS', if_true: files('xen-9p-backend.c'))
+if have_xen_legacy_backends
+  fs_ss.add(when: 'CONFIG_XEN_BUS', if_true: files('xen-9p-backend.c'))
+endif
 system_ss.add_all(when: 'CONFIG_FSDEV_9P', if_true: fs_ss)
 
 specific_ss.add(when: 'CONFIG_VIRTIO_9P', if_true: files('virtio-9p-device.c'))
diff --git a/hw/display/meson.build b/hw/display/meson.build
index 344dfe3d8c..18d657f6b3 100644
--- a/hw/display/meson.build
+++ b/hw/display/meson.build
@@ -14,7 +14,9 @@ system_ss.add(when: 'CONFIG_PL110', if_true: files('pl110.c'))
 system_ss.add(when: 'CONFIG_SII9022', if_true: files('sii9022.c'))
 system_ss.add(when: 'CONFIG_SSD0303', if_true: files('ssd0303.c'))
 system_ss.add(when: 'CONFIG_SSD0323', if_true: files('ssd0323.c'))
-system_ss.add(when: 'CONFIG_XEN_BUS', if_true: files('xenfb.c'))
+if have_xen_legacy_backends
+  system_ss.add(when: 'CONFIG_XEN_BUS', if_true: files('xenfb.c'))
+endif
 
 system_ss.add(when: 'CONFIG_VGA_PCI', if_true: files('vga-pci.c'))
 system_ss.add(when: 'CONFIG_VGA_ISA', if_true: files('vga-isa.c'))
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 29b9964733..91857af428 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1263,7 +1263,9 @@ void pc_basic_device_init(struct PCMachineState *pcms,
             pci_create_simple(pcms->bus, -1, "xen-platform");
         }
         pcms->xenbus = xen_bus_init();
+#ifdef CONFIG_XEN_LEGACY_BACKENDS
         xen_be_init();
+#endif
     }
 #endif
 
diff --git a/hw/usb/meson.build b/hw/usb/meson.build
index e94149ebde..8d395745b2 100644
--- a/hw/usb/meson.build
+++ b/hw/usb/meson.build
@@ -84,6 +84,9 @@ if libusb.found()
   hw_usb_modules += {'host': usbhost_ss}
 endif
 
-system_ss.add(when: ['CONFIG_USB', 'CONFIG_XEN_BUS', libusb], if_true: files('xen-usb.c'))
+if have_xen_legacy_backends
+  system_ss.add(when: ['CONFIG_USB', 'CONFIG_XEN_BUS', libusb],
+                if_true: files('xen-usb.c'))
+endif
 
 modules += { 'hw-usb': hw_usb_modules }
diff --git a/hw/xen/meson.build b/hw/xen/meson.build
index d887fa9ba4..964c3364f2 100644
--- a/hw/xen/meson.build
+++ b/hw/xen/meson.build
@@ -2,11 +2,16 @@ system_ss.add(when: ['CONFIG_XEN_BUS'], if_true: files(
   'xen-backend.c',
   'xen-bus-helper.c',
   'xen-bus.c',
-  'xen-legacy-backend.c',
-  'xen_devconfig.c',
-  'xen_pvdev.c',
 ))
 
+if have_xen_legacy_backends
+  system_ss.add(when: ['CONFIG_XEN_BUS'], if_true: files(
+    'xen_pvdev.c',
+    'xen-legacy-backend.c',
+    'xen_devconfig.c',
+  ))
+endif
+
 system_ss.add(when: ['CONFIG_XEN', xen], if_true: files(
   'xen-operations.c',
 ))
diff --git a/hw/xen/xen-hvm-common.c b/hw/xen/xen-hvm-common.c
index 565dc39c8f..2e7897dbd2 100644
--- a/hw/xen/xen-hvm-common.c
+++ b/hw/xen/xen-hvm-common.c
@@ -869,7 +869,9 @@ void xen_register_ioreq(XenIOState *state, unsigned int max_cpus,
 
     xen_bus_init();
 
+#ifdef CONFIG_XEN_LEGACY_BACKENDS
     xen_be_init();
+#endif
 
     return;
 
diff --git a/hw/xen/xen-legacy-backend.c b/hw/xen/xen-legacy-backend.c
index 124dd5f3d6..717d5efc06 100644
--- a/hw/xen/xen-legacy-backend.c
+++ b/hw/xen/xen-legacy-backend.c
@@ -39,7 +39,6 @@ BusState *xen_sysbus;
 /* ------------------------------------------------------------- */
 
 /* public */
-struct qemu_xs_handle *xenstore;
 const char *xen_protocol;
 
 /* private */
@@ -605,12 +604,6 @@ static void xen_set_dynamic_sysbus(void)
 
 void xen_be_init(void)
 {
-    xenstore = qemu_xen_xs_open();
-    if (!xenstore) {
-        xen_pv_printf(NULL, 0, "can't connect to xenstored\n");
-        exit(1);
-    }
-
     if (xen_evtchn_ops == NULL || xen_gnttab_ops == NULL) {
         xen_pv_printf(NULL, 0, "Xen operations not set up\n");
         exit(1);
diff --git a/hw/xenpv/xen_machine_pv.c b/hw/xenpv/xen_machine_pv.c
index 9f9f137f99..03a55f345c 100644
--- a/hw/xenpv/xen_machine_pv.c
+++ b/hw/xenpv/xen_machine_pv.c
@@ -37,7 +37,9 @@ static void xen_init_pv(MachineState *machine)
     setup_xen_backend_ops();
 
     /* Initialize backend core & drivers */
+#ifdef CONFIG_XEN_LEGACY_BACKENDS
     xen_be_init();
+#endif
 
     switch (xen_mode) {
     case XEN_ATTACH:
diff --git a/meson.build b/meson.build
index ec01f8b138..c8a43dd97d 100644
--- a/meson.build
+++ b/meson.build
@@ -1749,6 +1749,9 @@ have_xen_pci_passthrough = get_option('xen_pci_passthrough') \
            error_message: 'Xen PCI passthrough not available on this platform') \
   .allowed()
 
+have_xen_legacy_backends = get_option('xen-legacy-backends').require(xen.found(),
+           error_message: 'Xen legacy backends requested but Xen not enabled').allowed()
+
 
 cacard = not_found
 if not get_option('smartcard').auto() or have_system
@@ -2219,6 +2222,7 @@ config_host_data.set('CONFIG_DBUS_DISPLAY', dbus_display)
 config_host_data.set('CONFIG_CFI', get_option('cfi'))
 config_host_data.set('CONFIG_SELINUX', selinux.found())
 config_host_data.set('CONFIG_XEN_BACKEND', xen.found())
+config_host_data.set('CONFIG_XEN_LEGACY_BACKENDS', have_xen_legacy_backends)
 config_host_data.set('CONFIG_LIBDW', libdw.found())
 if xen.found()
   # protect from xen.version() having less than three components
@@ -3049,6 +3053,7 @@ config_all += config_targetos
 config_all += config_all_disas
 config_all += {
   'CONFIG_XEN': xen.found(),
+  'CONFIG_XEN_LEGACY_BACKENDS': have_xen_legacy_backends,
   'CONFIG_SYSTEM_ONLY': have_system,
   'CONFIG_USER_ONLY': have_user,
   'CONFIG_ALL': true,
diff --git a/meson_options.txt b/meson_options.txt
index c9baeda639..91dd677257 100644
--- a/meson_options.txt
+++ b/meson_options.txt
@@ -77,6 +77,8 @@ option('nvmm', type: 'feature', value: 'auto',
        description: 'NVMM acceleration support')
 option('xen', type: 'feature', value: 'auto',
        description: 'Xen backend support')
+option('xen-legacy-backends', type: 'feature', value: 'auto',
+       description: 'Xen legacy backends (9pfs, fb, qusb) support')
 option('xen_pci_passthrough', type: 'feature', value: 'auto',
        description: 'Xen PCI passthrough support')
 option('tcg', type: 'feature', value: 'enabled',
diff --git a/scripts/meson-buildoptions.sh b/scripts/meson-buildoptions.sh
index 680fa3f581..b5acef008f 100644
--- a/scripts/meson-buildoptions.sh
+++ b/scripts/meson-buildoptions.sh
@@ -218,6 +218,8 @@ meson_options_help() {
   printf "%s\n" '  werror          Treat warnings as errors'
   printf "%s\n" '  whpx            WHPX acceleration support'
   printf "%s\n" '  xen             Xen backend support'
+  printf "%s\n" '  xen-legacy-backends'
+  printf "%s\n" '                  Xen legacy backends (9pfs, fb, qusb) support'
   printf "%s\n" '  xen-pci-passthrough'
   printf "%s\n" '                  Xen PCI passthrough support'
   printf "%s\n" '  xkbcommon       xkbcommon support'
@@ -556,6 +558,8 @@ _meson_option_parse() {
     --disable-whpx) printf "%s" -Dwhpx=disabled ;;
     --enable-xen) printf "%s" -Dxen=enabled ;;
     --disable-xen) printf "%s" -Dxen=disabled ;;
+    --enable-xen-legacy-backends) printf "%s" -Dxen-legacy-backends=enabled ;;
+    --disable-xen-legacy-backends) printf "%s" -Dxen-legacy-backends=disabled ;;
     --enable-xen-pci-passthrough) printf "%s" -Dxen_pci_passthrough=enabled ;;
     --disable-xen-pci-passthrough) printf "%s" -Dxen_pci_passthrough=disabled ;;
     --enable-xkbcommon) printf "%s" -Dxkbcommon=enabled ;;
-- 
2.42.0

[PATCH v3 5/5] xen_arm: Add virtual PCIe host bridge support

[Volodymyr Babchuk]

From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>

The bridge is needed for virtio-pci support, as QEMU can emulate the
whole bridge with any virtio-pci devices connected to it.

This patch provides a flexible way to configure PCIe bridge resources
using QEMU machine properties. 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>

---

Changes in v3:

 - Use QOM properties instead of reading from XenStore
 - Remove unneeded includes
 - Move pcie_* fields into "struct cfg"

Changes in v2:

 - Renamed virtio_pci_host to pcie_host entries in XenStore, because
 there is nothing specific to virtio-pci: any PCI device can be
 emulated via this newly created bridge.
---
 hw/arm/xen_arm.c            | 226 ++++++++++++++++++++++++++++++++++++
 hw/xen/xen-hvm-common.c     |   9 +-
 include/hw/xen/xen_native.h |   8 +-
 3 files changed, 240 insertions(+), 3 deletions(-)

diff --git a/hw/arm/xen_arm.c b/hw/arm/xen_arm.c
index b9c3ae14b6..dc6d3a1d82 100644
--- a/hw/arm/xen_arm.c
+++ b/hw/arm/xen_arm.c
@@ -34,6 +34,7 @@
 #include "hw/xen/xen-hvm-common.h"
 #include "sysemu/tpm.h"
 #include "hw/xen/arch_hvm.h"
+#include "hw/pci-host/gpex.h"
 
 #define TYPE_XEN_ARM  MACHINE_TYPE_NAME("xenpvh")
 OBJECT_DECLARE_SIMPLE_TYPE(XenArmState, XEN_ARM)
@@ -57,6 +58,10 @@ struct XenArmState {
 
     struct {
         uint64_t tpm_base_addr;
+        MemMapEntry pcie_mmio;
+        MemMapEntry pcie_ecam;
+        MemMapEntry pcie_mmio_high;
+        int         pcie_irq_base;
     } cfg;
 };
 
@@ -73,6 +78,15 @@ static MemoryRegion ram_lo, ram_hi;
 #define NR_VIRTIO_MMIO_DEVICES   \
    (GUEST_VIRTIO_MMIO_SPI_LAST - GUEST_VIRTIO_MMIO_SPI_FIRST)
 
+#define XEN_ARM_PCIE_ECAM_BASE          "pcie-ecam-base"
+#define XEN_ARM_PCIE_ECAM_SIZE          "pcie-ecam-size"
+#define XEN_ARM_PCIE_MEM_BASE           "pcie-mem-base"
+#define XEN_ARM_PCIE_MEM_SIZE           "pcie-mem-size"
+#define XEN_ARM_PCIE_PREFETCH_BASE      "pcie-prefetch-mem-base"
+#define XEN_ARM_PCIE_PREFETCH_SIZE      "pcie-prefetch-mem-size"
+#define XEN_ARM_PCIE_IRQ_BASE           "pcie-irq-base"
+
+/* TODO It should be xendevicemodel_set_pci_intx_level() for PCI interrupts. */
 static void xen_set_irq(void *opaque, int irq, int level)
 {
     if (xendevicemodel_set_irq_level(xen_dmod, xen_domid, irq, level)) {
@@ -129,6 +143,89 @@ static void xen_init_ram(MachineState *machine)
     }
 }
 
+static bool xen_validate_pcie_config(XenArmState *xam)
+{
+    if (xam->cfg.pcie_ecam.base == 0 &&
+        xam->cfg.pcie_ecam.size == 0 &&
+        xam->cfg.pcie_mmio.base == 0 &&
+        xam->cfg.pcie_mmio.size == 0 &&
+        xam->cfg.pcie_mmio_high.base == 0 &&
+        xam->cfg.pcie_mmio_high.size == 0 &&
+        xam->cfg.pcie_irq_base == 0) {
+
+        /* It's okay, user just don't want PCIe brige */
+
+        return false;
+    }
+
+    if (xam->cfg.pcie_ecam.base == 0 ||
+        xam->cfg.pcie_ecam.size == 0 ||
+        xam->cfg.pcie_mmio.base == 0 ||
+        xam->cfg.pcie_mmio.size == 0 ||
+        xam->cfg.pcie_mmio_high.base == 0 ||
+        xam->cfg.pcie_mmio_high.size == 0 ||
+        xam->cfg.pcie_irq_base == 0) {
+
+        /* User provided some PCIe options, but not all of them */
+
+        error_printf("Incomplete PCIe bridge configuration\n");
+
+        exit(1);
+    }
+
+    return true;
+}
+
+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->cfg.pcie_ecam.size);
+    memory_region_add_subregion(get_system_memory(), xam->cfg.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->cfg.pcie_mmio.base,
+                             xam->cfg.pcie_mmio.size);
+    memory_region_add_subregion(get_system_memory(), xam->cfg.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->cfg.pcie_mmio_high.base,
+                             xam->cfg.pcie_mmio_high.size);
+    memory_region_add_subregion(get_system_memory(),
+                                xam->cfg.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->cfg.pcie_irq_base + i);
+
+        sysbus_connect_irq(SYS_BUS_DEVICE(dev), i, irq);
+        gpex_set_irq_num(GPEX_HOST(dev), i, xam->cfg.pcie_irq_base + i);
+    }
+
+    DPRINTF("Created PCIe host bridge\n");
+}
+
 void arch_handle_ioreq(XenIOState *state, ioreq_t *req)
 {
     hw_error("Invalid ioreq type 0x%x\n", req->type);
@@ -189,6 +286,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_validate_pcie_config(xam)) {
+        xen_create_pcie(xam);
+    } else {
+        DPRINTF("PCIe host bridge is not configured,"
+                " only virtio-mmio can be used\n");
+    }
 
 #ifdef CONFIG_TPM
     if (xam->cfg.tpm_base_addr) {
@@ -225,6 +328,90 @@ static void xen_arm_set_tpm_base_addr(Object *obj, Visitor *v,
 }
 #endif
 
+static void xen_arm_get_pcie_prop(Object *obj, Visitor *v,
+                                  const char *name, void *opaque,
+                                  Error **errp)
+{
+    XenArmState *xam = XEN_ARM(obj);
+    hwaddr *target;
+
+    if (!strcmp(name, XEN_ARM_PCIE_ECAM_BASE)) {
+        target = &xam->cfg.pcie_ecam.base;
+    } else if (!strcmp(name, XEN_ARM_PCIE_ECAM_SIZE)) {
+        target = &xam->cfg.pcie_ecam.size;
+    } else if (!strcmp(name, XEN_ARM_PCIE_MEM_BASE)) {
+        target = &xam->cfg.pcie_mmio.base;
+    } else if (!strcmp(name, XEN_ARM_PCIE_MEM_SIZE)) {
+        target = &xam->cfg.pcie_mmio.size;
+    } else if (!strcmp(name, XEN_ARM_PCIE_PREFETCH_BASE)) {
+        target = &xam->cfg.pcie_mmio_high.base;
+    } else if (!strcmp(name, XEN_ARM_PCIE_PREFETCH_SIZE)) {
+        target = &xam->cfg.pcie_mmio_high.size;
+    } else {
+        /* Unreachable */
+        assert(false);
+        return;
+    }
+
+    visit_type_uint64(v, name, target, errp);
+}
+
+static void xen_arm_set_pcie_prop(Object *obj, Visitor *v,
+                                  const char *name, void *opaque,
+                                  Error **errp)
+{
+    XenArmState *xam = XEN_ARM(obj);
+    uint64_t value;
+    hwaddr *target;
+
+    if (!strcmp(name, XEN_ARM_PCIE_ECAM_BASE)) {
+        target = &xam->cfg.pcie_ecam.base;
+    } else if (!strcmp(name, XEN_ARM_PCIE_ECAM_SIZE)) {
+        target = &xam->cfg.pcie_ecam.size;
+    } else if (!strcmp(name, XEN_ARM_PCIE_MEM_BASE)) {
+        target = &xam->cfg.pcie_mmio.base;
+    } else if (!strcmp(name, XEN_ARM_PCIE_MEM_SIZE)) {
+        target = &xam->cfg.pcie_mmio.size;
+    } else if (!strcmp(name, XEN_ARM_PCIE_PREFETCH_BASE)) {
+        target = &xam->cfg.pcie_mmio_high.base;
+    } else if (!strcmp(name, XEN_ARM_PCIE_PREFETCH_SIZE)) {
+        target = &xam->cfg.pcie_mmio_high.size;
+    } else {
+        /* Unreachable */
+        assert(false);
+        return;
+    }
+
+    if (!visit_type_uint64(v, name, &value, errp)) {
+        return;
+    }
+    *target = value;
+}
+
+static void xen_arm_get_pcie_irq_base(Object *obj, Visitor *v,
+                                      const char *name, void *opaque,
+                                      Error **errp)
+{
+    XenArmState *xam = XEN_ARM(obj);
+    int64_t value = xam->cfg.pcie_irq_base;
+
+    visit_type_int(v, name, &value, errp);
+}
+
+static void xen_arm_set_pcie_irq_base(Object *obj, Visitor *v,
+                                      const char *name, void *opaque,
+                                      Error **errp)
+{
+    XenArmState *xam = XEN_ARM(obj);
+    int64_t value;
+
+    if (!visit_type_int(v, name, &value, errp)) {
+        return;
+    }
+
+    xam->cfg.pcie_irq_base = value;
+}
+
 static void xen_arm_machine_class_init(ObjectClass *oc, void *data)
 {
 
@@ -246,6 +433,45 @@ static void xen_arm_machine_class_init(ObjectClass *oc, void *data)
 
     machine_class_allow_dynamic_sysbus_dev(mc, TYPE_TPM_TIS_SYSBUS);
 #endif
+
+    object_class_property_add(oc, XEN_ARM_PCIE_ECAM_BASE, "uint64_t",
+                              xen_arm_get_pcie_prop,
+                              xen_arm_set_pcie_prop, NULL, NULL);
+    object_class_property_set_description(oc, XEN_ARM_PCIE_ECAM_BASE,
+        "Base address for ECAM range of virtual PCIe host bridge");
+    object_class_property_add(oc, XEN_ARM_PCIE_ECAM_SIZE, "uint64_t",
+                              xen_arm_get_pcie_prop,
+                              xen_arm_set_pcie_prop, NULL, NULL);
+    object_class_property_set_description(oc, XEN_ARM_PCIE_ECAM_SIZE,
+        "Size of ECAM range of virtual PCIe host bridge");
+
+    object_class_property_add(oc, XEN_ARM_PCIE_MEM_BASE, "uint64_t",
+                              xen_arm_get_pcie_prop,
+                              xen_arm_set_pcie_prop, NULL, NULL);
+    object_class_property_set_description(oc, XEN_ARM_PCIE_MEM_BASE,
+        "Base address for non-prefetchable memory of virtual PCIe host bridge");
+    object_class_property_add(oc, XEN_ARM_PCIE_MEM_SIZE, "uint64_t",
+                              xen_arm_get_pcie_prop,
+                              xen_arm_set_pcie_prop, NULL, NULL);
+    object_class_property_set_description(oc, XEN_ARM_PCIE_MEM_SIZE,
+        "Size of non-prefetchable memory of virtual PCIe host bridge");
+
+    object_class_property_add(oc, XEN_ARM_PCIE_PREFETCH_BASE, "uint64_t",
+                              xen_arm_get_pcie_prop,
+                              xen_arm_set_pcie_prop, NULL, NULL);
+    object_class_property_set_description(oc, XEN_ARM_PCIE_PREFETCH_BASE,
+        "Base address for prefetchable memory of virtual PCIe host bridge");
+    object_class_property_add(oc, XEN_ARM_PCIE_PREFETCH_SIZE, "uint64_t",
+                              xen_arm_get_pcie_prop,
+                              xen_arm_set_pcie_prop, NULL, NULL);
+    object_class_property_set_description(oc, XEN_ARM_PCIE_PREFETCH_SIZE,
+        "Size of prefetchable memory of virtual PCIe host bridge");
+
+    object_class_property_add(oc, XEN_ARM_PCIE_IRQ_BASE, "int",
+                              xen_arm_get_pcie_irq_base,
+                              xen_arm_set_pcie_irq_base, NULL, NULL);
+    object_class_property_set_description(oc, XEN_ARM_PCIE_IRQ_BASE,
+        "Number of first PCI legacy interrupt for PCIe host bridge");
 }
 
 static const TypeInfo xen_arm_machine_type = {
diff --git a/hw/xen/xen-hvm-common.c b/hw/xen/xen-hvm-common.c
index 2e7897dbd2..19fcccdb16 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 6f09c48823..2b1debaff4 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

[PATCH v3 4/5] xen_arm: set mc->max_cpus to GUEST_MAX_VCPUS

[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. So when the toolstack pass
max_vcpus using "-smp" arg, machine creation will not fail.

Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 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 a5631529d0..b9c3ae14b6 100644
--- a/hw/arm/xen_arm.c
+++ b/hw/arm/xen_arm.c
@@ -231,7 +231,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

Re: [EXTERNAL] [PATCH v3 2/5] xen: backends: don't overwrite XenStore nodes created by toolstack

[David Woodhouse]

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

On Fri, 2023-11-24 at 23:24 +0000, Volodymyr Babchuk wrote:
> Xen PV devices in QEMU can be created in two ways: either by QEMU
> itself, if they were passed via command line, or by Xen toolstack. In
> the latter case, QEMU scans XenStore entries and configures devices
> accordingly.
> 
> In the second case we don't want QEMU to write/delete front-end
> entries for two reasons: it might have no access to those entries if
> it is running in un-privileged domain and it is just incorrect to
> overwrite entries already provided by Xen toolstack, because
> toolstack
> manages those nodes. For example, it might read backend- or frontend-
> state to be sure that they are both disconnected and it is safe to
> destroy a domain.
> 
> This patch checks presence of xendev->backend to check if Xen PV
> device was configured by Xen toolstack to decide if it should touch
> frontend entries in XenStore. Also, when we need to remove XenStore
> entries during device teardown only if they weren't created by Xen
> toolstack. If they were created by toolstack, then it is toolstack's
> job to do proper clean-up.
> 
> Suggested-by: Paul Durrant <xadimgnik@gmail.com>
> Suggested-by: David Woodhouse <dwmw@amazon.co.uk>
> Co-Authored-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>

Reviewed-by: David Woodhouse <dwmw@amazon.co.uk>

... albeit with a couple of suggestions... 

> diff --git a/hw/char/xen_console.c b/hw/char/xen_console.c
> index bef8a3a621..b52ddddabf 100644
> --- a/hw/char/xen_console.c
> +++ b/hw/char/xen_console.c
> @@ -450,7 +450,7 @@ static void xen_console_realize(XenDevice *xendev, Error **errp)
> 
>      trace_xen_console_realize(con->dev, object_get_typename(OBJECT(cs)));
> 
> -    if (CHARDEV_IS_PTY(cs)) {
> +    if (CHARDEV_IS_PTY(cs) && !xendev->backend) {
>          /* Strip the leading 'pty:' */
>          xen_device_frontend_printf(xendev, "tty", "%s", cs->filename + 4);
>      }


It's kind of weird that that one is a frontend node at all; surely it
should have been a backend node? But it is known only to QEMU once it
actually opens /dev/ptmx and creates a new pty. It can't be populated
by the toolstack in advance.

So shouldn't the toolstack have made it writable by the driver domain? 

I think we should attempt to write this and just gracefully handle the
failure if we can't. (In fact, xen_device_frontend_printf() will just
use error_report_err() which is probably OK unless you feel strongly
about silencing it).

> diff --git a/hw/net/xen_nic.c b/hw/net/xen_nic.c
> index afa10c96e8..27442bef38 100644
> --- a/hw/net/xen_nic.c
> +++ b/hw/net/xen_nic.c
> @@ -315,14 +315,16 @@ static void xen_netdev_realize(XenDevice *xendev, Error **errp)
> 
>      qemu_macaddr_default_if_unset(&netdev->conf.macaddr);
> 
> -    xen_device_frontend_printf(xendev, "mac", "%02x:%02x:%02x:%02x:%02x:%02x",
> -                               netdev->conf.macaddr.a[0],
> -                               netdev->conf.macaddr.a[1],
> -                               netdev->conf.macaddr.a[2],
> -                               netdev->conf.macaddr.a[3],
> -                               netdev->conf.macaddr.a[4],
> -                               netdev->conf.macaddr.a[5]);
> -
> +    if (!xendev->backend) {
> +        xen_device_frontend_printf(xendev, "mac",
> +                                   "%02x:%02x:%02x:%02x:%02x:%02x",
> +                                   netdev->conf.macaddr.a[0],
> +                                   netdev->conf.macaddr.a[1],
> +                                   netdev->conf.macaddr.a[2],
> +                                   netdev->conf.macaddr.a[3],
> +                                   netdev->conf.macaddr.a[4],
> +                                   netdev->conf.macaddr.a[5]);
> +    }
>      netdev->nic = qemu_new_nic(&net_xen_info, &netdev->conf,
>                                 object_get_typename(OBJECT(xendev)),
>                                 DEVICE(xendev)->id, netdev);


Perhaps here you should create the "mac" node if it doesn't exist (or
is that "if it doesn't match netdev->conf.macaddr"?) and just
gracefully accept failure too?




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

Re: [RFC PATCH v3 3/5] xen: add option to disable legacy backends

[Woodhouse, David]

[-- Attachment #1.1: Type: text/plain, Size: 2471 bytes --]

On Fri, 2023-11-24 at 23:24 +0000, Volodymyr Babchuk wrote:
> This patch makes legacy backends optional. As was discussed at [1]
> this is a solution to a problem when we can't run QEMU as a device
> model in a non-privileged domain. This is because legacy backends
> assume that they are always running in domain with ID = 0. Actually,
> this may prevent running QEMU in a privileged domain with ID not equal
> to zero.
> 
> To be able to disable legacy backends we need to alter couple of
> source files that unintentionally depend on them. For example
> xen-all.c used xen_pv_printf to report errors, while not providing any
> additional like xendev pointer. Also, we need to move xenstore
> structure from xen-legacy-backend.c, because it is apparently used in
> xen-all.c.
> 
> With this patch it is possible to provide
> "--disable-xen-legacy-backends" configure option to get QEMU binary
> that can run in a driver domain. With price of not be able to use
> legacy backends of course.
> 
> [1]
> https://lists.gnu.org/archive/html/qemu-devel/2023-11/msg05022.html
> 
> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
> 
> ---
> 
> I am not sure if I made correct changes to the build system, thus this
> patch is tagged as RFC.

Hm, I was imagining a new CONFIG_LEGACY_XEN_BACKENDS option which would
look a lot like CONFIG_XEN_BUS (which would now be only for the new
XenBus code).

This looks weird to me:

> --- a/hw/display/meson.build
> +++ b/hw/display/meson.build
> @@ -14,7 +14,9 @@ system_ss.add(when: 'CONFIG_PL110', if_true:
> files('pl110.c'))
>  system_ss.add(when: 'CONFIG_SII9022', if_true: files('sii9022.c'))
>  system_ss.add(when: 'CONFIG_SSD0303', if_true: files('ssd0303.c'))
>  system_ss.add(when: 'CONFIG_SSD0323', if_true: files('ssd0323.c'))
> -system_ss.add(when: 'CONFIG_XEN_BUS', if_true: files('xenfb.c'))
> +if have_xen_legacy_backends
> +  system_ss.add(when: 'CONFIG_XEN_BUS', if_true: files('xenfb.c'))
> +endif
> 
>  system_ss.add(when: 'CONFIG_VGA_PCI', if_true: files('vga-pci.c'))
>  system_ss.add(when: 'CONFIG_VGA_ISA', if_true: files('vga-isa.c'))

I'd prefer to see just:

-system_ss.add(when: 'CONFIG_XEN_BUS', if_true: files('xenfb.c'))
+system_ss.add(when: 'CONFIG_XEN_LEGACY_BACKENDS', if_true: files('xenfb.c'))


Probably also better to split out the bits in accel/xen/xen-all.c and
hw/xen/xen-legacy-backend.c to a separate preparatory commit.

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

[-- Attachment #2.1: Type: text/plain, Size: 215 bytes --]




Amazon Development Centre (London) Ltd. Registered in England and Wales with registration number 04543232 with its registered office at 1 Principal Place, Worship Street, London EC2A 2FA, United Kingdom.



[-- Attachment #2.2: Type: text/html, Size: 228 bytes --]

Re: [RFC PATCH v3 3/5] xen: add option to disable legacy backends

[Woodhouse, David via]

[-- Attachment #1.1: Type: text/plain, Size: 2471 bytes --]

On Fri, 2023-11-24 at 23:24 +0000, Volodymyr Babchuk wrote:
> This patch makes legacy backends optional. As was discussed at [1]
> this is a solution to a problem when we can't run QEMU as a device
> model in a non-privileged domain. This is because legacy backends
> assume that they are always running in domain with ID = 0. Actually,
> this may prevent running QEMU in a privileged domain with ID not equal
> to zero.
> 
> To be able to disable legacy backends we need to alter couple of
> source files that unintentionally depend on them. For example
> xen-all.c used xen_pv_printf to report errors, while not providing any
> additional like xendev pointer. Also, we need to move xenstore
> structure from xen-legacy-backend.c, because it is apparently used in
> xen-all.c.
> 
> With this patch it is possible to provide
> "--disable-xen-legacy-backends" configure option to get QEMU binary
> that can run in a driver domain. With price of not be able to use
> legacy backends of course.
> 
> [1]
> https://lists.gnu.org/archive/html/qemu-devel/2023-11/msg05022.html
> 
> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
> 
> ---
> 
> I am not sure if I made correct changes to the build system, thus this
> patch is tagged as RFC.

Hm, I was imagining a new CONFIG_LEGACY_XEN_BACKENDS option which would
look a lot like CONFIG_XEN_BUS (which would now be only for the new
XenBus code).

This looks weird to me:

> --- a/hw/display/meson.build
> +++ b/hw/display/meson.build
> @@ -14,7 +14,9 @@ system_ss.add(when: 'CONFIG_PL110', if_true:
> files('pl110.c'))
>  system_ss.add(when: 'CONFIG_SII9022', if_true: files('sii9022.c'))
>  system_ss.add(when: 'CONFIG_SSD0303', if_true: files('ssd0303.c'))
>  system_ss.add(when: 'CONFIG_SSD0323', if_true: files('ssd0323.c'))
> -system_ss.add(when: 'CONFIG_XEN_BUS', if_true: files('xenfb.c'))
> +if have_xen_legacy_backends
> +  system_ss.add(when: 'CONFIG_XEN_BUS', if_true: files('xenfb.c'))
> +endif
> 
>  system_ss.add(when: 'CONFIG_VGA_PCI', if_true: files('vga-pci.c'))
>  system_ss.add(when: 'CONFIG_VGA_ISA', if_true: files('vga-isa.c'))

I'd prefer to see just:

-system_ss.add(when: 'CONFIG_XEN_BUS', if_true: files('xenfb.c'))
+system_ss.add(when: 'CONFIG_XEN_LEGACY_BACKENDS', if_true: files('xenfb.c'))


Probably also better to split out the bits in accel/xen/xen-all.c and
hw/xen/xen-legacy-backend.c to a separate preparatory commit.

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

[-- Attachment #2.1: Type: text/plain, Size: 215 bytes --]




Amazon Development Centre (London) Ltd. Registered in England and Wales with registration number 04543232 with its registered office at 1 Principal Place, Worship Street, London EC2A 2FA, United Kingdom.



[-- Attachment #2.2: Type: text/html, Size: 228 bytes --]

Re: [RFC PATCH v3 3/5] xen: add option to disable legacy backends

[Volodymyr Babchuk]

Hi David,

"Woodhouse, David" <dwmw@amazon.co.uk> writes:

> [[S/MIME Signed Part:Undecided]]
> On Fri, 2023-11-24 at 23:24 +0000, Volodymyr Babchuk wrote:
>> This patch makes legacy backends optional. As was discussed at [1]
>> this is a solution to a problem when we can't run QEMU as a device
>> model in a non-privileged domain. This is because legacy backends
>> assume that they are always running in domain with ID = 0. Actually,
>> this may prevent running QEMU in a privileged domain with ID not equal
>> to zero.
>> 
>> To be able to disable legacy backends we need to alter couple of
>> source files that unintentionally depend on them. For example
>> xen-all.c used xen_pv_printf to report errors, while not providing any
>> additional like xendev pointer. Also, we need to move xenstore
>> structure from xen-legacy-backend.c, because it is apparently used in
>> xen-all.c.
>> 
>> With this patch it is possible to provide
>> "--disable-xen-legacy-backends" configure option to get QEMU binary
>> that can run in a driver domain. With price of not be able to use
>> legacy backends of course.
>> 
>> [1]
>> https://lists.gnu.org/archive/html/qemu-devel/2023-11/msg05022.html
>> 
>> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
>> 
>> ---
>> 
>> I am not sure if I made correct changes to the build system, thus this
>> patch is tagged as RFC.
>
> Hm, I was imagining a new CONFIG_LEGACY_XEN_BACKENDS option which would
> look a lot like CONFIG_XEN_BUS (which would now be only for the new
> XenBus code).
>

It was my original intention too. But it appears that it is not possible
to add Kconfig value and then make it configurable via ./config
script. As I understood it can be set only via defconfig file.

> This looks weird to me:
>
>> --- a/hw/display/meson.build
>> +++ b/hw/display/meson.build
>> @@ -14,7 +14,9 @@ system_ss.add(when: 'CONFIG_PL110', if_true:
>> files('pl110.c'))
>>  system_ss.add(when: 'CONFIG_SII9022', if_true: files('sii9022.c'))
>>  system_ss.add(when: 'CONFIG_SSD0303', if_true: files('ssd0303.c'))
>>  system_ss.add(when: 'CONFIG_SSD0323', if_true: files('ssd0323.c'))
>> -system_ss.add(when: 'CONFIG_XEN_BUS', if_true: files('xenfb.c'))
>> +if have_xen_legacy_backends
>> +  system_ss.add(when: 'CONFIG_XEN_BUS', if_true: files('xenfb.c'))
>> +endif
>> 
>>  system_ss.add(when: 'CONFIG_VGA_PCI', if_true: files('vga-pci.c'))
>>  system_ss.add(when: 'CONFIG_VGA_ISA', if_true: files('vga-isa.c'))
>
> I'd prefer to see just:
>
> -system_ss.add(when: 'CONFIG_XEN_BUS', if_true: files('xenfb.c'))
> +system_ss.add(when: 'CONFIG_XEN_LEGACY_BACKENDS', if_true: files('xenfb.c'))

I tried, but it does not work this way. I need to create Kconfig
variable to do this, but then other problems appear.

>
> Probably also better to split out the bits in accel/xen/xen-all.c and
> hw/xen/xen-legacy-backend.c to a separate preparatory commit.

Okay, will do.

-- 
WBR, Volodymyr

Re: [EXTERNAL] [PATCH v3 2/5] xen: backends: don't overwrite XenStore nodes created by toolstack

[Volodymyr Babchuk]

Hi David,

Thank you for the review

David Woodhouse <dwmw2@infradead.org> writes:

> [[S/MIME Signed Part:Undecided]]
> On Fri, 2023-11-24 at 23:24 +0000, Volodymyr Babchuk wrote:
>> Xen PV devices in QEMU can be created in two ways: either by QEMU
>> itself, if they were passed via command line, or by Xen toolstack. In
>> the latter case, QEMU scans XenStore entries and configures devices
>> accordingly.
>> 
>> In the second case we don't want QEMU to write/delete front-end
>> entries for two reasons: it might have no access to those entries if
>> it is running in un-privileged domain and it is just incorrect to
>> overwrite entries already provided by Xen toolstack, because
>> toolstack
>> manages those nodes. For example, it might read backend- or frontend-
>> state to be sure that they are both disconnected and it is safe to
>> destroy a domain.
>> 
>> This patch checks presence of xendev->backend to check if Xen PV
>> device was configured by Xen toolstack to decide if it should touch
>> frontend entries in XenStore. Also, when we need to remove XenStore
>> entries during device teardown only if they weren't created by Xen
>> toolstack. If they were created by toolstack, then it is toolstack's
>> job to do proper clean-up.
>> 
>> Suggested-by: Paul Durrant <xadimgnik@gmail.com>
>> Suggested-by: David Woodhouse <dwmw@amazon.co.uk>
>> Co-Authored-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
>
> Reviewed-by: David Woodhouse <dwmw@amazon.co.uk>
>
> ... albeit with a couple of suggestions... 
>
>> diff --git a/hw/char/xen_console.c b/hw/char/xen_console.c
>> index bef8a3a621..b52ddddabf 100644
>> --- a/hw/char/xen_console.c
>> +++ b/hw/char/xen_console.c
>> @@ -450,7 +450,7 @@ static void xen_console_realize(XenDevice *xendev, Error **errp)
>> 
>>      trace_xen_console_realize(con->dev, object_get_typename(OBJECT(cs)));
>> 
>> -    if (CHARDEV_IS_PTY(cs)) {
>> +    if (CHARDEV_IS_PTY(cs) && !xendev->backend) {
>>          /* Strip the leading 'pty:' */
>>          xen_device_frontend_printf(xendev, "tty", "%s", cs->filename + 4);
>>      }
>
>
> It's kind of weird that that one is a frontend node at all; surely it
> should have been a backend node?

Yeah, AFAIK, console was the first PV driver, so it is a bit strange.
As I see, this frontend entry is used by "xl console" tool to find PTY
device to attach to. So yes, it should be in backend part of the
xenstore. But I don't believe we can fix this right now.

> But it is known only to QEMU once it
> actually opens /dev/ptmx and creates a new pty. It can't be populated
> by the toolstack in advance.
>
> So shouldn't the toolstack have made it writable by the driver domain?

Maybe it can lead to a weird situation when user in Dom-0 tries to use
"xl console" command to attach to a console that is absent in Dom-0,
because "tty" entry points to PTY in the driver domain.

> I think we should attempt to write this and just gracefully handle the
> failure if we can't. (In fact, xen_device_frontend_printf() will just
> use error_report_err() which is probably OK unless you feel strongly
> about silencing it).

Nope, I am fine with this approach. I'll remove this hunk in the next
version.

>
>> diff --git a/hw/net/xen_nic.c b/hw/net/xen_nic.c
>> index afa10c96e8..27442bef38 100644
>> --- a/hw/net/xen_nic.c
>> +++ b/hw/net/xen_nic.c
>> @@ -315,14 +315,16 @@ static void xen_netdev_realize(XenDevice *xendev, Error **errp)
>> 
>>      qemu_macaddr_default_if_unset(&netdev->conf.macaddr);
>> 
>> -    xen_device_frontend_printf(xendev, "mac", "%02x:%02x:%02x:%02x:%02x:%02x",
>> -                               netdev->conf.macaddr.a[0],
>> -                               netdev->conf.macaddr.a[1],
>> -                               netdev->conf.macaddr.a[2],
>> -                               netdev->conf.macaddr.a[3],
>> -                               netdev->conf.macaddr.a[4],
>> -                               netdev->conf.macaddr.a[5]);
>> -
>> +    if (!xendev->backend) {
>> +        xen_device_frontend_printf(xendev, "mac",
>> +                                   "%02x:%02x:%02x:%02x:%02x:%02x",
>> +                                   netdev->conf.macaddr.a[0],
>> +                                   netdev->conf.macaddr.a[1],
>> +                                   netdev->conf.macaddr.a[2],
>> +                                   netdev->conf.macaddr.a[3],
>> +                                   netdev->conf.macaddr.a[4],
>> +                                   netdev->conf.macaddr.a[5]);
>> +    }
>>      netdev->nic = qemu_new_nic(&net_xen_info, &netdev->conf,
>>                                 object_get_typename(OBJECT(xendev)),
>>                                 DEVICE(xendev)->id, netdev);
>
>
> Perhaps here you should create the "mac" node if it doesn't exist (or
> is that "if it doesn't match netdev->conf.macaddr"?) and just
> gracefully accept failure too?
>

I am not sure that I got this right. conf.maccadr can be sent in two
ways: via xen_net_device_create(), which will fail if toolstack didn't
provided a MAC address, or via qemu_macaddr_default_if_unset(), which is
the case for Xen emulation.

Xen toolstack is written in a such way, that it always provides "mac"
XenStore entry and frontend driver expects that this entry is always
present. So, if by any chance toolstack has not provided the MAC, then
there is something wrong and it is better to fail, what
xen_net_device_create() does. I am not sure that it is a good idea to
try to silently fix any toolstack issues.

-- 
WBR, Volodymyr

Re: [EXTERNAL] [PATCH v3 2/5] xen: backends: don't overwrite XenStore nodes created by toolstack

[David Woodhouse]

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

On Tue, 2023-11-28 at 01:20 +0000, Volodymyr Babchuk wrote:
> Hi David,
> 
> Thank you for the review
> 
> David Woodhouse <dwmw2@infradead.org> writes:
> 
> > [[S/MIME Signed Part:Undecided]]
> > On Fri, 2023-11-24 at 23:24 +0000, Volodymyr Babchuk wrote:
> > > Xen PV devices in QEMU can be created in two ways: either by QEMU
> > > itself, if they were passed via command line, or by Xen toolstack. In
> > > the latter case, QEMU scans XenStore entries and configures devices
> > > accordingly.
> > > 
> > > In the second case we don't want QEMU to write/delete front-end
> > > entries for two reasons: it might have no access to those entries if
> > > it is running in un-privileged domain and it is just incorrect to
> > > overwrite entries already provided by Xen toolstack, because
> > > toolstack
> > > manages those nodes. For example, it might read backend- or frontend-
> > > state to be sure that they are both disconnected and it is safe to
> > > destroy a domain.
> > > 
> > > This patch checks presence of xendev->backend to check if Xen PV
> > > device was configured by Xen toolstack to decide if it should touch
> > > frontend entries in XenStore. Also, when we need to remove XenStore
> > > entries during device teardown only if they weren't created by Xen
> > > toolstack. If they were created by toolstack, then it is toolstack's
> > > job to do proper clean-up.
> > > 
> > > Suggested-by: Paul Durrant <xadimgnik@gmail.com>
> > > Suggested-by: David Woodhouse <dwmw@amazon.co.uk>
> > > Co-Authored-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> > > Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
> > 
> > Reviewed-by: David Woodhouse <dwmw@amazon.co.uk>
> > 
> > ... albeit with a couple of suggestions... 
> > 
> > > diff --git a/hw/char/xen_console.c b/hw/char/xen_console.c
> > > index bef8a3a621..b52ddddabf 100644
> > > --- a/hw/char/xen_console.c
> > > +++ b/hw/char/xen_console.c
> > > @@ -450,7 +450,7 @@ static void xen_console_realize(XenDevice *xendev, Error **errp)
> > > 
> > >      trace_xen_console_realize(con->dev, object_get_typename(OBJECT(cs)));
> > > 
> > > -    if (CHARDEV_IS_PTY(cs)) {
> > > +    if (CHARDEV_IS_PTY(cs) && !xendev->backend) {
> > >          /* Strip the leading 'pty:' */
> > >          xen_device_frontend_printf(xendev, "tty", "%s", cs->filename + 4);
> > >      }
> > 
> > 
> > It's kind of weird that that one is a frontend node at all; surely it
> > should have been a backend node?
> 
> Yeah, AFAIK, console was the first PV driver, so it is a bit strange.
> As I see, this frontend entry is used by "xl console" tool to find PTY
> device to attach to. So yes, it should be in backend part of the
> xenstore. But I don't believe we can fix this right now.

Agreed.

> > But it is known only to QEMU once it
> > actually opens /dev/ptmx and creates a new pty. It can't be populated
> > by the toolstack in advance.
> > 
> > So shouldn't the toolstack have made it writable by the driver domain?
> 
> Maybe it can lead to a weird situation when user in Dom-0 tries to use
> "xl console" command to attach to a console that is absent in Dom-0,
> because "tty" entry points to PTY in the driver domain.

True, but that possibility exists the moment you have console provided
in a driver domain.

...

> > Perhaps here you should create the "mac" node if it doesn't exist (or
> > is that "if it doesn't match netdev->conf.macaddr"?) and just
> > gracefully accept failure too?
> > 
> 
> I am not sure that I got this right. conf.maccadr can be sent in two
> ways: via xen_net_device_create(), which will fail if toolstack didn't
> provided a MAC address, or via qemu_macaddr_default_if_unset(), which is
> the case for Xen emulation.

The latter happens with hotplug under Xen too, not just Xen emulation.

But the key point you make is that xen_net_device_create() will refuse
to drive a device which the toolstack created, if the "mac" node isn't
present. So creating it for ourselves based on 'if (!xendev->backend)'
as you did in your patch is reasonable enough. Thanks.


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