[XEN PATCH 0/7] dom0less PV drivers

[XEN PATCH 0/7] dom0less PV drivers

[Stefano Stabellini]

Hi all,

Currently dom0less guests cannot use PV drivers because they don't have
access to xenstore. Also, the hypervisor node in device tree is missing
so they don't detect that they are running on Xen (thus, they don't try
to enable PV interfaces.)

This patch series enables dom0less guests (on ARM) to use PV drivers.

Instead of initializing xenstore immediately at boot, dom0less guests
get access to xenstore later. They delay the initialization until they
receive a notification via the xenstore event channel (which is
available at boot.)

An example workflow is as follows:
- all domains start in parallel, dom0less guests are immediately running
- when dom0 is up and running, the init-dom0less application is called
- dom0less guests receive the notification and initialize xenstore
- now xl network-attach/disk-attach works as expected for dom0less domUs

In terms of interfaces, the patch series introduces:
- a new feature flag XENFEAT_xenstore_late_init to signal that xenstore
  is going to be available later
- a new dom0less device tree option "xen,enhanced" (in the Xen device
  tree) to specify whether PV interfaces should be enabled/disabled for
  the dom0less guest

Cheers,

Stefano

Luca Miccio (6):
      xen: introduce XENFEAT_xenstore_late_init
      xen: introduce _evtchn_alloc_unbound
      tools: add a late_init argument to xs_introduce_domain
      xen/arm: configure dom0less domain for enabling xenstore after boot
      xenstored: do_introduce: handle the late_init case
      tools: add example application to initialize dom0less PV drivers

Stefano Stabellini (1):
      xen: introduce xen,enhanced dom0less property

 docs/misc/arm/device-tree/booting.txt |  18 +++
 tools/helpers/Makefile                |  13 ++
 tools/helpers/init-dom0less.c         | 263 ++++++++++++++++++++++++++++++++++
 tools/include/xenstore.h              |   3 +-
 tools/libs/light/libxl_dom.c          |   3 +-
 tools/libs/store/xs.c                 |   8 +-
 tools/python/xen/lowlevel/xs/xs.c     |   2 +-
 tools/xenstore/xenstored_domain.c     |  15 +-
 xen/arch/arm/domain_build.c           |  39 +++++
 xen/arch/arm/include/asm/domain.h     |   2 +
 xen/arch/arm/include/asm/kernel.h     |   3 +
 xen/common/event_channel.c            |  49 ++++---
 xen/common/kernel.c                   |   2 +
 xen/include/public/features.h         |   6 +
 xen/include/xen/event.h               |   3 +
 15 files changed, 403 insertions(+), 26 deletions(-)
 create mode 100644 tools/helpers/init-dom0less.c

[XEN PATCH 1/7] xen: introduce XENFEAT_xenstore_late_init

[Stefano Stabellini]

From: Luca Miccio <lucmiccio@gmail.com>

Introduce a new feature flag to signal that xenstore will not be
immediately available at boot time. Instead, xenstore will become
available later, and a notification of xenstore readiness will be
signalled to the guest using the xenstore event channel.

Signed-off-by: Luca Miccio <lucmiccio@gmail.com>
Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
CC: Julien Grall <julien@xen.org>
CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
CC: Bertrand Marquis <bertrand.marquis@arm.com>
CC: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Jan Beulich <jbeulich@suse.com>
CC: George Dunlap <george.dunlap@citrix.com>
CC: Wei Liu <wl@xen.org>
---
 xen/arch/arm/include/asm/domain.h | 2 ++
 xen/common/kernel.c               | 2 ++
 xen/include/public/features.h     | 6 ++++++
 3 files changed, 10 insertions(+)

diff --git a/xen/arch/arm/include/asm/domain.h b/xen/arch/arm/include/asm/domain.h
index 9b3647587a..e5ae57cd09 100644
--- a/xen/arch/arm/include/asm/domain.h
+++ b/xen/arch/arm/include/asm/domain.h
@@ -89,6 +89,8 @@ struct arch_domain
 #ifdef CONFIG_TEE
     void *tee;
 #endif
+    /* Is this guest a dom0less domain? */
+    bool is_dom0less;
 }  __cacheline_aligned;
 
 struct arch_vcpu
diff --git a/xen/common/kernel.c b/xen/common/kernel.c
index e119e5401f..c00ea67e5f 100644
--- a/xen/common/kernel.c
+++ b/xen/common/kernel.c
@@ -550,6 +550,8 @@ DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
             if ( is_hardware_domain(d) )
                 fi.submap |= 1U << XENFEAT_dom0;
 #ifdef CONFIG_ARM
+            if ( d->arch.is_dom0less )
+                fi.submap |= (1U << XENFEAT_xenstore_late_init);
             fi.submap |= (1U << XENFEAT_ARM_SMCCC_supported);
 #endif
 #ifdef CONFIG_X86
diff --git a/xen/include/public/features.h b/xen/include/public/features.h
index 9ee2f760ef..18f32b1a98 100644
--- a/xen/include/public/features.h
+++ b/xen/include/public/features.h
@@ -128,6 +128,12 @@
 #define XENFEAT_not_direct_mapped         16
 #define XENFEAT_direct_mapped             17
 
+/*
+ * The xenstore interface should be initialized only after receiving a
+ * xenstore event channel notification.
+ */
+#define XENFEAT_xenstore_late_init 18
+
 #define XENFEAT_NR_SUBMAPS 1
 
 #endif /* __XEN_PUBLIC_FEATURES_H__ */
-- 
2.25.1

[XEN PATCH 2/7] xen: introduce _evtchn_alloc_unbound

[Stefano Stabellini]

From: Luca Miccio <lucmiccio@gmail.com>

The xenstore event channel will be allocated for dom0less domains. It is
necessary to have access to the evtchn_alloc_unbound function to do
that.

Factor out the part that actually allocates the event channel from
evtchn_alloc_unbound and introduce this new function as
_evtchn_alloc_unbound. (xsm_evtchn_unbound wouldn't work for a call
originated from Xen.)

Signed-off-by: Luca Miccio <lucmiccio@gmail.com>
Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
CC: Julien Grall <julien@xen.org>
CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
CC: Bertrand Marquis <bertrand.marquis@arm.com>
CC: Andrew Cooper <andrew.cooper3@citrix.com>
CC: George Dunlap <george.dunlap@citrix.com>
CC: Jan Beulich <jbeulich@suse.com>
CC: Wei Liu <wl@xen.org>
---
 xen/common/event_channel.c | 49 +++++++++++++++++++++++++-------------
 xen/include/xen/event.h    |  3 +++
 2 files changed, 36 insertions(+), 16 deletions(-)

diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
index da88ad141a..8a19bbf7ae 100644
--- a/xen/common/event_channel.c
+++ b/xen/common/event_channel.c
@@ -18,6 +18,7 @@
 
 #include <xen/init.h>
 #include <xen/lib.h>
+#include <xen/err.h>
 #include <xen/errno.h>
 #include <xen/sched.h>
 #include <xen/irq.h>
@@ -284,11 +285,32 @@ void evtchn_free(struct domain *d, struct evtchn *chn)
     xsm_evtchn_close_post(chn);
 }
 
-static int evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc)
+struct evtchn *_evtchn_alloc_unbound(struct domain *d, domid_t remote_dom)
 {
     struct evtchn *chn;
+    int port;
+
+    if ( (port = get_free_port(d)) < 0 )
+        return ERR_PTR(port);
+    chn = evtchn_from_port(d, port);
+
+    evtchn_write_lock(chn);
+
+    chn->state = ECS_UNBOUND;
+    if ( (chn->u.unbound.remote_domid = remote_dom) == DOMID_SELF )
+        chn->u.unbound.remote_domid = current->domain->domain_id;
+    evtchn_port_init(d, chn);
+
+    evtchn_write_unlock(chn);
+
+    return chn;
+}
+
+static int evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc)
+{
+    struct evtchn *chn = NULL;
     struct domain *d;
-    int            port, rc;
+    int            rc;
     domid_t        dom = alloc->dom;
 
     d = rcu_lock_domain_by_any_id(dom);
@@ -297,27 +319,22 @@ static int evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc)
 
     spin_lock(&d->event_lock);
 
-    if ( (port = get_free_port(d)) < 0 )
-        ERROR_EXIT_DOM(port, d);
-    chn = evtchn_from_port(d, port);
+    chn = _evtchn_alloc_unbound(d, alloc->remote_dom);
+    if ( IS_ERR(chn) )
+    {
+        rc = PTR_ERR(chn);
+        ERROR_EXIT_DOM(rc, d);
+    }
 
     rc = xsm_evtchn_unbound(XSM_TARGET, d, chn, alloc->remote_dom);
     if ( rc )
         goto out;
 
-    evtchn_write_lock(chn);
-
-    chn->state = ECS_UNBOUND;
-    if ( (chn->u.unbound.remote_domid = alloc->remote_dom) == DOMID_SELF )
-        chn->u.unbound.remote_domid = current->domain->domain_id;
-    evtchn_port_init(d, chn);
-
-    evtchn_write_unlock(chn);
-
-    alloc->port = port;
+    alloc->port = chn->port;
 
  out:
-    check_free_port(d, port);
+    if ( chn != NULL )
+        check_free_port(d, chn->port);
     spin_unlock(&d->event_lock);
     rcu_unlock_domain(d);
 
diff --git a/xen/include/xen/event.h b/xen/include/xen/event.h
index 21c95e14fd..6aedbccbf1 100644
--- a/xen/include/xen/event.h
+++ b/xen/include/xen/event.h
@@ -68,6 +68,9 @@ int evtchn_close(struct domain *d1, int port1, bool guest);
 /* Free an event channel. */
 void evtchn_free(struct domain *d, struct evtchn *chn);
 
+/* Create a new event channel port */
+struct evtchn *_evtchn_alloc_unbound(struct domain *d, domid_t remote_dom);
+
 /* Allocate a specific event channel port. */
 int evtchn_allocate_port(struct domain *d, unsigned int port);
 
-- 
2.25.1

[XEN PATCH 3/7] tools: add a late_init argument to xs_introduce_domain

[Stefano Stabellini]

From: Luca Miccio <lucmiccio@gmail.com>

Add a late_init argument to xs_introduce_domain to handle dom0less
guests whose xenstore interfaces are initialized after boot.

This patch mechanically adds the new parameter; it doesn't change
behaviors.

Signed-off-by: Luca Miccio <lucmiccio@gmail.com>
Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
CC: wl@xen.org
CC: anthony.perard@citrix.com
CC: jgross@suse.com
CC: marmarek@invisiblethingslab.com
---
 tools/include/xenstore.h          | 3 ++-
 tools/libs/light/libxl_dom.c      | 3 ++-
 tools/libs/store/xs.c             | 8 ++++++--
 tools/python/xen/lowlevel/xs/xs.c | 2 +-
 4 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/tools/include/xenstore.h b/tools/include/xenstore.h
index 2b3f69fb61..1a302b5ff9 100644
--- a/tools/include/xenstore.h
+++ b/tools/include/xenstore.h
@@ -226,7 +226,8 @@ bool xs_transaction_end(struct xs_handle *h, xs_transaction_t t,
 bool xs_introduce_domain(struct xs_handle *h,
 			 unsigned int domid,
 			 unsigned long mfn,
-                         unsigned int eventchn); 
+			 unsigned int eventchn,
+			 bool late_init);
 
 /* Set the target of a domain
  * This tells the store daemon that a domain is targetting another one, so
diff --git a/tools/libs/light/libxl_dom.c b/tools/libs/light/libxl_dom.c
index 2abaab439c..bacfdfa9df 100644
--- a/tools/libs/light/libxl_dom.c
+++ b/tools/libs/light/libxl_dom.c
@@ -497,7 +497,8 @@ retry_transaction:
     if (!xs_transaction_end(ctx->xsh, t, 0))
         if (errno == EAGAIN)
             goto retry_transaction;
-    xs_introduce_domain(ctx->xsh, domid, state->store_mfn, state->store_port);
+    xs_introduce_domain(ctx->xsh, domid, state->store_mfn, state->store_port,
+                        false);
     free(vm_path);
     return 0;
 }
diff --git a/tools/libs/store/xs.c b/tools/libs/store/xs.c
index 7a9a8b1656..dd47d607fd 100644
--- a/tools/libs/store/xs.c
+++ b/tools/libs/store/xs.c
@@ -1089,16 +1089,18 @@ bool xs_transaction_end(struct xs_handle *h, xs_transaction_t t,
  */
 bool xs_introduce_domain(struct xs_handle *h,
 			 unsigned int domid, unsigned long mfn,
-			 unsigned int eventchn)
+			 unsigned int eventchn, bool late_init)
 {
 	char domid_str[MAX_STRLEN(domid)];
 	char mfn_str[MAX_STRLEN(mfn)];
 	char eventchn_str[MAX_STRLEN(eventchn)];
-	struct iovec iov[3];
+	char late_init_str[MAX_STRLEN(late_init)];
+	struct iovec iov[4];
 
 	snprintf(domid_str, sizeof(domid_str), "%u", domid);
 	snprintf(mfn_str, sizeof(mfn_str), "%lu", mfn);
 	snprintf(eventchn_str, sizeof(eventchn_str), "%u", eventchn);
+	snprintf(late_init_str, sizeof(late_init_str), "%u", late_init);
 
 	iov[0].iov_base = domid_str;
 	iov[0].iov_len = strlen(domid_str) + 1;
@@ -1106,6 +1108,8 @@ bool xs_introduce_domain(struct xs_handle *h,
 	iov[1].iov_len = strlen(mfn_str) + 1;
 	iov[2].iov_base = eventchn_str;
 	iov[2].iov_len = strlen(eventchn_str) + 1;
+	iov[3].iov_base = late_init_str;
+	iov[3].iov_len = strlen(late_init_str) + 1;
 
 	return xs_bool(xs_talkv(h, XBT_NULL, XS_INTRODUCE, iov,
 				ARRAY_SIZE(iov), NULL));
diff --git a/tools/python/xen/lowlevel/xs/xs.c b/tools/python/xen/lowlevel/xs/xs.c
index 0dad7fa5f2..dc667fb231 100644
--- a/tools/python/xen/lowlevel/xs/xs.c
+++ b/tools/python/xen/lowlevel/xs/xs.c
@@ -678,7 +678,7 @@ static PyObject *xspy_introduce_domain(XsHandle *self, PyObject *args)
         return NULL;
 
     Py_BEGIN_ALLOW_THREADS
-    result = xs_introduce_domain(xh, dom, page, port);
+    result = xs_introduce_domain(xh, dom, page, port, false);
     Py_END_ALLOW_THREADS
 
     return none(result);
-- 
2.25.1

[XEN PATCH 4/7] xen: introduce xen,enhanced dom0less property

[Stefano Stabellini]

From: Stefano Stabellini <stefano.stabellini@xilinx.com>

Introduce a new "xen,enhanced" dom0less property to enable/disable PV
driver interfaces for dom0less guests. Currently only "enabled" and
"disabled" are supported property values (and empty). Leave the option
open to implement further possible values in the future (e.g.
"xenstore" to enable only xenstore.)

This patch only parses the property. Next patches will make use of it.

Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
CC: Julien Grall <julien@xen.org>
CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
CC: Bertrand Marquis <bertrand.marquis@arm.com>
---
 docs/misc/arm/device-tree/booting.txt | 18 ++++++++++++++++++
 xen/arch/arm/domain_build.c           |  5 +++++
 xen/arch/arm/include/asm/kernel.h     |  3 +++
 3 files changed, 26 insertions(+)

diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt
index 71895663a4..38c29fb3d8 100644
--- a/docs/misc/arm/device-tree/booting.txt
+++ b/docs/misc/arm/device-tree/booting.txt
@@ -169,6 +169,24 @@ with the following properties:
     Please note that the SPI used for the virtual pl011 could clash with the
     physical SPI of a physical device assigned to the guest.
 
+- xen,enhanced
+
+    A string property. Possible property values are:
+
+    - "enabled" (or missing property value)
+    Xen PV interfaces, including grant-table and xenstore, will be
+    enabled for the VM.
+
+    - "disabled"
+    Xen PV interfaces are disabled.
+
+    If the xen,enhanced property is present with no value, it defaults
+    to "enabled". If the xen,enhanced property is not present, PV
+    interfaces are disabled.
+
+    In the future other possible property values might be added to
+    enable only selected interfaces.
+
 - nr_spis
 
     Optional. A 32-bit integer specifying the number of SPIs (Shared
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 6931c022a2..96a94fa434 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -2963,6 +2963,7 @@ static int __init construct_domU(struct domain *d,
                                  const struct dt_device_node *node)
 {
     struct kernel_info kinfo = {};
+    const char *enhanced;
     int rc;
     u64 mem;
 
@@ -2978,6 +2979,10 @@ static int __init construct_domU(struct domain *d,
 
     kinfo.vpl011 = dt_property_read_bool(node, "vpl011");
 
+    rc = dt_property_read_string(node, "xen,enhanced", &enhanced);
+    if ( rc == -EILSEQ || (rc == 0 && !strcmp(enhanced, "enabled")) )
+        kinfo.enhanced = true;
+
     if ( vcpu_create(d, 0) == NULL )
         return -ENOMEM;
 
diff --git a/xen/arch/arm/include/asm/kernel.h b/xen/arch/arm/include/asm/kernel.h
index 874aa108a7..3275f7fbca 100644
--- a/xen/arch/arm/include/asm/kernel.h
+++ b/xen/arch/arm/include/asm/kernel.h
@@ -36,6 +36,9 @@ struct kernel_info {
     /* Enable pl011 emulation */
     bool vpl011;
 
+    /* Enable PV drivers */
+    bool enhanced;
+
     /* GIC phandle */
     uint32_t phandle_gic;
 
-- 
2.25.1

[XEN PATCH 5/7] xen/arm: configure dom0less domain for enabling xenstore after boot

[Stefano Stabellini]

From: Luca Miccio <lucmiccio@gmail.com>

If "xen,enhanced" is enabled, then add to dom0less domains:

- the hypervisor node in device tree
- the xenstore event channel

The xenstore event channel is also used for the first notification to
let the guest know that xenstore has become available.

Signed-off-by: Luca Miccio <lucmiccio@gmail.com>
Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
CC: Julien Grall <julien@xen.org>
CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
CC: Bertrand Marquis <bertrand.marquis@arm.com>
---
 xen/arch/arm/domain_build.c | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 96a94fa434..fafa921aa8 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -27,6 +27,7 @@
 #include <asm/setup.h>
 #include <asm/cpufeature.h>
 #include <asm/domain_build.h>
+#include <xen/event.h>
 
 #include <xen/irq.h>
 #include <xen/grant_table.h>
@@ -2619,6 +2620,8 @@ static int __init prepare_dtb_domU(struct domain *d, struct kernel_info *kinfo)
     int ret;
 
     kinfo->phandle_gic = GUEST_PHANDLE_GIC;
+    kinfo->gnttab_start = GUEST_GNTTAB_BASE;
+    kinfo->gnttab_size = GUEST_GNTTAB_SIZE;
 
     addrcells = GUEST_ROOT_ADDRESS_CELLS;
     sizecells = GUEST_ROOT_SIZE_CELLS;
@@ -2693,6 +2696,13 @@ static int __init prepare_dtb_domU(struct domain *d, struct kernel_info *kinfo)
             goto err;
     }
 
+    if ( kinfo->enhanced )
+    {
+        ret = make_hypervisor_node(d, kinfo, addrcells, sizecells);
+        if ( ret )
+            goto err;
+    }
+
     ret = fdt_end_node(kinfo->fdt);
     if ( ret < 0 )
         goto err;
@@ -2959,6 +2969,22 @@ static int __init construct_domain(struct domain *d, struct kernel_info *kinfo)
     return 0;
 }
 
+static int alloc_xenstore_evtchn(struct domain *d)
+{
+    struct evtchn *chn;
+
+    chn = _evtchn_alloc_unbound(d, hardware_domain->domain_id);
+    if ( !chn )
+    {
+        printk("Failed allocating event channel for domain\n");
+        return -ENOMEM;
+    }
+
+    d->arch.hvm.params[HVM_PARAM_STORE_EVTCHN] = chn->port;
+
+    return 0;
+}
+
 static int __init construct_domU(struct domain *d,
                                  const struct dt_device_node *node)
 {
@@ -3012,7 +3038,14 @@ static int __init construct_domU(struct domain *d,
         return rc;
 
     if ( kinfo.vpl011 )
+    {
         rc = domain_vpl011_init(d, NULL);
+        if ( rc < 0 )
+            return rc;
+    }
+
+    if ( kinfo.enhanced )
+        rc = alloc_xenstore_evtchn(d);
 
     return rc;
 }
@@ -3068,6 +3101,7 @@ void __init create_domUs(void)
             panic("Error creating domain %s\n", dt_node_name(node));
 
         d->is_console = true;
+        d->arch.is_dom0less = true;
 
         if ( construct_domU(d, node) != 0 )
             panic("Could not set up domain %s\n", dt_node_name(node));
-- 
2.25.1

[XEN PATCH 6/7] xenstored: do_introduce: handle the late_init case

[Stefano Stabellini]

From: Luca Miccio <lucmiccio@gmail.com>

If the function is called with late_init set then also notify the domain
using the xenstore event channel.

Signed-off-by: Luca Miccio <lucmiccio@gmail.com>
Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
CC: wl@xen.org
CC: Anthony PERARD <anthony.perard@citrix.com>
CC: Juergen Gross <jgross@suse.com>
CC: julien@xen.org
---
 tools/xenstore/xenstored_domain.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/tools/xenstore/xenstored_domain.c b/tools/xenstore/xenstored_domain.c
index d03c7d93a9..17b8021ca8 100644
--- a/tools/xenstore/xenstored_domain.c
+++ b/tools/xenstore/xenstored_domain.c
@@ -429,7 +429,7 @@ static void domain_conn_reset(struct domain *domain)
 
 static struct domain *introduce_domain(const void *ctx,
 				       unsigned int domid,
-				       evtchn_port_t port, bool restore)
+				       evtchn_port_t port, bool restore, bool late_init)
 {
 	struct domain *domain;
 	int rc;
@@ -461,6 +461,9 @@ static struct domain *introduce_domain(const void *ctx,
 		/* Now domain belongs to its connection. */
 		talloc_steal(domain->conn, domain);
 
+		if (late_init)
+			xenevtchn_notify(xce_handle, domain->port);
+
 		if (!is_master_domain && !restore)
 			fire_watches(NULL, ctx, "@introduceDomain", NULL,
 				     false, NULL);
@@ -479,9 +482,10 @@ static struct domain *introduce_domain(const void *ctx,
 int do_introduce(struct connection *conn, struct buffered_data *in)
 {
 	struct domain *domain;
-	char *vec[3];
+	char *vec[4];
 	unsigned int domid;
 	evtchn_port_t port;
+	bool late_init;
 
 	if (get_strings(in, vec, ARRAY_SIZE(vec)) < ARRAY_SIZE(vec))
 		return EINVAL;
@@ -489,12 +493,13 @@ int do_introduce(struct connection *conn, struct buffered_data *in)
 	domid = atoi(vec[0]);
 	/* Ignore the gfn, we don't need it. */
 	port = atoi(vec[2]);
+	late_init = atoi(vec[3]);
 
 	/* Sanity check args. */
 	if (port <= 0)
 		return EINVAL;
 
-	domain = introduce_domain(in, domid, port, false);
+	domain = introduce_domain(in, domid, port, false, late_init);
 	if (!domain)
 		return errno;
 
@@ -723,7 +728,7 @@ void dom0_init(void)
 	if (port == -1)
 		barf_perror("Failed to initialize dom0 port");
 
-	dom0 = introduce_domain(NULL, xenbus_master_domid(), port, false);
+	dom0 = introduce_domain(NULL, xenbus_master_domid(), port, false, false);
 	if (!dom0)
 		barf_perror("Failed to initialize dom0");
 
@@ -1292,7 +1297,7 @@ void read_state_connection(const void *ctx, const void *state)
 #endif
 	} else {
 		domain = introduce_domain(ctx, sc->spec.ring.domid,
-					  sc->spec.ring.evtchn, true);
+					  sc->spec.ring.evtchn, true, false);
 		if (!domain)
 			barf("domain allocation error");
 
-- 
2.25.1

[XEN PATCH 7/7] tools: add example application to initialize dom0less PV drivers

[Stefano Stabellini]

From: Luca Miccio <lucmiccio@gmail.com>

Add an example application that can be run in dom0 to complete the
dom0less domains initialization so that they can get access to xenstore
and use PV drivers.

Signed-off-by: Luca Miccio <lucmiccio@gmail.com>
Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
CC: Wei Liu <wl@xen.org>
CC: Anthony PERARD <anthony.perard@citrix.com>
CC: Juergen Gross <jgross@suse.com>
---
 tools/helpers/Makefile        |  13 ++
 tools/helpers/init-dom0less.c | 263 ++++++++++++++++++++++++++++++++++
 2 files changed, 276 insertions(+)
 create mode 100644 tools/helpers/init-dom0less.c

diff --git a/tools/helpers/Makefile b/tools/helpers/Makefile
index 7f6c422440..8e42997052 100644
--- a/tools/helpers/Makefile
+++ b/tools/helpers/Makefile
@@ -10,6 +10,9 @@ ifeq ($(CONFIG_Linux),y)
 ifeq ($(CONFIG_X86),y)
 PROGS += init-xenstore-domain
 endif
+ifeq ($(CONFIG_ARM),y)
+PROGS += init-dom0less
+endif
 endif
 
 XEN_INIT_DOM0_OBJS = xen-init-dom0.o init-dom-json.o
@@ -26,6 +29,13 @@ $(INIT_XENSTORE_DOMAIN_OBJS): CFLAGS += $(CFLAGS_libxenstore)
 $(INIT_XENSTORE_DOMAIN_OBJS): CFLAGS += $(CFLAGS_libxenlight)
 $(INIT_XENSTORE_DOMAIN_OBJS): CFLAGS += -include $(XEN_ROOT)/tools/config.h
 
+INIT_DOM0LESS_OBJS = init-dom0less.o init-dom-json.o
+$(INIT_DOM0LESS_OBJS): CFLAGS += $(CFLAGS_libxentoollog)
+$(INIT_DOM0LESS_OBJS): CFLAGS += $(CFLAGS_libxenstore)
+$(INIT_DOM0LESS_OBJS): CFLAGS += $(CFLAGS_libxenlight)
+$(INIT_DOM0LESS_OBJS): CFLAGS += $(CFLAGS_libxenctrl)
+$(INIT_DOM0LESS_OBJS): CFLAGS += $(CFLAGS_libxenevtchn)
+
 .PHONY: all
 all: $(PROGS)
 
@@ -35,6 +45,9 @@ xen-init-dom0: $(XEN_INIT_DOM0_OBJS)
 init-xenstore-domain: $(INIT_XENSTORE_DOMAIN_OBJS)
 	$(CC) $(LDFLAGS) -o $@ $(INIT_XENSTORE_DOMAIN_OBJS) $(LDLIBS_libxentoollog) $(LDLIBS_libxenstore) $(LDLIBS_libxenctrl) $(LDLIBS_libxenguest) $(LDLIBS_libxenlight) $(APPEND_LDFLAGS)
 
+init-dom0less: $(INIT_DOM0LESS_OBJS)
+	$(CC) $(LDFLAGS) -o $@ $(INIT_DOM0LESS_OBJS) $(LDLIBS_libxenctrl) $(LDLIBS_libxenevtchn) $(LDLIBS_libxentoollog) $(LDLIBS_libxenstore) $(LDLIBS_libxenlight) $(LDLIBS_libxenguest)  $(APPEND_LDFLAGS)
+
 .PHONY: install
 install: all
 	$(INSTALL_DIR) $(DESTDIR)$(LIBEXEC_BIN)
diff --git a/tools/helpers/init-dom0less.c b/tools/helpers/init-dom0less.c
new file mode 100644
index 0000000000..055bf76cf5
--- /dev/null
+++ b/tools/helpers/init-dom0less.c
@@ -0,0 +1,263 @@
+#include <stdbool.h>
+#include <syslog.h>
+#include <stdio.h>
+#include <err.h>
+#include <stdlib.h>
+#include <xenstore.h>
+#include <xenctrl.h>
+#include <xenguest.h>
+#include <libxl.h>
+#include <xenevtchn.h>
+
+#include "init-dom-json.h"
+
+#define NR_MAGIC_PAGES 4
+#define CONSOLE_PFN_OFFSET 0
+#define XENSTORE_PFN_OFFSET 1
+#define STR_MAX_LENGTH 64
+
+static int alloc_magic_pages(struct xc_dom_image *dom)
+{
+    int rc, i;
+    const xen_pfn_t base = GUEST_MAGIC_BASE >> XC_PAGE_SHIFT;
+    xen_pfn_t p2m[NR_MAGIC_PAGES];
+
+    for (i = 0; i < NR_MAGIC_PAGES; i++)
+        p2m[i] = base + i;
+
+    rc = xc_domain_populate_physmap_exact(dom->xch, dom->guest_domid,
+                                          NR_MAGIC_PAGES, 0, 0, p2m);
+    if (rc < 0)
+        return rc;
+
+    dom->xenstore_pfn = base + XENSTORE_PFN_OFFSET;
+
+    xc_clear_domain_page(dom->xch, dom->guest_domid, dom->xenstore_pfn);
+
+    xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_STORE_PFN,
+                     dom->xenstore_pfn);
+    xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_STORE_EVTCHN,
+                     dom->xenstore_evtchn);
+    return 0;
+}
+
+static void do_xs_write(struct xs_handle *xsh, xs_transaction_t t,
+                        char *path, char *val)
+{
+    if (!xs_write(xsh, t, path, val, strlen(val)))
+        fprintf(stderr, "writing %s to xenstore failed.\n", path);
+}
+
+static void do_xs_write_dom(struct xs_handle *xsh, xs_transaction_t t,
+                            domid_t domid, char *path, char *val)
+{
+    char full_path[STR_MAX_LENGTH];
+
+    snprintf(full_path, STR_MAX_LENGTH,
+             "/local/domain/%d/%s", domid, path);
+    do_xs_write(xsh, t, full_path, val);
+}
+
+static void do_xs_write_libxl(struct xs_handle *xsh, xs_transaction_t t,
+                              domid_t domid, char *path, char *val)
+{
+    char full_path[STR_MAX_LENGTH];
+
+    snprintf(full_path, STR_MAX_LENGTH,
+             "/libxl/%d/%s", domid, path);
+    do_xs_write(xsh, t, full_path, val);
+}
+
+static void do_xs_write_vm(struct xs_handle *xsh, xs_transaction_t t,
+                           libxl_uuid uuid, char *path, char *val)
+{
+    char full_path[STR_MAX_LENGTH];
+
+    snprintf(full_path, STR_MAX_LENGTH,
+             "/vm/" LIBXL_UUID_FMT "/%s", LIBXL_UUID_BYTES(uuid), path);
+    do_xs_write(xsh, t, full_path, val);
+}
+
+static int restore_xenstore(struct xs_handle *xsh,
+                            libxl_dominfo *info, libxl_uuid uuid,
+                            evtchn_port_t xenstore_port)
+{
+    domid_t domid;
+    int i;
+    char uuid_str[STR_MAX_LENGTH];
+    char dom_name_str[STR_MAX_LENGTH];
+    char vm_val_str[STR_MAX_LENGTH];
+    char id_str[STR_MAX_LENGTH];
+    char max_memkb_str[STR_MAX_LENGTH];
+    char cpu_str[STR_MAX_LENGTH];
+    char xenstore_port_str[STR_MAX_LENGTH];
+    char ring_ref_str[STR_MAX_LENGTH];
+    xs_transaction_t t;
+
+    domid = info->domid;
+    snprintf(id_str, STR_MAX_LENGTH, "%d", domid);
+    snprintf(dom_name_str, STR_MAX_LENGTH, "dom0less-%d", domid);
+    snprintf(uuid_str, STR_MAX_LENGTH, LIBXL_UUID_FMT, LIBXL_UUID_BYTES(uuid));
+    snprintf(vm_val_str, STR_MAX_LENGTH,
+             "vm/" LIBXL_UUID_FMT, LIBXL_UUID_BYTES(uuid));
+    snprintf(max_memkb_str, STR_MAX_LENGTH, "%lu", info->max_memkb);
+    snprintf(ring_ref_str, STR_MAX_LENGTH, "%lld",
+             (GUEST_MAGIC_BASE >> XC_PAGE_SHIFT) + XENSTORE_PFN_OFFSET);
+    snprintf(xenstore_port_str, STR_MAX_LENGTH, "%d", xenstore_port);
+
+retry_transaction:
+    t = xs_transaction_start(xsh);
+    if (t == XBT_NULL)
+        return errno;
+
+    /* /vm */
+    do_xs_write_vm(xsh, t, uuid, "name", dom_name_str);
+    do_xs_write_vm(xsh, t, uuid, "uuid", uuid_str);
+    do_xs_write_vm(xsh, t, uuid, "start_time", "0");
+
+    /* /domain */
+    do_xs_write_dom(xsh, t, domid, "vm", vm_val_str);
+    do_xs_write_dom(xsh, t, domid, "name", dom_name_str);
+    do_xs_write_dom(xsh, t, domid, "cpu", "");
+    for (i = 0; i < info->vcpu_max_id; i++) {
+        snprintf(cpu_str, STR_MAX_LENGTH, "cpu/%d/availability/", i);
+        do_xs_write_dom(xsh, t, domid, cpu_str,
+                        (info->cpupool & (1 << i)) ? "online" : "offline");
+    }
+    do_xs_write_dom(xsh, t, domid, "cpu/0", "");
+    do_xs_write_dom(xsh, t, domid, "cpu/availability", "online");
+
+    do_xs_write_dom(xsh, t, domid, "memory", "");
+    do_xs_write_dom(xsh, t, domid, "memory/static-max", max_memkb_str);
+    do_xs_write_dom(xsh, t, domid, "memory/videoram", "-1");
+
+    do_xs_write_dom(xsh, t, domid, "device", "");
+    do_xs_write_dom(xsh, t, domid, "device/suspend", "");
+    do_xs_write_dom(xsh, t, domid, "device/suspend/event-channel", "");
+
+    do_xs_write_dom(xsh, t, domid, "control", "");
+    do_xs_write_dom(xsh, t, domid, "control/shutdown", "");
+    do_xs_write_dom(xsh, t, domid, "control/feature-poweroff", "1");
+    do_xs_write_dom(xsh, t, domid, "control/feature-reboot", "1");
+    do_xs_write_dom(xsh, t, domid, "control/feature-suspend", "");
+    do_xs_write_dom(xsh, t, domid, "control/sysrq", "");
+    do_xs_write_dom(xsh, t, domid, "control/platform-feature-multiprocessor-suspend", "1");
+    do_xs_write_dom(xsh, t, domid, "control", "platform-feature-xs_reset_watches");
+
+    do_xs_write_dom(xsh, t, domid, "domid", id_str);
+    do_xs_write_dom(xsh, t, domid, "data", "");
+    do_xs_write_dom(xsh, t, domid, "drivers", "");
+    do_xs_write_dom(xsh, t, domid, "feature", "");
+    do_xs_write_dom(xsh, t, domid, "attr", "");
+
+    do_xs_write_dom(xsh, t, domid, "store/port", xenstore_port_str);
+    do_xs_write_dom(xsh, t, domid, "store/ring-ref", ring_ref_str);
+
+    do_xs_write_libxl(xsh, t, domid, "type", "pvh");
+    do_xs_write_libxl(xsh, t, domid, "dm-version", "qemu_xen");
+
+    if (!xs_transaction_end(xsh, t, false))
+        if (errno == EAGAIN)
+            goto retry_transaction;
+
+    return 0;
+}
+
+static int init_domain(struct xs_handle *xsh, libxl_dominfo *info)
+{
+    struct xc_dom_image dom;
+    libxl_uuid uuid;
+    uint64_t v;
+    int rc;
+
+    printf("#### Init dom0less domain: %d ####\n", info->domid);
+    dom.guest_domid = info->domid;
+    dom.xenstore_domid = 0;
+    dom.xch = xc_interface_open(0, 0, 0);
+
+    rc = xc_hvm_param_get(dom.xch, info->domid, HVM_PARAM_STORE_EVTCHN, &v);
+    if (rc != 0) {
+        printf("Failed to get HVM_PARAM_STORE_EVTCHN\n");
+        return 1;
+    }
+    dom.xenstore_evtchn = v;
+
+    /* Console won't be initialized but set its data for completeness */
+    dom.console_domid = 0;
+
+    /* Alloc magic pages */
+    printf("Allocating magic pages\n");
+    if (alloc_magic_pages(&dom) != 0) {
+        printf("Error on alloc magic pages\n");
+        return 1;
+    }
+
+    printf("Setup Grant Tables\n");
+    xc_dom_gnttab_init(&dom);
+
+    printf("Setup UUID\n");
+    libxl_uuid_generate(&uuid);
+    xc_domain_sethandle(dom.xch, info->domid, libxl_uuid_bytearray(&uuid));
+
+    printf("Creating JSON\n");
+    rc = gen_stub_json_config(info->domid, &uuid);
+    if (rc)
+        err(1, "gen_stub_json_config");
+
+    printf("Restoring Xenstore values\n");
+    restore_xenstore(xsh, info, uuid, dom.xenstore_evtchn);
+
+    printf("Introducing domain\n");
+    xs_introduce_domain(xsh, info->domid,
+            (GUEST_MAGIC_BASE >> XC_PAGE_SHIFT) + XENSTORE_PFN_OFFSET,
+            dom.xenstore_evtchn, true);
+    return 0;
+}
+
+/* Check if domain has been configured in XS */
+static bool domain_exists(struct xs_handle *xsh, int domid)
+{
+    return xs_is_domain_introduced(xsh, domid);
+}
+
+int main(int argc, char **argv)
+{
+    libxl_dominfo *info;
+    libxl_ctx *ctx;
+    int nb_vm, i;
+    struct xs_handle *xsh;
+
+    xsh = xs_daemon_open();
+    if (xsh == NULL) {
+        fprintf(stderr, "Could not contact XenStore");
+        exit(1);
+    }
+
+    if (libxl_ctx_alloc(&ctx, LIBXL_VERSION, 0, NULL)) {
+        fprintf(stderr, "cannot init xl context\n");
+        exit(1);
+    }
+
+    info = libxl_list_domain(ctx, &nb_vm);
+    if (!info) {
+        fprintf(stderr, "libxl_list_vm failed.\n");
+        exit(EXIT_FAILURE);
+    }
+
+    for (i = 0; i < nb_vm; i++) {
+        domid_t domid = info[i].domid;
+
+        /* Don't need to check for Dom0 */
+        if (!domid)
+            continue;
+
+        printf("Checking domid: %u\n", domid);
+        if (!domain_exists(xsh, domid))
+            init_domain(xsh, &info[i]);
+        else
+            printf("Domain %d has already been initialized\n", domid);
+    }
+    libxl_dominfo_list_free(info, nb_vm);
+    xs_close(xsh);
+    return 0;
+}
-- 
2.25.1

Re: [XEN PATCH 3/7] tools: add a late_init argument to xs_introduce_domain

[Marek Marczykowski-Górecki]

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

On Fri, Jan 07, 2022 at 04:49:08PM -0800, Stefano Stabellini wrote:
> From: Luca Miccio <lucmiccio@gmail.com>
> 
> Add a late_init argument to xs_introduce_domain to handle dom0less
> guests whose xenstore interfaces are initialized after boot.
> 
> This patch mechanically adds the new parameter; it doesn't change
> behaviors.
> 
> Signed-off-by: Luca Miccio <lucmiccio@gmail.com>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>

I realize there is not much sense in making the parameter usable in the
Python API, since it's only useful for xenstored. So, for the Python part:

Acked-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>

> CC: wl@xen.org
> CC: anthony.perard@citrix.com
> CC: jgross@suse.com
> CC: marmarek@invisiblethingslab.com
> ---
>  tools/include/xenstore.h          | 3 ++-
>  tools/libs/light/libxl_dom.c      | 3 ++-
>  tools/libs/store/xs.c             | 8 ++++++--
>  tools/python/xen/lowlevel/xs/xs.c | 2 +-
>  4 files changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/include/xenstore.h b/tools/include/xenstore.h
> index 2b3f69fb61..1a302b5ff9 100644
> --- a/tools/include/xenstore.h
> +++ b/tools/include/xenstore.h
> @@ -226,7 +226,8 @@ bool xs_transaction_end(struct xs_handle *h, xs_transaction_t t,
>  bool xs_introduce_domain(struct xs_handle *h,
>  			 unsigned int domid,
>  			 unsigned long mfn,
> -                         unsigned int eventchn); 
> +			 unsigned int eventchn,
> +			 bool late_init);
>  
>  /* Set the target of a domain
>   * This tells the store daemon that a domain is targetting another one, so
> diff --git a/tools/libs/light/libxl_dom.c b/tools/libs/light/libxl_dom.c
> index 2abaab439c..bacfdfa9df 100644
> --- a/tools/libs/light/libxl_dom.c
> +++ b/tools/libs/light/libxl_dom.c
> @@ -497,7 +497,8 @@ retry_transaction:
>      if (!xs_transaction_end(ctx->xsh, t, 0))
>          if (errno == EAGAIN)
>              goto retry_transaction;
> -    xs_introduce_domain(ctx->xsh, domid, state->store_mfn, state->store_port);
> +    xs_introduce_domain(ctx->xsh, domid, state->store_mfn, state->store_port,
> +                        false);
>      free(vm_path);
>      return 0;
>  }
> diff --git a/tools/libs/store/xs.c b/tools/libs/store/xs.c
> index 7a9a8b1656..dd47d607fd 100644
> --- a/tools/libs/store/xs.c
> +++ b/tools/libs/store/xs.c
> @@ -1089,16 +1089,18 @@ bool xs_transaction_end(struct xs_handle *h, xs_transaction_t t,
>   */
>  bool xs_introduce_domain(struct xs_handle *h,
>  			 unsigned int domid, unsigned long mfn,
> -			 unsigned int eventchn)
> +			 unsigned int eventchn, bool late_init)
>  {
>  	char domid_str[MAX_STRLEN(domid)];
>  	char mfn_str[MAX_STRLEN(mfn)];
>  	char eventchn_str[MAX_STRLEN(eventchn)];
> -	struct iovec iov[3];
> +	char late_init_str[MAX_STRLEN(late_init)];
> +	struct iovec iov[4];
>  
>  	snprintf(domid_str, sizeof(domid_str), "%u", domid);
>  	snprintf(mfn_str, sizeof(mfn_str), "%lu", mfn);
>  	snprintf(eventchn_str, sizeof(eventchn_str), "%u", eventchn);
> +	snprintf(late_init_str, sizeof(late_init_str), "%u", late_init);
>  
>  	iov[0].iov_base = domid_str;
>  	iov[0].iov_len = strlen(domid_str) + 1;
> @@ -1106,6 +1108,8 @@ bool xs_introduce_domain(struct xs_handle *h,
>  	iov[1].iov_len = strlen(mfn_str) + 1;
>  	iov[2].iov_base = eventchn_str;
>  	iov[2].iov_len = strlen(eventchn_str) + 1;
> +	iov[3].iov_base = late_init_str;
> +	iov[3].iov_len = strlen(late_init_str) + 1;
>  
>  	return xs_bool(xs_talkv(h, XBT_NULL, XS_INTRODUCE, iov,
>  				ARRAY_SIZE(iov), NULL));
> diff --git a/tools/python/xen/lowlevel/xs/xs.c b/tools/python/xen/lowlevel/xs/xs.c
> index 0dad7fa5f2..dc667fb231 100644
> --- a/tools/python/xen/lowlevel/xs/xs.c
> +++ b/tools/python/xen/lowlevel/xs/xs.c
> @@ -678,7 +678,7 @@ static PyObject *xspy_introduce_domain(XsHandle *self, PyObject *args)
>          return NULL;
>  
>      Py_BEGIN_ALLOW_THREADS
> -    result = xs_introduce_domain(xh, dom, page, port);
> +    result = xs_introduce_domain(xh, dom, page, port, false);
>      Py_END_ALLOW_THREADS
>  
>      return none(result);
> -- 
> 2.25.1
> 

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

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

Re: [XEN PATCH 6/7] xenstored: do_introduce: handle the late_init case

[Marek Marczykowski-Górecki]

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

On Fri, Jan 07, 2022 at 04:49:11PM -0800, Stefano Stabellini wrote:
> From: Luca Miccio <lucmiccio@gmail.com>
> 
> If the function is called with late_init set then also notify the domain
> using the xenstore event channel.
> 
> Signed-off-by: Luca Miccio <lucmiccio@gmail.com>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
> CC: wl@xen.org
> CC: Anthony PERARD <anthony.perard@citrix.com>
> CC: Juergen Gross <jgross@suse.com>
> CC: julien@xen.org
> ---
>  tools/xenstore/xenstored_domain.c | 15 ++++++++++-----

Isn't the same necessary in oxenstored too? Otherwise, I think it needs
some explicit documentation, that late PV with dom0less requires
cxenstored.

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

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

Re: [XEN PATCH 1/7] xen: introduce XENFEAT_xenstore_late_init

[Julien Grall]

Hi,

On 08/01/2022 00:49, Stefano Stabellini wrote:
> From: Luca Miccio <lucmiccio@gmail.com>
> 
> Introduce a new feature flag to signal that xenstore will not be
> immediately available at boot time. Instead, xenstore will become
> available later, and a notification of xenstore readiness will be
> signalled to the guest using the xenstore event channel.

Hmmm... On the thread [1], you semmed to imply that new Linux version (I 
am assuming master) are ready to be used in dom0less with the node xen. 
So I am bit confused why this is necessary?

> 
> Signed-off-by: Luca Miccio <lucmiccio@gmail.com>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
> CC: Julien Grall <julien@xen.org>
> CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
> CC: Bertrand Marquis <bertrand.marquis@arm.com>
> CC: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Jan Beulich <jbeulich@suse.com>
> CC: George Dunlap <george.dunlap@citrix.com>
> CC: Wei Liu <wl@xen.org>
> ---
>   xen/arch/arm/include/asm/domain.h | 2 ++
>   xen/common/kernel.c               | 2 ++
>   xen/include/public/features.h     | 6 ++++++
>   3 files changed, 10 insertions(+)
> 
> diff --git a/xen/arch/arm/include/asm/domain.h b/xen/arch/arm/include/asm/domain.h
> index 9b3647587a..e5ae57cd09 100644
> --- a/xen/arch/arm/include/asm/domain.h
> +++ b/xen/arch/arm/include/asm/domain.h
> @@ -89,6 +89,8 @@ struct arch_domain
>   #ifdef CONFIG_TEE
>       void *tee;
>   #endif
> +    /* Is this guest a dom0less domain? */
> +    bool is_dom0less;
>   }  __cacheline_aligned;
>   
>   struct arch_vcpu
> diff --git a/xen/common/kernel.c b/xen/common/kernel.c
> index e119e5401f..c00ea67e5f 100644
> --- a/xen/common/kernel.c
> +++ b/xen/common/kernel.c
> @@ -550,6 +550,8 @@ DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>               if ( is_hardware_domain(d) )
>                   fi.submap |= 1U << XENFEAT_dom0;
>   #ifdef CONFIG_ARM
> +            if ( d->arch.is_dom0less )
> +                fi.submap |= (1U << XENFEAT_xenstore_late_init);
>               fi.submap |= (1U << XENFEAT_ARM_SMCCC_supported);
>   #endif
>   #ifdef CONFIG_X86
> diff --git a/xen/include/public/features.h b/xen/include/public/features.h
> index 9ee2f760ef..18f32b1a98 100644
> --- a/xen/include/public/features.h
> +++ b/xen/include/public/features.h
> @@ -128,6 +128,12 @@
>   #define XENFEAT_not_direct_mapped         16
>   #define XENFEAT_direct_mapped             17
>   
> +/*
> + * The xenstore interface should be initialized only after receiving a
> + * xenstore event channel notification.
> + */
> +#define XENFEAT_xenstore_late_init 18

You are assuming that there will be no event until Xenstored has 
discovered the domain. If I am not mistaken, this works because when you 
allocate an unbound port, we will not raise the event.

But I am not sure this is a guarantee for the event channel ABI. For 
instance, when using bind interdomain an event will be raised on the 
local port.

Looking at the Xenstore interface, there are a field connection. Could 
we use it (maybe a flag) to tell when the connection was fully initiated?

> +
>   #define XENFEAT_NR_SUBMAPS 1
>   
>   #endif /* __XEN_PUBLIC_FEATURES_H__ */

Cheers,

[1] <alpine.DEB.2.22.394.2112131729100.3376@ubuntu-linux-20-04-desktop>

-- 
Julien Grall

Re: [XEN PATCH 3/7] tools: add a late_init argument to xs_introduce_domain

[Julien Grall]

Hi,

On 08/01/2022 00:49, Stefano Stabellini wrote:
> From: Luca Miccio <lucmiccio@gmail.com>
> 
> Add a late_init argument to xs_introduce_domain to handle dom0less
> guests whose xenstore interfaces are initialized after boot.
> 
> This patch mechanically adds the new parameter; it doesn't change
> behaviors.
> 
> Signed-off-by: Luca Miccio <lucmiccio@gmail.com>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
> CC: wl@xen.org
> CC: anthony.perard@citrix.com
> CC: jgross@suse.com
> CC: marmarek@invisiblethingslab.com
> ---
>   tools/include/xenstore.h          | 3 ++-
>   tools/libs/light/libxl_dom.c      | 3 ++-
>   tools/libs/store/xs.c             | 8 ++++++--
>   tools/python/xen/lowlevel/xs/xs.c | 2 +-
>   4 files changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/include/xenstore.h b/tools/include/xenstore.h
> index 2b3f69fb61..1a302b5ff9 100644
> --- a/tools/include/xenstore.h
> +++ b/tools/include/xenstore.h
> @@ -226,7 +226,8 @@ bool xs_transaction_end(struct xs_handle *h, xs_transaction_t t,
>   bool xs_introduce_domain(struct xs_handle *h,
>   			 unsigned int domid,
>   			 unsigned long mfn,
> -                         unsigned int eventchn);
> +			 unsigned int eventchn,
> +			 bool late_init);
>   
>   /* Set the target of a domain
>    * This tells the store daemon that a domain is targetting another one, so
> diff --git a/tools/libs/light/libxl_dom.c b/tools/libs/light/libxl_dom.c
> index 2abaab439c..bacfdfa9df 100644
> --- a/tools/libs/light/libxl_dom.c
> +++ b/tools/libs/light/libxl_dom.c
> @@ -497,7 +497,8 @@ retry_transaction:
>       if (!xs_transaction_end(ctx->xsh, t, 0))
>           if (errno == EAGAIN)
>               goto retry_transaction;
> -    xs_introduce_domain(ctx->xsh, domid, state->store_mfn, state->store_port);
> +    xs_introduce_domain(ctx->xsh, domid, state->store_mfn, state->store_port,
> +                        false);
>       free(vm_path);
>       return 0;
>   }
> diff --git a/tools/libs/store/xs.c b/tools/libs/store/xs.c
> index 7a9a8b1656..dd47d607fd 100644
> --- a/tools/libs/store/xs.c
> +++ b/tools/libs/store/xs.c
> @@ -1089,16 +1089,18 @@ bool xs_transaction_end(struct xs_handle *h, xs_transaction_t t,
>    */
>   bool xs_introduce_domain(struct xs_handle *h,
>   			 unsigned int domid, unsigned long mfn,
> -			 unsigned int eventchn)
> +			 unsigned int eventchn, bool late_init)

The interface of libstore is meant to be stable/versioned. So I don't 
think you can change xs_introduce_domain().

>   {
>   	char domid_str[MAX_STRLEN(domid)];
>   	char mfn_str[MAX_STRLEN(mfn)];
>   	char eventchn_str[MAX_STRLEN(eventchn)];
> -	struct iovec iov[3];
> +	char late_init_str[MAX_STRLEN(late_init)];
> +	struct iovec iov[4];
>   
>   	snprintf(domid_str, sizeof(domid_str), "%u", domid);
>   	snprintf(mfn_str, sizeof(mfn_str), "%lu", mfn);
>   	snprintf(eventchn_str, sizeof(eventchn_str), "%u", eventchn);
> +	snprintf(late_init_str, sizeof(late_init_str), "%u", late_init);
>   
>   	iov[0].iov_base = domid_str;
>   	iov[0].iov_len = strlen(domid_str) + 1;
> @@ -1106,6 +1108,8 @@ bool xs_introduce_domain(struct xs_handle *h,
>   	iov[1].iov_len = strlen(mfn_str) + 1;
>   	iov[2].iov_base = eventchn_str;
>   	iov[2].iov_len = strlen(eventchn_str) + 1;
> +	iov[3].iov_base = late_init_str;
> +	iov[3].iov_len = strlen(late_init_str) + 1;
>   
>   	return xs_bool(xs_talkv(h, XBT_NULL, XS_INTRODUCE, iov,
>   				ARRAY_SIZE(iov), NULL));
> diff --git a/tools/python/xen/lowlevel/xs/xs.c b/tools/python/xen/lowlevel/xs/xs.c
> index 0dad7fa5f2..dc667fb231 100644
> --- a/tools/python/xen/lowlevel/xs/xs.c
> +++ b/tools/python/xen/lowlevel/xs/xs.c
> @@ -678,7 +678,7 @@ static PyObject *xspy_introduce_domain(XsHandle *self, PyObject *args)
>           return NULL;
>   
>       Py_BEGIN_ALLOW_THREADS
> -    result = xs_introduce_domain(xh, dom, page, port);
> +    result = xs_introduce_domain(xh, dom, page, port, false);
>       Py_END_ALLOW_THREADS
>   
>       return none(result);

Cheers,

-- 
Julien Grall

Re: [XEN PATCH 6/7] xenstored: do_introduce: handle the late_init case

[Julien Grall]

Hi Stefano,

On 08/01/2022 00:49, Stefano Stabellini wrote:
> From: Luca Miccio <lucmiccio@gmail.com>
> 
> If the function is called with late_init set then also notify the domain
> using the xenstore event channel.
> 
> Signed-off-by: Luca Miccio <lucmiccio@gmail.com>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
> CC: wl@xen.org
> CC: Anthony PERARD <anthony.perard@citrix.com>
> CC: Juergen Gross <jgross@suse.com>
> CC: julien@xen.org
> ---
>   tools/xenstore/xenstored_domain.c | 15 ++++++++++-----

All the changes to the protocol should be reflected in 
docs/misc/xenstore.txt. However...

>   1 file changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/xenstore/xenstored_domain.c b/tools/xenstore/xenstored_domain.c
> index d03c7d93a9..17b8021ca8 100644
> --- a/tools/xenstore/xenstored_domain.c
> +++ b/tools/xenstore/xenstored_domain.c
> @@ -429,7 +429,7 @@ static void domain_conn_reset(struct domain *domain)
>   
>   static struct domain *introduce_domain(const void *ctx,
>   				       unsigned int domid,
> -				       evtchn_port_t port, bool restore)
> +				       evtchn_port_t port, bool restore, bool late_init)
>   {
>   	struct domain *domain;
>   	int rc;
> @@ -461,6 +461,9 @@ static struct domain *introduce_domain(const void *ctx,
>   		/* Now domain belongs to its connection. */
>   		talloc_steal(domain->conn, domain);
>   
> +		if (late_init)
> +			xenevtchn_notify(xce_handle, domain->port);

... I am not convinced the parameter late_init is necessary. I believe 
it would be safe to always raise an event channel because a domain 
should be resilient (event channel are just to say "Please check the 
status", there are no data carried).

If you really need late_init, then it should be made optional to avoid 
breaking existing user of Xenstore (IHMO the protocol is stable and 
should be backward compatible).

Cheers,

-- 
Julien Grall

Re: [XEN PATCH 7/7] tools: add example application to initialize dom0less PV drivers

[Julien Grall]

Hi Stefano,

On 08/01/2022 00:49, Stefano Stabellini wrote:
> From: Luca Miccio <lucmiccio@gmail.com>
> 
> Add an example application that can be run in dom0 to complete the
> dom0less domains initialization so that they can get access to xenstore
> and use PV drivers.
> 
> Signed-off-by: Luca Miccio <lucmiccio@gmail.com>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
> CC: Wei Liu <wl@xen.org>
> CC: Anthony PERARD <anthony.perard@citrix.com>
> CC: Juergen Gross <jgross@suse.com>
> ---
>   tools/helpers/Makefile        |  13 ++
>   tools/helpers/init-dom0less.c | 263 ++++++++++++++++++++++++++++++++++
>   2 files changed, 276 insertions(+)
>   create mode 100644 tools/helpers/init-dom0less.c
> 
> diff --git a/tools/helpers/Makefile b/tools/helpers/Makefile
> index 7f6c422440..8e42997052 100644
> --- a/tools/helpers/Makefile
> +++ b/tools/helpers/Makefile
> @@ -10,6 +10,9 @@ ifeq ($(CONFIG_Linux),y)
>   ifeq ($(CONFIG_X86),y)
>   PROGS += init-xenstore-domain
>   endif
> +ifeq ($(CONFIG_ARM),y)
> +PROGS += init-dom0less
> +endif
>   endif
>   
>   XEN_INIT_DOM0_OBJS = xen-init-dom0.o init-dom-json.o
> @@ -26,6 +29,13 @@ $(INIT_XENSTORE_DOMAIN_OBJS): CFLAGS += $(CFLAGS_libxenstore)
>   $(INIT_XENSTORE_DOMAIN_OBJS): CFLAGS += $(CFLAGS_libxenlight)
>   $(INIT_XENSTORE_DOMAIN_OBJS): CFLAGS += -include $(XEN_ROOT)/tools/config.h
>   
> +INIT_DOM0LESS_OBJS = init-dom0less.o init-dom-json.o
> +$(INIT_DOM0LESS_OBJS): CFLAGS += $(CFLAGS_libxentoollog)
> +$(INIT_DOM0LESS_OBJS): CFLAGS += $(CFLAGS_libxenstore)
> +$(INIT_DOM0LESS_OBJS): CFLAGS += $(CFLAGS_libxenlight)
> +$(INIT_DOM0LESS_OBJS): CFLAGS += $(CFLAGS_libxenctrl)
> +$(INIT_DOM0LESS_OBJS): CFLAGS += $(CFLAGS_libxenevtchn)
> +
>   .PHONY: all
>   all: $(PROGS)
>   
> @@ -35,6 +45,9 @@ xen-init-dom0: $(XEN_INIT_DOM0_OBJS)
>   init-xenstore-domain: $(INIT_XENSTORE_DOMAIN_OBJS)
>   	$(CC) $(LDFLAGS) -o $@ $(INIT_XENSTORE_DOMAIN_OBJS) $(LDLIBS_libxentoollog) $(LDLIBS_libxenstore) $(LDLIBS_libxenctrl) $(LDLIBS_libxenguest) $(LDLIBS_libxenlight) $(APPEND_LDFLAGS)
>   
> +init-dom0less: $(INIT_DOM0LESS_OBJS)
> +	$(CC) $(LDFLAGS) -o $@ $(INIT_DOM0LESS_OBJS) $(LDLIBS_libxenctrl) $(LDLIBS_libxenevtchn) $(LDLIBS_libxentoollog) $(LDLIBS_libxenstore) $(LDLIBS_libxenlight) $(LDLIBS_libxenguest)  $(APPEND_LDFLAGS)
> +
>   .PHONY: install
>   install: all
>   	$(INSTALL_DIR) $(DESTDIR)$(LIBEXEC_BIN)
> diff --git a/tools/helpers/init-dom0less.c b/tools/helpers/init-dom0less.c
> new file mode 100644
> index 0000000000..055bf76cf5
> --- /dev/null
> +++ b/tools/helpers/init-dom0less.c
> @@ -0,0 +1,263 @@
> +#include <stdbool.h>
> +#include <syslog.h>
> +#include <stdio.h>
> +#include <err.h>
> +#include <stdlib.h>
> +#include <xenstore.h>
> +#include <xenctrl.h>
> +#include <xenguest.h>
> +#include <libxl.h>
> +#include <xenevtchn.h>
> +
> +#include "init-dom-json.h"
> +
> +#define NR_MAGIC_PAGES 4
> +#define CONSOLE_PFN_OFFSET 0
> +#define XENSTORE_PFN_OFFSET 1
> +#define STR_MAX_LENGTH 64
> +
> +static int alloc_magic_pages(struct xc_dom_image *dom)
> +{
> +    int rc, i;
> +    const xen_pfn_t base = GUEST_MAGIC_BASE >> XC_PAGE_SHIFT;
> +    xen_pfn_t p2m[NR_MAGIC_PAGES];
> +
> +    for (i = 0; i < NR_MAGIC_PAGES; i++)
> +        p2m[i] = base + i;
> +
> +    rc = xc_domain_populate_physmap_exact(dom->xch, dom->guest_domid,
> +                                          NR_MAGIC_PAGES, 0, 0, p2m);
> +    if (rc < 0)
> +        return rc;
> +
> +    dom->xenstore_pfn = base + XENSTORE_PFN_OFFSET;
> +
> +    xc_clear_domain_page(dom->xch, dom->guest_domid, dom->xenstore_pfn);
> +
> +    xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_STORE_PFN,
> +                     dom->xenstore_pfn);

I think it would be best if the page is initialized in Xen. This would 
allow to use the fields in the interface to propage the connection state 
(see my comment in patch #1).

> +    xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_STORE_EVTCHN,
> +                     dom->xenstore_evtchn);

On patch #1, you wrote the domain will have to wait on the event 
channel. So shouldn't the event channel be initialized before the domain 
is created? Otherwise, how would the domain knows when it is set?

> +    return 0;
> +}
> +
> +static void do_xs_write(struct xs_handle *xsh, xs_transaction_t t,
> +                        char *path, char *val)
> +{
> +    if (!xs_write(xsh, t, path, val, strlen(val)))
> +        fprintf(stderr, "writing %s to xenstore failed.\n", path);
> +}
> +
> +static void do_xs_write_dom(struct xs_handle *xsh, xs_transaction_t t,
> +                            domid_t domid, char *path, char *val)
> +{
> +    char full_path[STR_MAX_LENGTH];
> +
> +    snprintf(full_path, STR_MAX_LENGTH,
> +             "/local/domain/%d/%s", domid, path);
> +    do_xs_write(xsh, t, full_path, val);
> +}
> +
> +static void do_xs_write_libxl(struct xs_handle *xsh, xs_transaction_t t,
> +                              domid_t domid, char *path, char *val)
> +{
> +    char full_path[STR_MAX_LENGTH];
> +
> +    snprintf(full_path, STR_MAX_LENGTH,
> +             "/libxl/%d/%s", domid, path);
> +    do_xs_write(xsh, t, full_path, val);
> +}
> +
> +static void do_xs_write_vm(struct xs_handle *xsh, xs_transaction_t t,
> +                           libxl_uuid uuid, char *path, char *val)
> +{
> +    char full_path[STR_MAX_LENGTH];
> +
> +    snprintf(full_path, STR_MAX_LENGTH,
> +             "/vm/" LIBXL_UUID_FMT "/%s", LIBXL_UUID_BYTES(uuid), path);
> +    do_xs_write(xsh, t, full_path, val);
> +}
> +
> +static int restore_xenstore(struct xs_handle *xsh,

I think "restore" is misleading because the domain was never in 
Xenstore. So how about "create"?

> +                            libxl_dominfo *info, libxl_uuid uuid,
> +                            evtchn_port_t xenstore_port)
> +{
> +    domid_t domid;
> +    int i;
> +    char uuid_str[STR_MAX_LENGTH];
> +    char dom_name_str[STR_MAX_LENGTH];
> +    char vm_val_str[STR_MAX_LENGTH];
> +    char id_str[STR_MAX_LENGTH];
> +    char max_memkb_str[STR_MAX_LENGTH];
> +    char cpu_str[STR_MAX_LENGTH];
> +    char xenstore_port_str[STR_MAX_LENGTH];
> +    char ring_ref_str[STR_MAX_LENGTH];
> +    xs_transaction_t t;
> +
> +    domid = info->domid;
> +    snprintf(id_str, STR_MAX_LENGTH, "%d", domid);
> +    snprintf(dom_name_str, STR_MAX_LENGTH, "dom0less-%d", domid);
> +    snprintf(uuid_str, STR_MAX_LENGTH, LIBXL_UUID_FMT, LIBXL_UUID_BYTES(uuid));
> +    snprintf(vm_val_str, STR_MAX_LENGTH,
> +             "vm/" LIBXL_UUID_FMT, LIBXL_UUID_BYTES(uuid));
> +    snprintf(max_memkb_str, STR_MAX_LENGTH, "%lu", info->max_memkb);
> +    snprintf(ring_ref_str, STR_MAX_LENGTH, "%lld",
> +             (GUEST_MAGIC_BASE >> XC_PAGE_SHIFT) + XENSTORE_PFN_OFFSET);
> +    snprintf(xenstore_port_str, STR_MAX_LENGTH, "%d", xenstore_port);
> +
> +retry_transaction:
> +    t = xs_transaction_start(xsh);
> +    if (t == XBT_NULL)
> +        return errno;
> +
> +    /* /vm */
> +    do_xs_write_vm(xsh, t, uuid, "name", dom_name_str);
> +    do_xs_write_vm(xsh, t, uuid, "uuid", uuid_str);
> +    do_xs_write_vm(xsh, t, uuid, "start_time", "0");
> +
> +    /* /domain */
> +    do_xs_write_dom(xsh, t, domid, "vm", vm_val_str);
> +    do_xs_write_dom(xsh, t, domid, "name", dom_name_str);
> +    do_xs_write_dom(xsh, t, domid, "cpu", "");
> +    for (i = 0; i < info->vcpu_max_id; i++) {
> +        snprintf(cpu_str, STR_MAX_LENGTH, "cpu/%d/availability/", i);
> +        do_xs_write_dom(xsh, t, domid, cpu_str,
> +                        (info->cpupool & (1 << i)) ? "online" : "offline");
> +    }
> +    do_xs_write_dom(xsh, t, domid, "cpu/0", "");
> +    do_xs_write_dom(xsh, t, domid, "cpu/availability", "online");
> +
> +    do_xs_write_dom(xsh, t, domid, "memory", "");
> +    do_xs_write_dom(xsh, t, domid, "memory/static-max", max_memkb_str);
> +    do_xs_write_dom(xsh, t, domid, "memory/videoram", "-1");
> +
> +    do_xs_write_dom(xsh, t, domid, "device", "");
> +    do_xs_write_dom(xsh, t, domid, "device/suspend", "");
> +    do_xs_write_dom(xsh, t, domid, "device/suspend/event-channel", "");
> +
> +    do_xs_write_dom(xsh, t, domid, "control", "");
> +    do_xs_write_dom(xsh, t, domid, "control/shutdown", "");
> +    do_xs_write_dom(xsh, t, domid, "control/feature-poweroff", "1");
> +    do_xs_write_dom(xsh, t, domid, "control/feature-reboot", "1");
> +    do_xs_write_dom(xsh, t, domid, "control/feature-suspend", "");
> +    do_xs_write_dom(xsh, t, domid, "control/sysrq", "");
> +    do_xs_write_dom(xsh, t, domid, "control/platform-feature-multiprocessor-suspend", "1");
> +    do_xs_write_dom(xsh, t, domid, "control", "platform-feature-xs_reset_watches");
> +
> +    do_xs_write_dom(xsh, t, domid, "domid", id_str);
> +    do_xs_write_dom(xsh, t, domid, "data", "");
> +    do_xs_write_dom(xsh, t, domid, "drivers", "");
> +    do_xs_write_dom(xsh, t, domid, "feature", "");
> +    do_xs_write_dom(xsh, t, domid, "attr", "");
> +
> +    do_xs_write_dom(xsh, t, domid, "store/port", xenstore_port_str);
> +    do_xs_write_dom(xsh, t, domid, "store/ring-ref", ring_ref_str);
> +
> +    do_xs_write_libxl(xsh, t, domid, "type", "pvh");
> +    do_xs_write_libxl(xsh, t, domid, "dm-version", "qemu_xen");

Can you outline how you decided which nodes need to be created?

Cheers,

-- 
Julien Grall

Re: [XEN PATCH 1/7] xen: introduce XENFEAT_xenstore_late_init

[Jan Beulich]

On 08.01.2022 01:49, Stefano Stabellini wrote:
> Introduce a new feature flag to signal that xenstore will not be
> immediately available at boot time. Instead, xenstore will become
> available later, and a notification of xenstore readiness will be
> signalled to the guest using the xenstore event channel.

In addition to what Julien has said, I'd like to point out that Linux'es
xenbus driver already has means to deal with xenstored not being around
right away (perhaps because of living in a stubdom which starts in
parallel). I therefore wonder whether what you want can't be achieved
entirely inside that driver, without any new feature flag.

Jan

Re: [XEN PATCH 2/7] xen: introduce _evtchn_alloc_unbound

[Jan Beulich]

On 08.01.2022 01:49, Stefano Stabellini wrote:
> @@ -284,11 +285,32 @@ void evtchn_free(struct domain *d, struct evtchn *chn)
>      xsm_evtchn_close_post(chn);
>  }
>  
> -static int evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc)
> +struct evtchn *_evtchn_alloc_unbound(struct domain *d, domid_t remote_dom)

Function names want to be the other way around, to be in line with
naming rules of the C spec: The static function may be underscore-
prefixed, while the non-static one may not.

>  {
>      struct evtchn *chn;
> +    int port;
> +
> +    if ( (port = get_free_port(d)) < 0 )
> +        return ERR_PTR(port);
> +    chn = evtchn_from_port(d, port);
> +
> +    evtchn_write_lock(chn);
> +
> +    chn->state = ECS_UNBOUND;
> +    if ( (chn->u.unbound.remote_domid = remote_dom) == DOMID_SELF )
> +        chn->u.unbound.remote_domid = current->domain->domain_id;

I think the resolving of DOMID_SELF should remain in the caller, as I'm
pretty sure your planned new user(s) can't sensibly pass that value.

> +    evtchn_port_init(d, chn);
> +
> +    evtchn_write_unlock(chn);
> +
> +    return chn;
> +}
> +
> +static int evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc)
> +{
> +    struct evtchn *chn = NULL;

I don't think the initializer is needed.

> @@ -297,27 +319,22 @@ static int evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc)
>  
>      spin_lock(&d->event_lock);
>  
> -    if ( (port = get_free_port(d)) < 0 )
> -        ERROR_EXIT_DOM(port, d);
> -    chn = evtchn_from_port(d, port);
> +    chn = _evtchn_alloc_unbound(d, alloc->remote_dom);
> +    if ( IS_ERR(chn) )
> +    {
> +        rc = PTR_ERR(chn);
> +        ERROR_EXIT_DOM(rc, d);
> +    }
>  
>      rc = xsm_evtchn_unbound(XSM_TARGET, d, chn, alloc->remote_dom);
>      if ( rc )
>          goto out;
>  
> -    evtchn_write_lock(chn);
> -
> -    chn->state = ECS_UNBOUND;

This cannot be pulled ahead of the XSM check (or in general anything
potentially resulting in an error), as check_free_port() relies on
->state remaining ECS_FREE until it is known that the calling function
can't fail anymore.

> -    if ( (chn->u.unbound.remote_domid = alloc->remote_dom) == DOMID_SELF )
> -        chn->u.unbound.remote_domid = current->domain->domain_id;
> -    evtchn_port_init(d, chn);
> -
> -    evtchn_write_unlock(chn);
> -
> -    alloc->port = port;
> +    alloc->port = chn->port;
>  
>   out:
> -    check_free_port(d, port);
> +    if ( chn != NULL )
> +        check_free_port(d, chn->port);

Without the initializer above it'll then be more obvious that the
condition here needs to be !IS_ERR(chn).

Also (nit) please prefer the shorter "if ( chn )".

Overall I wonder in how far it would be possible to instead re-use PV
shim's "backdoor" into port allocation: evtchn_allocate_port() was
specifically made available for it, iirc.

Jan

Re: [XEN PATCH 6/7] xenstored: do_introduce: handle the late_init case

[Stefano Stabellini]

On Sat, 8 Jan 2022, Julien Grall wrote:
> Hi Stefano,
> 
> On 08/01/2022 00:49, Stefano Stabellini wrote:
> > From: Luca Miccio <lucmiccio@gmail.com>
> > 
> > If the function is called with late_init set then also notify the domain
> > using the xenstore event channel.
> > 
> > Signed-off-by: Luca Miccio <lucmiccio@gmail.com>
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
> > CC: wl@xen.org
> > CC: Anthony PERARD <anthony.perard@citrix.com>
> > CC: Juergen Gross <jgross@suse.com>
> > CC: julien@xen.org
> > ---
> >   tools/xenstore/xenstored_domain.c | 15 ++++++++++-----
> 
> All the changes to the protocol should be reflected in docs/misc/xenstore.txt.
> However...
> 
> >   1 file changed, 10 insertions(+), 5 deletions(-)
> > 
> > diff --git a/tools/xenstore/xenstored_domain.c
> > b/tools/xenstore/xenstored_domain.c
> > index d03c7d93a9..17b8021ca8 100644
> > --- a/tools/xenstore/xenstored_domain.c
> > +++ b/tools/xenstore/xenstored_domain.c
> > @@ -429,7 +429,7 @@ static void domain_conn_reset(struct domain *domain)
> >     static struct domain *introduce_domain(const void *ctx,
> >   				       unsigned int domid,
> > -				       evtchn_port_t port, bool restore)
> > +				       evtchn_port_t port, bool restore, bool
> > late_init)
> >   {
> >   	struct domain *domain;
> >   	int rc;
> > @@ -461,6 +461,9 @@ static struct domain *introduce_domain(const void *ctx,
> >   		/* Now domain belongs to its connection. */
> >   		talloc_steal(domain->conn, domain);
> >   +		if (late_init)
> > +			xenevtchn_notify(xce_handle, domain->port);
> 
> ... I am not convinced the parameter late_init is necessary. I believe it
> would be safe to always raise an event channel because a domain should be
> resilient (event channel are just to say "Please check the status", there are
> no data carried).

This is a fantastic idea. I gave it a quick try and it seems to work
fine. If everything checks out I'll make the change in the next version
and drop late_init (the new parameter to xs_introduce_domain) completely.


> If you really need late_init, then it should be made optional to avoid
> breaking existing user of Xenstore (IHMO the protocol is stable and should be
> backward compatible).

Re: [XEN PATCH 1/7] xen: introduce XENFEAT_xenstore_late_init

[Stefano Stabellini]

On Sat, 8 Jan 2022, Julien Grall wrote:
> On 08/01/2022 00:49, Stefano Stabellini wrote:
> > From: Luca Miccio <lucmiccio@gmail.com>
> > 
> > Introduce a new feature flag to signal that xenstore will not be
> > immediately available at boot time. Instead, xenstore will become
> > available later, and a notification of xenstore readiness will be
> > signalled to the guest using the xenstore event channel.
> 
> Hmmm... On the thread [1], you semmed to imply that new Linux version (I am
> assuming master) are ready to be used in dom0less with the node xen. So I am
> bit confused why this is necessary?

Today Linux/master can boot on Xen with this patch series applied and
with the hypervisor node in device tree. Linux boots fine but it is not
able to make use of the PV interfaces. During xenstore initialization,
Linux sees that HVM_PARAM_STORE_PFN has an invalid value, so it returns
error and continues without xenstore.

I have a patch for Linux that if XENFEAT_xenstore_late_init is present
makes Linux wait for an event notification before initializing xenstore:
https://marc.info/?l=xen-devel&m=164160299315589

So with v1 of the Xen and Linux patches series:
- Xen allocates the event channel at domain creation
- Linux boots, sees XENFEAT_xenstore_late_init and wait for an event
- init-dom0less later allocates the xenstore page
- init-dom0less triggers the xenstore event channel
- Linux receives the event and finishes the initialization, including
  mapping the xenstore page

With the Xen patches applies but no Linux patches, Linux would:
- try to initialize xenstore
- see an invalid HVM_PARAM_STORE_PFN and return error
- continue without xenstore



> > diff --git a/xen/include/public/features.h b/xen/include/public/features.h
> > index 9ee2f760ef..18f32b1a98 100644
> > --- a/xen/include/public/features.h
> > +++ b/xen/include/public/features.h
> > @@ -128,6 +128,12 @@
> >   #define XENFEAT_not_direct_mapped         16
> >   #define XENFEAT_direct_mapped             17
> >   +/*
> > + * The xenstore interface should be initialized only after receiving a
> > + * xenstore event channel notification.
> > + */
> > +#define XENFEAT_xenstore_late_init 18
> 
> You are assuming that there will be no event until Xenstored has discovered
> the domain. If I am not mistaken, this works because when you allocate an
> unbound port, we will not raise the event.
> 
> But I am not sure this is a guarantee for the event channel ABI. For instance,
> when using bind interdomain an event will be raised on the local port.
> 
> Looking at the Xenstore interface, there are a field connection. Could we use
> it (maybe a flag) to tell when the connection was fully initiated?

If we allocate HVM_PARAM_STORE_PFN directly from Xen, that would work
but the Linux xenbus driver will try to initialize the xenstore
interface immediately and it will get stuck in xenbus_thread. In my
tests wait_event_interruptible is the last thing that is called before
Linux getting stuck. Also note that functions like xb_init_comms looks
like they expect xenstored to be already up and running; xb_init_comms
is called unconditionally if the xenstore page and evtchn are
initialized successfully.

I liked your suggestion of adding a flag to struct
xenstore_domain_interface and I prototyped it. For instance, I added:

+#define XENSTORE_NOTREADY  2 /* xenstored not ready */

intf->connection is set to 2 by Xen at domain creation and later it is
set to 0 by init-dom0less.c to signal that the interface is now ready to
use. I think that would work fine but unfortunately it would break Linux
compatibility, because Linux/master of today doesn't know that it needs
to check for intf->connection to be 0 before continuing. It would get
stuck again because instead of waiting it would proceed with the
initialization.

Thus, I think we need to keep the allocation of HVM_PARAM_STORE_PFN
in init-dom0less.c not to break compatibility.

But we could get rid of XENFEAT_xenstore_late_init. The invalid value of
HVM_PARAM_STORE_PFN could be enough to tell Linux that it needs to
wait before it can continue with the initialization. There is no need
for XENFEAT_xenstore_late_init if we check that HVM_PARAM_STORE_EVTCHN
is valid but HVM_PARAM_STORE_PFN is zero.

If we do that, Linux/master keeps working (without PV drivers) because it
considers HVM_PARAM_STORE_PFN == 0 an error.

Linux with a new TBD patch would wait for an event notification and
check again HVM_PARAM_STORE_PFN when it receives the notification.

It is similar to what you suggested but instead of using a flag on the
Xenstore interface we would use the xen_param HVM_PARAM_STORE_PFN for
the same purpose. (FYI note that I'd be fine with using a flag on the
Xenstore shared interface page as well, but I cannot see how to make it
work without breaking compatibility with Linux/master.)

Re: [XEN PATCH 7/7] tools: add example application to initialize dom0less PV drivers

[Stefano Stabellini]

On Sat, 8 Jan 2022, Julien Grall wrote:
> On 08/01/2022 00:49, Stefano Stabellini wrote:
> > From: Luca Miccio <lucmiccio@gmail.com>
> > 
> > Add an example application that can be run in dom0 to complete the
> > dom0less domains initialization so that they can get access to xenstore
> > and use PV drivers.
> > 
> > Signed-off-by: Luca Miccio <lucmiccio@gmail.com>
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
> > CC: Wei Liu <wl@xen.org>
> > CC: Anthony PERARD <anthony.perard@citrix.com>
> > CC: Juergen Gross <jgross@suse.com>
> > ---
> >   tools/helpers/Makefile        |  13 ++
> >   tools/helpers/init-dom0less.c | 263 ++++++++++++++++++++++++++++++++++
> >   2 files changed, 276 insertions(+)
> >   create mode 100644 tools/helpers/init-dom0less.c
> > 
> > diff --git a/tools/helpers/Makefile b/tools/helpers/Makefile
> > index 7f6c422440..8e42997052 100644
> > --- a/tools/helpers/Makefile
> > +++ b/tools/helpers/Makefile
> > @@ -10,6 +10,9 @@ ifeq ($(CONFIG_Linux),y)
> >   ifeq ($(CONFIG_X86),y)
> >   PROGS += init-xenstore-domain
> >   endif
> > +ifeq ($(CONFIG_ARM),y)
> > +PROGS += init-dom0less
> > +endif
> >   endif
> >     XEN_INIT_DOM0_OBJS = xen-init-dom0.o init-dom-json.o
> > @@ -26,6 +29,13 @@ $(INIT_XENSTORE_DOMAIN_OBJS): CFLAGS +=
> > $(CFLAGS_libxenstore)
> >   $(INIT_XENSTORE_DOMAIN_OBJS): CFLAGS += $(CFLAGS_libxenlight)
> >   $(INIT_XENSTORE_DOMAIN_OBJS): CFLAGS += -include
> > $(XEN_ROOT)/tools/config.h
> >   +INIT_DOM0LESS_OBJS = init-dom0less.o init-dom-json.o
> > +$(INIT_DOM0LESS_OBJS): CFLAGS += $(CFLAGS_libxentoollog)
> > +$(INIT_DOM0LESS_OBJS): CFLAGS += $(CFLAGS_libxenstore)
> > +$(INIT_DOM0LESS_OBJS): CFLAGS += $(CFLAGS_libxenlight)
> > +$(INIT_DOM0LESS_OBJS): CFLAGS += $(CFLAGS_libxenctrl)
> > +$(INIT_DOM0LESS_OBJS): CFLAGS += $(CFLAGS_libxenevtchn)
> > +
> >   .PHONY: all
> >   all: $(PROGS)
> >   @@ -35,6 +45,9 @@ xen-init-dom0: $(XEN_INIT_DOM0_OBJS)
> >   init-xenstore-domain: $(INIT_XENSTORE_DOMAIN_OBJS)
> >   	$(CC) $(LDFLAGS) -o $@ $(INIT_XENSTORE_DOMAIN_OBJS)
> > $(LDLIBS_libxentoollog) $(LDLIBS_libxenstore) $(LDLIBS_libxenctrl)
> > $(LDLIBS_libxenguest) $(LDLIBS_libxenlight) $(APPEND_LDFLAGS)
> >   +init-dom0less: $(INIT_DOM0LESS_OBJS)
> > +	$(CC) $(LDFLAGS) -o $@ $(INIT_DOM0LESS_OBJS) $(LDLIBS_libxenctrl)
> > $(LDLIBS_libxenevtchn) $(LDLIBS_libxentoollog) $(LDLIBS_libxenstore)
> > $(LDLIBS_libxenlight) $(LDLIBS_libxenguest)  $(APPEND_LDFLAGS)
> > +
> >   .PHONY: install
> >   install: all
> >   	$(INSTALL_DIR) $(DESTDIR)$(LIBEXEC_BIN)
> > diff --git a/tools/helpers/init-dom0less.c b/tools/helpers/init-dom0less.c
> > new file mode 100644
> > index 0000000000..055bf76cf5
> > --- /dev/null
> > +++ b/tools/helpers/init-dom0less.c
> > @@ -0,0 +1,263 @@
> > +#include <stdbool.h>
> > +#include <syslog.h>
> > +#include <stdio.h>
> > +#include <err.h>
> > +#include <stdlib.h>
> > +#include <xenstore.h>
> > +#include <xenctrl.h>
> > +#include <xenguest.h>
> > +#include <libxl.h>
> > +#include <xenevtchn.h>
> > +
> > +#include "init-dom-json.h"
> > +
> > +#define NR_MAGIC_PAGES 4
> > +#define CONSOLE_PFN_OFFSET 0
> > +#define XENSTORE_PFN_OFFSET 1
> > +#define STR_MAX_LENGTH 64
> > +
> > +static int alloc_magic_pages(struct xc_dom_image *dom)
> > +{
> > +    int rc, i;
> > +    const xen_pfn_t base = GUEST_MAGIC_BASE >> XC_PAGE_SHIFT;
> > +    xen_pfn_t p2m[NR_MAGIC_PAGES];
> > +
> > +    for (i = 0; i < NR_MAGIC_PAGES; i++)
> > +        p2m[i] = base + i;
> > +
> > +    rc = xc_domain_populate_physmap_exact(dom->xch, dom->guest_domid,
> > +                                          NR_MAGIC_PAGES, 0, 0, p2m);
> > +    if (rc < 0)
> > +        return rc;
> > +
> > +    dom->xenstore_pfn = base + XENSTORE_PFN_OFFSET;
> > +
> > +    xc_clear_domain_page(dom->xch, dom->guest_domid, dom->xenstore_pfn);
> > +
> > +    xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_STORE_PFN,
> > +                     dom->xenstore_pfn);
> 
> I think it would be best if the page is initialized in Xen. This would allow
> to use the fields in the interface to propage the connection state (see my
> comment in patch #1).

Technically, it would work fine from a Xen point of view, but it would
cause problems to existing Linux kernels (see longer reply to patch #1.)


> > +    xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_STORE_EVTCHN,
> > +                     dom->xenstore_evtchn);
> 
> On patch #1, you wrote the domain will have to wait on the event channel. So
> shouldn't the event channel be initialized before the domain is created?
> Otherwise, how would the domain knows when it is set?

Yeah this is a mistake. HVM_PARAM_STORE_EVTCHN is already set by Xen at
domain creation (as you wrote) and there is no need to set it again here.


> > +    return 0;
> > +}
> > +
> > +static void do_xs_write(struct xs_handle *xsh, xs_transaction_t t,
> > +                        char *path, char *val)
> > +{
> > +    if (!xs_write(xsh, t, path, val, strlen(val)))
> > +        fprintf(stderr, "writing %s to xenstore failed.\n", path);
> > +}
> > +
> > +static void do_xs_write_dom(struct xs_handle *xsh, xs_transaction_t t,
> > +                            domid_t domid, char *path, char *val)
> > +{
> > +    char full_path[STR_MAX_LENGTH];
> > +
> > +    snprintf(full_path, STR_MAX_LENGTH,
> > +             "/local/domain/%d/%s", domid, path);
> > +    do_xs_write(xsh, t, full_path, val);
> > +}
> > +
> > +static void do_xs_write_libxl(struct xs_handle *xsh, xs_transaction_t t,
> > +                              domid_t domid, char *path, char *val)
> > +{
> > +    char full_path[STR_MAX_LENGTH];
> > +
> > +    snprintf(full_path, STR_MAX_LENGTH,
> > +             "/libxl/%d/%s", domid, path);
> > +    do_xs_write(xsh, t, full_path, val);
> > +}
> > +
> > +static void do_xs_write_vm(struct xs_handle *xsh, xs_transaction_t t,
> > +                           libxl_uuid uuid, char *path, char *val)
> > +{
> > +    char full_path[STR_MAX_LENGTH];
> > +
> > +    snprintf(full_path, STR_MAX_LENGTH,
> > +             "/vm/" LIBXL_UUID_FMT "/%s", LIBXL_UUID_BYTES(uuid), path);
> > +    do_xs_write(xsh, t, full_path, val);
> > +}
> > +
> > +static int restore_xenstore(struct xs_handle *xsh,
> 
> I think "restore" is misleading because the domain was never in Xenstore. So
> how about "create"?

Makes sense


> > +                            libxl_dominfo *info, libxl_uuid uuid,
> > +                            evtchn_port_t xenstore_port)
> > +{
> > +    domid_t domid;
> > +    int i;
> > +    char uuid_str[STR_MAX_LENGTH];
> > +    char dom_name_str[STR_MAX_LENGTH];
> > +    char vm_val_str[STR_MAX_LENGTH];
> > +    char id_str[STR_MAX_LENGTH];
> > +    char max_memkb_str[STR_MAX_LENGTH];
> > +    char cpu_str[STR_MAX_LENGTH];
> > +    char xenstore_port_str[STR_MAX_LENGTH];
> > +    char ring_ref_str[STR_MAX_LENGTH];
> > +    xs_transaction_t t;
> > +
> > +    domid = info->domid;
> > +    snprintf(id_str, STR_MAX_LENGTH, "%d", domid);
> > +    snprintf(dom_name_str, STR_MAX_LENGTH, "dom0less-%d", domid);
> > +    snprintf(uuid_str, STR_MAX_LENGTH, LIBXL_UUID_FMT,
> > LIBXL_UUID_BYTES(uuid));
> > +    snprintf(vm_val_str, STR_MAX_LENGTH,
> > +             "vm/" LIBXL_UUID_FMT, LIBXL_UUID_BYTES(uuid));
> > +    snprintf(max_memkb_str, STR_MAX_LENGTH, "%lu", info->max_memkb);
> > +    snprintf(ring_ref_str, STR_MAX_LENGTH, "%lld",
> > +             (GUEST_MAGIC_BASE >> XC_PAGE_SHIFT) + XENSTORE_PFN_OFFSET);
> > +    snprintf(xenstore_port_str, STR_MAX_LENGTH, "%d", xenstore_port);
> > +
> > +retry_transaction:
> > +    t = xs_transaction_start(xsh);
> > +    if (t == XBT_NULL)
> > +        return errno;
> > +
> > +    /* /vm */
> > +    do_xs_write_vm(xsh, t, uuid, "name", dom_name_str);
> > +    do_xs_write_vm(xsh, t, uuid, "uuid", uuid_str);
> > +    do_xs_write_vm(xsh, t, uuid, "start_time", "0");
> > +
> > +    /* /domain */
> > +    do_xs_write_dom(xsh, t, domid, "vm", vm_val_str);
> > +    do_xs_write_dom(xsh, t, domid, "name", dom_name_str);
> > +    do_xs_write_dom(xsh, t, domid, "cpu", "");
> > +    for (i = 0; i < info->vcpu_max_id; i++) {
> > +        snprintf(cpu_str, STR_MAX_LENGTH, "cpu/%d/availability/", i);
> > +        do_xs_write_dom(xsh, t, domid, cpu_str,
> > +                        (info->cpupool & (1 << i)) ? "online" : "offline");
> > +    }
> > +    do_xs_write_dom(xsh, t, domid, "cpu/0", "");
> > +    do_xs_write_dom(xsh, t, domid, "cpu/availability", "online");
> > +
> > +    do_xs_write_dom(xsh, t, domid, "memory", "");
> > +    do_xs_write_dom(xsh, t, domid, "memory/static-max", max_memkb_str);
> > +    do_xs_write_dom(xsh, t, domid, "memory/videoram", "-1");
> > +
> > +    do_xs_write_dom(xsh, t, domid, "device", "");
> > +    do_xs_write_dom(xsh, t, domid, "device/suspend", "");
> > +    do_xs_write_dom(xsh, t, domid, "device/suspend/event-channel", "");
> > +
> > +    do_xs_write_dom(xsh, t, domid, "control", "");
> > +    do_xs_write_dom(xsh, t, domid, "control/shutdown", "");
> > +    do_xs_write_dom(xsh, t, domid, "control/feature-poweroff", "1");
> > +    do_xs_write_dom(xsh, t, domid, "control/feature-reboot", "1");
> > +    do_xs_write_dom(xsh, t, domid, "control/feature-suspend", "");
> > +    do_xs_write_dom(xsh, t, domid, "control/sysrq", "");
> > +    do_xs_write_dom(xsh, t, domid,
> > "control/platform-feature-multiprocessor-suspend", "1");
> > +    do_xs_write_dom(xsh, t, domid, "control",
> > "platform-feature-xs_reset_watches");
> > +
> > +    do_xs_write_dom(xsh, t, domid, "domid", id_str);
> > +    do_xs_write_dom(xsh, t, domid, "data", "");
> > +    do_xs_write_dom(xsh, t, domid, "drivers", "");
> > +    do_xs_write_dom(xsh, t, domid, "feature", "");
> > +    do_xs_write_dom(xsh, t, domid, "attr", "");
> > +
> > +    do_xs_write_dom(xsh, t, domid, "store/port", xenstore_port_str);
> > +    do_xs_write_dom(xsh, t, domid, "store/ring-ref", ring_ref_str);
> > +
> > +    do_xs_write_libxl(xsh, t, domid, "type", "pvh");
> > +    do_xs_write_libxl(xsh, t, domid, "dm-version", "qemu_xen");
> 
> Can you outline how you decided which nodes need to be created?
 
We looked at all the parameters written by libxl/xl and attempted to
populate them here.

Re: [XEN PATCH 1/7] xen: introduce XENFEAT_xenstore_late_init

[Stefano Stabellini]

On Mon, 10 Jan 2022, Jan Beulich wrote:
> On 08.01.2022 01:49, Stefano Stabellini wrote:
> > Introduce a new feature flag to signal that xenstore will not be
> > immediately available at boot time. Instead, xenstore will become
> > available later, and a notification of xenstore readiness will be
> > signalled to the guest using the xenstore event channel.
> 
> In addition to what Julien has said, I'd like to point out that Linux'es
> xenbus driver already has means to deal with xenstored not being around
> right away (perhaps because of living in a stubdom which starts in
> parallel). I therefore wonder whether what you want can't be achieved
> entirely inside that driver, without any new feature flag.

The fewer changes to Linux the better but we couldn't find a way to make
it work with zero changes.

In a way, the patch for Linux is re-using the existing infrastructure to
delay initialization, e.g. xenbus_probe_thread to call xenbus_probe
later.

However, today there is nothing stopping Linux/HVM to continue
initialization immediately except for xs_hvm_defer_init_for_callback
which is not applicable in this case. So we introduced
XENFEAT_xenstore_late_init.

As I wrote in another reply, I think we could do without
XENFEAT_xenstore_late_init if we instead rely on checking for
HVM_PARAM_STORE_EVTCHN being valid and HVM_PARAM_STORE_PFN being zero.

But either way as far as I can tell we need to add a new check to delay
xenstore initialization in Linux/HVM.

Re: [XEN PATCH 4/7] xen: introduce xen,enhanced dom0less property

[Volodymyr Babchuk]

Hi Stefano,

Stefano Stabellini <sstabellini@kernel.org> writes:

> From: Stefano Stabellini <stefano.stabellini@xilinx.com>
>
> Introduce a new "xen,enhanced" dom0less property to enable/disable PV
> driver interfaces for dom0less guests. Currently only "enabled" and
> "disabled" are supported property values (and empty). Leave the option
> open to implement further possible values in the future (e.g.
> "xenstore" to enable only xenstore.)
>
> This patch only parses the property. Next patches will make use of it.
>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
> CC: Julien Grall <julien@xen.org>
> CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
> CC: Bertrand Marquis <bertrand.marquis@arm.com>
> ---
>  docs/misc/arm/device-tree/booting.txt | 18 ++++++++++++++++++
>  xen/arch/arm/domain_build.c           |  5 +++++
>  xen/arch/arm/include/asm/kernel.h     |  3 +++
>  3 files changed, 26 insertions(+)
>
> diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt
> index 71895663a4..38c29fb3d8 100644
> --- a/docs/misc/arm/device-tree/booting.txt
> +++ b/docs/misc/arm/device-tree/booting.txt
> @@ -169,6 +169,24 @@ with the following properties:
>      Please note that the SPI used for the virtual pl011 could clash with the
>      physical SPI of a physical device assigned to the guest.
>  
> +- xen,enhanced
> +
> +    A string property. Possible property values are:
> +
> +    - "enabled" (or missing property value)
> +    Xen PV interfaces, including grant-table and xenstore, will be
> +    enabled for the VM.
> +
> +    - "disabled"
> +    Xen PV interfaces are disabled.
> +
> +    If the xen,enhanced property is present with no value, it defaults
> +    to "enabled". If the xen,enhanced property is not present, PV
> +    interfaces are disabled.
> +
> +    In the future other possible property values might be added to
> +    enable only selected interfaces.
> +
>  - nr_spis
>  
>      Optional. A 32-bit integer specifying the number of SPIs (Shared
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 6931c022a2..96a94fa434 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -2963,6 +2963,7 @@ static int __init construct_domU(struct domain *d,
>                                   const struct dt_device_node *node)
>  {
>      struct kernel_info kinfo = {};
> +    const char *enhanced;
>      int rc;
>      u64 mem;
>  
> @@ -2978,6 +2979,10 @@ static int __init construct_domU(struct domain *d,
>  
>      kinfo.vpl011 = dt_property_read_bool(node, "vpl011");
>  
> +    rc = dt_property_read_string(node, "xen,enhanced", &enhanced);
> +    if ( rc == -EILSEQ || (rc == 0 && !strcmp(enhanced, "enabled")) )

Are you sure you need to check for -EILSEQ?

From documentation for dt_property_read_string:

 * Returns 0 on
 * success, -EINVAL if the property does not exist, -ENODATA if property
 * doest not have value, and -EILSEQ if the string is not
 * null-terminated with the length of the property data.


So, for me it looks like you need to check for -ENODATA.

> +        kinfo.enhanced = true;
> +
>      if ( vcpu_create(d, 0) == NULL )
>          return -ENOMEM;
>  
> diff --git a/xen/arch/arm/include/asm/kernel.h b/xen/arch/arm/include/asm/kernel.h
> index 874aa108a7..3275f7fbca 100644
> --- a/xen/arch/arm/include/asm/kernel.h
> +++ b/xen/arch/arm/include/asm/kernel.h
> @@ -36,6 +36,9 @@ struct kernel_info {
>      /* Enable pl011 emulation */
>      bool vpl011;
>  
> +    /* Enable PV drivers */
> +    bool enhanced;

Maybe dom0less_enhanced will be better name?
Or what about dom0? Should it have this option enabled?

> +
>      /* GIC phandle */
>      uint32_t phandle_gic;


-- 
Volodymyr Babchuk at EPAM

Re: [XEN PATCH 1/7] xen: introduce XENFEAT_xenstore_late_init

[Jan Beulich]

On 11.01.2022 00:08, Stefano Stabellini wrote:
> On Mon, 10 Jan 2022, Jan Beulich wrote:
>> On 08.01.2022 01:49, Stefano Stabellini wrote:
>>> Introduce a new feature flag to signal that xenstore will not be
>>> immediately available at boot time. Instead, xenstore will become
>>> available later, and a notification of xenstore readiness will be
>>> signalled to the guest using the xenstore event channel.
>>
>> In addition to what Julien has said, I'd like to point out that Linux'es
>> xenbus driver already has means to deal with xenstored not being around
>> right away (perhaps because of living in a stubdom which starts in
>> parallel). I therefore wonder whether what you want can't be achieved
>> entirely inside that driver, without any new feature flag.
> 
> The fewer changes to Linux the better but we couldn't find a way to make
> it work with zero changes.
> 
> In a way, the patch for Linux is re-using the existing infrastructure to
> delay initialization, e.g. xenbus_probe_thread to call xenbus_probe
> later.
> 
> However, today there is nothing stopping Linux/HVM to continue
> initialization immediately except for xs_hvm_defer_init_for_callback
> which is not applicable in this case. So we introduced
> XENFEAT_xenstore_late_init.
> 
> As I wrote in another reply, I think we could do without
> XENFEAT_xenstore_late_init if we instead rely on checking for
> HVM_PARAM_STORE_EVTCHN being valid and HVM_PARAM_STORE_PFN being zero.

Just as an aside - as discussed in some other context not so long ago,
HVM_PARAM_*_PFN being zero isn't the best way of expressing "not yet
initialized", and hence you would also want to check against ~0.

> But either way as far as I can tell we need to add a new check to delay
> xenstore initialization in Linux/HVM.

Yes, I can see that a Linux side change might be needed. But isolating
the change to there will be much better than needing to also have a
Xen side change in place.

Jan

Re: [XEN PATCH 1/7] xen: introduce XENFEAT_xenstore_late_init

[David Vrabel]

On 10/01/2022 22:55, Stefano Stabellini wrote:
> 
> I have a patch for Linux that if XENFEAT_xenstore_late_init is present
> makes Linux wait for an event notification before initializing xenstore:
> https://marc.info/?l=xen-devel&m=164160299315589
> 
> So with v1 of the Xen and Linux patches series:
> - Xen allocates the event channel at domain creation
> - Linux boots, sees XENFEAT_xenstore_late_init and wait for an event
> - init-dom0less later allocates the xenstore page
> - init-dom0less triggers the xenstore event channel
> - Linux receives the event and finishes the initialization, including
>    mapping the xenstore page

You can get this behaviour without the new feature.

- Xen allocates the event channel at domain creation
- Linux boots, sees HVM_PARAM_STORE_PFN is invalid and there's a 
xenstore event channel and waits for an event
- init-dom0less later allocates the xenstore page
- init-dom0less triggers the xenstore event channel
- Linux receives the event and finishes the initialization, including
    mapping the xenstore page-

David

Re: [XEN PATCH 2/7] xen: introduce _evtchn_alloc_unbound

[Stefano Stabellini]

On Mon, 10 Jan 2022, Jan Beulich wrote:
> On 08.01.2022 01:49, Stefano Stabellini wrote:
> > @@ -284,11 +285,32 @@ void evtchn_free(struct domain *d, struct evtchn *chn)
> >      xsm_evtchn_close_post(chn);
> >  }
> >  
> > -static int evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc)
> > +struct evtchn *_evtchn_alloc_unbound(struct domain *d, domid_t remote_dom)
> 
> Function names want to be the other way around, to be in line with
> naming rules of the C spec: The static function may be underscore-
> prefixed, while the non-static one may not.

OK


> >  {
> >      struct evtchn *chn;
> > +    int port;
> > +
> > +    if ( (port = get_free_port(d)) < 0 )
> > +        return ERR_PTR(port);
> > +    chn = evtchn_from_port(d, port);
> > +
> > +    evtchn_write_lock(chn);
> > +
> > +    chn->state = ECS_UNBOUND;
> > +    if ( (chn->u.unbound.remote_domid = remote_dom) == DOMID_SELF )
> > +        chn->u.unbound.remote_domid = current->domain->domain_id;
> 
> I think the resolving of DOMID_SELF should remain in the caller, as I'm
> pretty sure your planned new user(s) can't sensibly pass that value.

Yep, no problem


> > +    evtchn_port_init(d, chn);
> > +
> > +    evtchn_write_unlock(chn);
> > +
> > +    return chn;
> > +}
> > +
> > +static int evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc)
> > +{
> > +    struct evtchn *chn = NULL;
> 
> I don't think the initializer is needed.

OK


> > @@ -297,27 +319,22 @@ static int evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc)
> >  
> >      spin_lock(&d->event_lock);
> >  
> > -    if ( (port = get_free_port(d)) < 0 )
> > -        ERROR_EXIT_DOM(port, d);
> > -    chn = evtchn_from_port(d, port);
> > +    chn = _evtchn_alloc_unbound(d, alloc->remote_dom);
> > +    if ( IS_ERR(chn) )
> > +    {
> > +        rc = PTR_ERR(chn);
> > +        ERROR_EXIT_DOM(rc, d);
> > +    }
> >  
> >      rc = xsm_evtchn_unbound(XSM_TARGET, d, chn, alloc->remote_dom);
> >      if ( rc )
> >          goto out;
> >  
> > -    evtchn_write_lock(chn);
> > -
> > -    chn->state = ECS_UNBOUND;
> 
> This cannot be pulled ahead of the XSM check (or in general anything
> potentially resulting in an error), as check_free_port() relies on
> ->state remaining ECS_FREE until it is known that the calling function
> can't fail anymore.

OK, I didn't realize. Unfortunately it means we have to move setting
chn->state = ECS_UNBOUND to the caller.


> > -    if ( (chn->u.unbound.remote_domid = alloc->remote_dom) == DOMID_SELF )
> > -        chn->u.unbound.remote_domid = current->domain->domain_id;
> > -    evtchn_port_init(d, chn);
> > -
> > -    evtchn_write_unlock(chn);
> > -
> > -    alloc->port = port;
> > +    alloc->port = chn->port;
> >  
> >   out:
> > -    check_free_port(d, port);
> > +    if ( chn != NULL )
> > +        check_free_port(d, chn->port);
> 
> Without the initializer above it'll then be more obvious that the
> condition here needs to be !IS_ERR(chn).
> 
> Also (nit) please prefer the shorter "if ( chn )".
> 
> Overall I wonder in how far it would be possible to instead re-use PV
> shim's "backdoor" into port allocation: evtchn_allocate_port() was
> specifically made available for it, iirc.

I don't see an obvious way to do it. These are the 4 things we need to
do:

1) call get_free_port/evtchn_allocate_port
2) set state = ECS_UNBOUND
3) set remote_domid
4) call evtchn_port_init

It doesn't look like we could enhance evtchn_allocate_port to do 2) and
3). And probably even 4) couldn't be added to evtchn_allocate_port.

So basically it is like calling get_free_port() and do 2,3,4 ourselves
from the caller in xen/arch/arm/domain_build.c. But that might be a good
idea actually. Maybe we should leave evtchn_alloc_unbound unmodified and
instead open-code what we need to do in xen/arch/arm/domain_build.c.
This is how it would look like as a new function in
xen/arch/arm/domain_build.c:

static int alloc_xenstore_evtchn(struct domain *d)
{
    struct evtchn *chn;
    int port;

    if ( (port = get_free_port(d)) < 0 )
        return ERR_PTR(port);
    chn = evtchn_from_port(d, port);

    chn->state = ECS_UNBOUND;
    chn->u.unbound.remote_domid = hardware_domain->domain_id;
    evtchn_port_init(d, chn);

    return chn->port;
}

What do you think? It might not be worth introducing
evtchn_alloc_unbound / _evtchn_alloc_unbound for this?

I am happy to follow what you think is best.

Cheers,

Stefano

Re: [XEN PATCH 1/7] xen: introduce XENFEAT_xenstore_late_init

[Stefano Stabellini]

On Tue, 11 Jan 2022, Jan Beulich wrote:
> On 11.01.2022 00:08, Stefano Stabellini wrote:
> > On Mon, 10 Jan 2022, Jan Beulich wrote:
> >> On 08.01.2022 01:49, Stefano Stabellini wrote:
> >>> Introduce a new feature flag to signal that xenstore will not be
> >>> immediately available at boot time. Instead, xenstore will become
> >>> available later, and a notification of xenstore readiness will be
> >>> signalled to the guest using the xenstore event channel.
> >>
> >> In addition to what Julien has said, I'd like to point out that Linux'es
> >> xenbus driver already has means to deal with xenstored not being around
> >> right away (perhaps because of living in a stubdom which starts in
> >> parallel). I therefore wonder whether what you want can't be achieved
> >> entirely inside that driver, without any new feature flag.
> > 
> > The fewer changes to Linux the better but we couldn't find a way to make
> > it work with zero changes.
> > 
> > In a way, the patch for Linux is re-using the existing infrastructure to
> > delay initialization, e.g. xenbus_probe_thread to call xenbus_probe
> > later.
> > 
> > However, today there is nothing stopping Linux/HVM to continue
> > initialization immediately except for xs_hvm_defer_init_for_callback
> > which is not applicable in this case. So we introduced
> > XENFEAT_xenstore_late_init.
> > 
> > As I wrote in another reply, I think we could do without
> > XENFEAT_xenstore_late_init if we instead rely on checking for
> > HVM_PARAM_STORE_EVTCHN being valid and HVM_PARAM_STORE_PFN being zero.
> 
> Just as an aside - as discussed in some other context not so long ago,
> HVM_PARAM_*_PFN being zero isn't the best way of expressing "not yet
> initialized", and hence you would also want to check against ~0.
 
Yes, good point


> > But either way as far as I can tell we need to add a new check to delay
> > xenstore initialization in Linux/HVM.
> 
> Yes, I can see that a Linux side change might be needed. But isolating
> the change to there will be much better than needing to also have a
> Xen side change in place.

I agree. I managed to remove XENFEAT_xenstore_late_init from the patch
series and everything works fine. It will be in the next version.

Re: [XEN PATCH 1/7] xen: introduce XENFEAT_xenstore_late_init

[Stefano Stabellini]

On Tue, 11 Jan 2022, David Vrabel wrote:
> On 10/01/2022 22:55, Stefano Stabellini wrote:
> > 
> > I have a patch for Linux that if XENFEAT_xenstore_late_init is present
> > makes Linux wait for an event notification before initializing xenstore:
> > https://marc.info/?l=xen-devel&m=164160299315589
> > 
> > So with v1 of the Xen and Linux patches series:
> > - Xen allocates the event channel at domain creation
> > - Linux boots, sees XENFEAT_xenstore_late_init and wait for an event
> > - init-dom0less later allocates the xenstore page
> > - init-dom0less triggers the xenstore event channel
> > - Linux receives the event and finishes the initialization, including
> >    mapping the xenstore page
> 
> You can get this behaviour without the new feature.
> 
> - Xen allocates the event channel at domain creation
> - Linux boots, sees HVM_PARAM_STORE_PFN is invalid and there's a xenstore
> event channel and waits for an event
> - init-dom0less later allocates the xenstore page
> - init-dom0less triggers the xenstore event channel
> - Linux receives the event and finishes the initialization, including
>    mapping the xenstore page-

Hey David!

Yes, this is similar to what I had in mind and I managed to make it work
successfully.

Re: [XEN PATCH 4/7] xen: introduce xen,enhanced dom0less property

[Stefano Stabellini]

On Tue, 11 Jan 2022, Volodymyr Babchuk wrote:
> Hi Stefano,
> 
> Stefano Stabellini <sstabellini@kernel.org> writes:
> 
> > From: Stefano Stabellini <stefano.stabellini@xilinx.com>
> >
> > Introduce a new "xen,enhanced" dom0less property to enable/disable PV
> > driver interfaces for dom0less guests. Currently only "enabled" and
> > "disabled" are supported property values (and empty). Leave the option
> > open to implement further possible values in the future (e.g.
> > "xenstore" to enable only xenstore.)
> >
> > This patch only parses the property. Next patches will make use of it.
> >
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
> > CC: Julien Grall <julien@xen.org>
> > CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
> > CC: Bertrand Marquis <bertrand.marquis@arm.com>
> > ---
> >  docs/misc/arm/device-tree/booting.txt | 18 ++++++++++++++++++
> >  xen/arch/arm/domain_build.c           |  5 +++++
> >  xen/arch/arm/include/asm/kernel.h     |  3 +++
> >  3 files changed, 26 insertions(+)
> >
> > diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt
> > index 71895663a4..38c29fb3d8 100644
> > --- a/docs/misc/arm/device-tree/booting.txt
> > +++ b/docs/misc/arm/device-tree/booting.txt
> > @@ -169,6 +169,24 @@ with the following properties:
> >      Please note that the SPI used for the virtual pl011 could clash with the
> >      physical SPI of a physical device assigned to the guest.
> >  
> > +- xen,enhanced
> > +
> > +    A string property. Possible property values are:
> > +
> > +    - "enabled" (or missing property value)
> > +    Xen PV interfaces, including grant-table and xenstore, will be
> > +    enabled for the VM.
> > +
> > +    - "disabled"
> > +    Xen PV interfaces are disabled.
> > +
> > +    If the xen,enhanced property is present with no value, it defaults
> > +    to "enabled". If the xen,enhanced property is not present, PV
> > +    interfaces are disabled.
> > +
> > +    In the future other possible property values might be added to
> > +    enable only selected interfaces.
> > +
> >  - nr_spis
> >  
> >      Optional. A 32-bit integer specifying the number of SPIs (Shared
> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > index 6931c022a2..96a94fa434 100644
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -2963,6 +2963,7 @@ static int __init construct_domU(struct domain *d,
> >                                   const struct dt_device_node *node)
> >  {
> >      struct kernel_info kinfo = {};
> > +    const char *enhanced;
> >      int rc;
> >      u64 mem;
> >  
> > @@ -2978,6 +2979,10 @@ static int __init construct_domU(struct domain *d,
> >  
> >      kinfo.vpl011 = dt_property_read_bool(node, "vpl011");
> >  
> > +    rc = dt_property_read_string(node, "xen,enhanced", &enhanced);
> > +    if ( rc == -EILSEQ || (rc == 0 && !strcmp(enhanced, "enabled")) )
> 
> Are you sure you need to check for -EILSEQ?
> 
> >From documentation for dt_property_read_string:
> 
>  * Returns 0 on
>  * success, -EINVAL if the property does not exist, -ENODATA if property
>  * doest not have value, and -EILSEQ if the string is not
>  * null-terminated with the length of the property data.
> 
> So, for me it looks like you need to check for -ENODATA.

That's interesting, I ran a few tests with:

  fdt set /chosen/domU0 xen,enhanced

and I keep getting:

  (XEN) DEBUG construct_domU 3009 rc=-84 val=<NULL>

And -84 is -EILSEQ.

So I'll check for both -EILSEQ and -ENODATA to be safe.

 
> 
> > +        kinfo.enhanced = true;
> > +
> >      if ( vcpu_create(d, 0) == NULL )
> >          return -ENOMEM;
> >  
> > diff --git a/xen/arch/arm/include/asm/kernel.h b/xen/arch/arm/include/asm/kernel.h
> > index 874aa108a7..3275f7fbca 100644
> > --- a/xen/arch/arm/include/asm/kernel.h
> > +++ b/xen/arch/arm/include/asm/kernel.h
> > @@ -36,6 +36,9 @@ struct kernel_info {
> >      /* Enable pl011 emulation */
> >      bool vpl011;
> >  
> > +    /* Enable PV drivers */
> > +    bool enhanced;
> 
> Maybe dom0less_enhanced will be better name?

I am OK with that and maybe you are right, it is clearer. I'll keep the
device tree option as "xen,enhanced" unless you meant to also change
that too.


> Or what about dom0? Should it have this option enabled?

Yes, I think it makes sense to set it to true for dom0 too for
consistency.

Re: [XEN PATCH 2/7] xen: introduce _evtchn_alloc_unbound

[Jan Beulich]

On 11.01.2022 23:49, Stefano Stabellini wrote:
> On Mon, 10 Jan 2022, Jan Beulich wrote:
>> On 08.01.2022 01:49, Stefano Stabellini wrote:
>>> @@ -284,11 +285,32 @@ void evtchn_free(struct domain *d, struct evtchn *chn)
>>>      xsm_evtchn_close_post(chn);
>>>  }
>>>  
>>> -static int evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc)
>>> +struct evtchn *_evtchn_alloc_unbound(struct domain *d, domid_t remote_dom)
>>
>> Function names want to be the other way around, to be in line with
>> naming rules of the C spec: The static function may be underscore-
>> prefixed, while the non-static one may not.
> 
> OK
> 
> 
>>>  {
>>>      struct evtchn *chn;
>>> +    int port;
>>> +
>>> +    if ( (port = get_free_port(d)) < 0 )
>>> +        return ERR_PTR(port);
>>> +    chn = evtchn_from_port(d, port);
>>> +
>>> +    evtchn_write_lock(chn);
>>> +
>>> +    chn->state = ECS_UNBOUND;
>>> +    if ( (chn->u.unbound.remote_domid = remote_dom) == DOMID_SELF )
>>> +        chn->u.unbound.remote_domid = current->domain->domain_id;
>>
>> I think the resolving of DOMID_SELF should remain in the caller, as I'm
>> pretty sure your planned new user(s) can't sensibly pass that value.
> 
> Yep, no problem
> 
> 
>>> +    evtchn_port_init(d, chn);
>>> +
>>> +    evtchn_write_unlock(chn);
>>> +
>>> +    return chn;
>>> +}
>>> +
>>> +static int evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc)
>>> +{
>>> +    struct evtchn *chn = NULL;
>>
>> I don't think the initializer is needed.
> 
> OK
> 
> 
>>> @@ -297,27 +319,22 @@ static int evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc)
>>>  
>>>      spin_lock(&d->event_lock);
>>>  
>>> -    if ( (port = get_free_port(d)) < 0 )
>>> -        ERROR_EXIT_DOM(port, d);
>>> -    chn = evtchn_from_port(d, port);
>>> +    chn = _evtchn_alloc_unbound(d, alloc->remote_dom);
>>> +    if ( IS_ERR(chn) )
>>> +    {
>>> +        rc = PTR_ERR(chn);
>>> +        ERROR_EXIT_DOM(rc, d);
>>> +    }
>>>  
>>>      rc = xsm_evtchn_unbound(XSM_TARGET, d, chn, alloc->remote_dom);
>>>      if ( rc )
>>>          goto out;
>>>  
>>> -    evtchn_write_lock(chn);
>>> -
>>> -    chn->state = ECS_UNBOUND;
>>
>> This cannot be pulled ahead of the XSM check (or in general anything
>> potentially resulting in an error), as check_free_port() relies on
>> ->state remaining ECS_FREE until it is known that the calling function
>> can't fail anymore.
> 
> OK, I didn't realize. Unfortunately it means we have to move setting
> chn->state = ECS_UNBOUND to the caller.
> 
> 
>>> -    if ( (chn->u.unbound.remote_domid = alloc->remote_dom) == DOMID_SELF )
>>> -        chn->u.unbound.remote_domid = current->domain->domain_id;
>>> -    evtchn_port_init(d, chn);
>>> -
>>> -    evtchn_write_unlock(chn);
>>> -
>>> -    alloc->port = port;
>>> +    alloc->port = chn->port;
>>>  
>>>   out:
>>> -    check_free_port(d, port);
>>> +    if ( chn != NULL )
>>> +        check_free_port(d, chn->port);
>>
>> Without the initializer above it'll then be more obvious that the
>> condition here needs to be !IS_ERR(chn).
>>
>> Also (nit) please prefer the shorter "if ( chn )".
>>
>> Overall I wonder in how far it would be possible to instead re-use PV
>> shim's "backdoor" into port allocation: evtchn_allocate_port() was
>> specifically made available for it, iirc.
> 
> I don't see an obvious way to do it. These are the 4 things we need to
> do:
> 
> 1) call get_free_port/evtchn_allocate_port
> 2) set state = ECS_UNBOUND
> 3) set remote_domid
> 4) call evtchn_port_init
> 
> It doesn't look like we could enhance evtchn_allocate_port to do 2) and
> 3). And probably even 4) couldn't be added to evtchn_allocate_port.
> 
> So basically it is like calling get_free_port() and do 2,3,4 ourselves
> from the caller in xen/arch/arm/domain_build.c. But that might be a good
> idea actually. Maybe we should leave evtchn_alloc_unbound unmodified and
> instead open-code what we need to do in xen/arch/arm/domain_build.c.

Right, that's what I was trying to hint at as an alternative.

Jan

> This is how it would look like as a new function in
> xen/arch/arm/domain_build.c:
> 
> static int alloc_xenstore_evtchn(struct domain *d)
> {
>     struct evtchn *chn;
>     int port;
> 
>     if ( (port = get_free_port(d)) < 0 )
>         return ERR_PTR(port);
>     chn = evtchn_from_port(d, port);
> 
>     chn->state = ECS_UNBOUND;
>     chn->u.unbound.remote_domid = hardware_domain->domain_id;
>     evtchn_port_init(d, chn);
> 
>     return chn->port;
> }
> 
> What do you think? It might not be worth introducing
> evtchn_alloc_unbound / _evtchn_alloc_unbound for this?
> 
> I am happy to follow what you think is best.
> 
> Cheers,
> 
> Stefano
> 

Re: [XEN PATCH 2/7] xen: introduce _evtchn_alloc_unbound

[Stefano Stabellini]

On Wed, 12 Jan 2022, Jan Beulich wrote:
> On 11.01.2022 23:49, Stefano Stabellini wrote:
> > On Mon, 10 Jan 2022, Jan Beulich wrote:
> >> On 08.01.2022 01:49, Stefano Stabellini wrote:
> >>> @@ -284,11 +285,32 @@ void evtchn_free(struct domain *d, struct evtchn *chn)
> >>>      xsm_evtchn_close_post(chn);
> >>>  }
> >>>  
> >>> -static int evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc)
> >>> +struct evtchn *_evtchn_alloc_unbound(struct domain *d, domid_t remote_dom)
> >>
> >> Function names want to be the other way around, to be in line with
> >> naming rules of the C spec: The static function may be underscore-
> >> prefixed, while the non-static one may not.
> > 
> > OK
> > 
> > 
> >>>  {
> >>>      struct evtchn *chn;
> >>> +    int port;
> >>> +
> >>> +    if ( (port = get_free_port(d)) < 0 )
> >>> +        return ERR_PTR(port);
> >>> +    chn = evtchn_from_port(d, port);
> >>> +
> >>> +    evtchn_write_lock(chn);
> >>> +
> >>> +    chn->state = ECS_UNBOUND;
> >>> +    if ( (chn->u.unbound.remote_domid = remote_dom) == DOMID_SELF )
> >>> +        chn->u.unbound.remote_domid = current->domain->domain_id;
> >>
> >> I think the resolving of DOMID_SELF should remain in the caller, as I'm
> >> pretty sure your planned new user(s) can't sensibly pass that value.
> > 
> > Yep, no problem
> > 
> > 
> >>> +    evtchn_port_init(d, chn);
> >>> +
> >>> +    evtchn_write_unlock(chn);
> >>> +
> >>> +    return chn;
> >>> +}
> >>> +
> >>> +static int evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc)
> >>> +{
> >>> +    struct evtchn *chn = NULL;
> >>
> >> I don't think the initializer is needed.
> > 
> > OK
> > 
> > 
> >>> @@ -297,27 +319,22 @@ static int evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc)
> >>>  
> >>>      spin_lock(&d->event_lock);
> >>>  
> >>> -    if ( (port = get_free_port(d)) < 0 )
> >>> -        ERROR_EXIT_DOM(port, d);
> >>> -    chn = evtchn_from_port(d, port);
> >>> +    chn = _evtchn_alloc_unbound(d, alloc->remote_dom);
> >>> +    if ( IS_ERR(chn) )
> >>> +    {
> >>> +        rc = PTR_ERR(chn);
> >>> +        ERROR_EXIT_DOM(rc, d);
> >>> +    }
> >>>  
> >>>      rc = xsm_evtchn_unbound(XSM_TARGET, d, chn, alloc->remote_dom);
> >>>      if ( rc )
> >>>          goto out;
> >>>  
> >>> -    evtchn_write_lock(chn);
> >>> -
> >>> -    chn->state = ECS_UNBOUND;
> >>
> >> This cannot be pulled ahead of the XSM check (or in general anything
> >> potentially resulting in an error), as check_free_port() relies on
> >> ->state remaining ECS_FREE until it is known that the calling function
> >> can't fail anymore.
> > 
> > OK, I didn't realize. Unfortunately it means we have to move setting
> > chn->state = ECS_UNBOUND to the caller.
> > 
> > 
> >>> -    if ( (chn->u.unbound.remote_domid = alloc->remote_dom) == DOMID_SELF )
> >>> -        chn->u.unbound.remote_domid = current->domain->domain_id;
> >>> -    evtchn_port_init(d, chn);
> >>> -
> >>> -    evtchn_write_unlock(chn);
> >>> -
> >>> -    alloc->port = port;
> >>> +    alloc->port = chn->port;
> >>>  
> >>>   out:
> >>> -    check_free_port(d, port);
> >>> +    if ( chn != NULL )
> >>> +        check_free_port(d, chn->port);
> >>
> >> Without the initializer above it'll then be more obvious that the
> >> condition here needs to be !IS_ERR(chn).
> >>
> >> Also (nit) please prefer the shorter "if ( chn )".
> >>
> >> Overall I wonder in how far it would be possible to instead re-use PV
> >> shim's "backdoor" into port allocation: evtchn_allocate_port() was
> >> specifically made available for it, iirc.
> > 
> > I don't see an obvious way to do it. These are the 4 things we need to
> > do:
> > 
> > 1) call get_free_port/evtchn_allocate_port
> > 2) set state = ECS_UNBOUND
> > 3) set remote_domid
> > 4) call evtchn_port_init
> > 
> > It doesn't look like we could enhance evtchn_allocate_port to do 2) and
> > 3). And probably even 4) couldn't be added to evtchn_allocate_port.
> > 
> > So basically it is like calling get_free_port() and do 2,3,4 ourselves
> > from the caller in xen/arch/arm/domain_build.c. But that might be a good
> > idea actually. Maybe we should leave evtchn_alloc_unbound unmodified and
> > instead open-code what we need to do in xen/arch/arm/domain_build.c.
> 
> Right, that's what I was trying to hint at as an alternative.

OK, I'll do that then

Re: [XEN PATCH 3/7] tools: add a late_init argument to xs_introduce_domain

[Stefano Stabellini]

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

On Sat, 8 Jan 2022, Marek Marczykowski-Górecki wrote:
> On Fri, Jan 07, 2022 at 04:49:08PM -0800, Stefano Stabellini wrote:
> > From: Luca Miccio <lucmiccio@gmail.com>
> > 
> > Add a late_init argument to xs_introduce_domain to handle dom0less
> > guests whose xenstore interfaces are initialized after boot.
> > 
> > This patch mechanically adds the new parameter; it doesn't change
> > behaviors.
> > 
> > Signed-off-by: Luca Miccio <lucmiccio@gmail.com>
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
> 
> I realize there is not much sense in making the parameter usable in the
> Python API, since it's only useful for xenstored. So, for the Python part:
> 
> Acked-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>

Thanks Marek for the quick comeback and for the ack. This patch will get
dropped in the next version of the series.

Re: [XEN PATCH 6/7] xenstored: do_introduce: handle the late_init case

[Stefano Stabellini]

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

On Sat, 8 Jan 2022, Marek Marczykowski-Górecki wrote:
> On Fri, Jan 07, 2022 at 04:49:11PM -0800, Stefano Stabellini wrote:
> > From: Luca Miccio <lucmiccio@gmail.com>
> > 
> > If the function is called with late_init set then also notify the domain
> > using the xenstore event channel.
> > 
> > Signed-off-by: Luca Miccio <lucmiccio@gmail.com>
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
> > CC: wl@xen.org
> > CC: Anthony PERARD <anthony.perard@citrix.com>
> > CC: Juergen Gross <jgross@suse.com>
> > CC: julien@xen.org
> > ---
> >  tools/xenstore/xenstored_domain.c | 15 ++++++++++-----
> 
> Isn't the same necessary in oxenstored too? Otherwise, I think it needs
> some explicit documentation, that late PV with dom0less requires
> cxenstored.

You have a point here, thanks for the heads up. Given my lack of OCaml
skills, I'll re-submit without the oxenstored part. Once we are settled
on the cxenstored changes, I'll attempt to change oxenstored too.