[PATCH v6 0/7] dom0less PV drivers

[PATCH v6 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

The patch series introduces 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.

This patch series is based on Daniel P. Smith's "Adds starting the idle
domain privileged".

A important change in v5 is the usage of
XS_CONNECTION_STATE_RECONNECTING to signal that the xenstore interface
is not ready.

Cheers,

Stefano

Luca Miccio (3):
      xen/arm: configure dom0less domain for enabling xenstore after boot
      xenstored: send an evtchn notification on introduce_domain
      tools: add example application to initialize dom0less PV drivers

Stefano Stabellini (4):
      xen/dt: of_property_read_string return -ENODATA when !length
      xen/arm: implement domU extended regions
      xen: introduce xen,enhanced dom0less property
      docs: document dom0less + PV drivers

 docs/features/dom0less.pandoc         |  43 ++++-
 docs/misc/arm/device-tree/booting.txt |  18 ++
 docs/misc/xen-command-line.pandoc     |   9 +-
 tools/helpers/Makefile                |  13 ++
 tools/helpers/init-dom0less.c         | 340 ++++++++++++++++++++++++++++++++++
 tools/xenstore/xenstored_domain.c     |   4 +
 xen/arch/arm/domain_build.c           | 104 ++++++++++-
 xen/arch/arm/include/asm/kernel.h     |   3 +
 xen/common/device_tree.c              |   2 +-
 xen/common/event_channel.c            |   2 +-
 xen/include/public/io/xs_wire.h       |   2 +-
 xen/include/xen/device_tree.h         |   3 +
 xen/include/xen/event.h               |   3 +
 13 files changed, 527 insertions(+), 19 deletions(-)
 create mode 100644 tools/helpers/init-dom0less.c

[PATCH v6 1/7] xen/dt: of_property_read_string return -ENODATA when !length

[Stefano Stabellini]

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

When the length of the string is zero of_property_read_string should
return -ENODATA according to the description of the function.

However, of_property_read_string doesn't check prop->length. If
prop->length is zero, return -ENODATA.

Without this patch the following command in u-boot:

fdt set /chosen/node property-name

results in of_property_read_string returning -EILSEQ when attempting to
read property-name. With this patch, it returns -ENODATA as expected.

This commit is a backport of:
https://lore.kernel.org/xen-devel/20220416003028.1315268-1-sstabellini@kernel.org/

Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>
---
Changes in v5:
- backport from Linux
---
 xen/common/device_tree.c      | 2 +-
 xen/include/xen/device_tree.h | 3 +++
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
index 4aae281e89..0e8798bd24 100644
--- a/xen/common/device_tree.c
+++ b/xen/common/device_tree.c
@@ -198,7 +198,7 @@ int dt_property_read_string(const struct dt_device_node *np,
 
     if ( !pp )
         return -EINVAL;
-    if ( !pp->value )
+    if ( !pp->length )
         return -ENODATA;
     if ( strnlen(pp->value, pp->length) >= pp->length )
         return -EILSEQ;
diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
index fd6cd00b43..430a1ef445 100644
--- a/xen/include/xen/device_tree.h
+++ b/xen/include/xen/device_tree.h
@@ -451,6 +451,9 @@ static inline bool_t dt_property_read_bool(const struct dt_device_node *np,
  * doest not have value, and -EILSEQ if the string is not
  * null-terminated with the length of the property data.
  *
+ * Note that the empty string "" has length of 1, thus -ENODATA cannot
+ * be interpreted as an empty string.
+ *
  * The out_string pointer is modified only if a valid string can be decoded.
  */
 int dt_property_read_string(const struct dt_device_node *np,
-- 
2.25.1

[PATCH v6 2/7] xen/arm: implement domU extended regions

[Stefano Stabellini]

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

Implement extended regions for dom0less domUs. The implementation is
based on the libxl implementation.

Also update docs for the ext_regions command line option.

Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>
CC: olekstysh@gmail.com
---
Changes in v6:
- add reviewed-by
- address 2 NITs
- update docs

Changes in v5:
- print the domain
- coding style
- simplify the code in find_domU_holes
- return error if no regions allocated in find_domU_holes
- use ROUNDUP
- uint64_t/paddr_t
---
 docs/misc/xen-command-line.pandoc |  9 ++---
 xen/arch/arm/domain_build.c       | 60 ++++++++++++++++++++++++++-----
 2 files changed, 56 insertions(+), 13 deletions(-)

diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
index 1dc7e1ca07..881fe409ac 100644
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -1110,11 +1110,12 @@ to use the default.
 
 > Default : `true`
 
-Flag to enable or disable support for extended regions for Dom0.
+Flag to enable or disable support for extended regions for Dom0 and
+Dom0less DomUs.
 
-Extended regions are ranges of unused address space exposed to Dom0 as
-"safe to use" for special memory mappings. Disable if your board device
-tree is incomplete.
+Extended regions are ranges of unused address space exposed to the guest
+as "safe to use" for special memory mappings. Disable if your board
+device tree is incomplete.
 
 ### flask
 > `= permissive | enforcing | late | disabled`
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 1472ca4972..f22450b4b7 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -35,7 +35,10 @@
 static unsigned int __initdata opt_dom0_max_vcpus;
 integer_param("dom0_max_vcpus", opt_dom0_max_vcpus);
 
-/* If true, the extended regions support is enabled for dom0 */
+/*
+ * If true, the extended regions support is enabled for dom0 and
+ * dom0less domUs.
+ */
 static bool __initdata opt_ext_regions = true;
 boolean_param("ext_regions", opt_ext_regions);
 
@@ -1324,6 +1327,36 @@ out:
     return res;
 }
 
+static int __init find_domU_holes(const struct kernel_info *kinfo,
+                                  struct meminfo *ext_regions)
+{
+    unsigned int i;
+    paddr_t bankend;
+    const paddr_t bankbase[] = GUEST_RAM_BANK_BASES;
+    const paddr_t banksize[] = GUEST_RAM_BANK_SIZES;
+    int res = -ENOENT;
+
+    for ( i = 0; i < GUEST_RAM_BANKS; i++ )
+    {
+        struct membank *ext_bank = &(ext_regions->bank[ext_regions->nr_banks]);
+
+        ext_bank->start = ROUNDUP(bankbase[i] + kinfo->mem.bank[i].size, SZ_2M);
+
+        bankend = ~0ULL >> (64 - p2m_ipa_bits);
+        bankend = min(bankend, bankbase[i] + banksize[i] - 1);
+        if ( bankend > ext_bank->start )
+            ext_bank->size = bankend - ext_bank->start + 1;
+
+        /* 64MB is the minimum size of an extended region */
+        if ( ext_bank->size < MB(64) )
+            continue;
+        ext_regions->nr_banks++;
+        res = 0;
+    }
+
+    return res;
+}
+
 static int __init make_hypervisor_node(struct domain *d,
                                        const struct kernel_info *kinfo,
                                        int addrcells, int sizecells)
@@ -1360,12 +1393,13 @@ static int __init make_hypervisor_node(struct domain *d,
 
     if ( !opt_ext_regions )
     {
-        printk(XENLOG_INFO "The extended regions support is disabled\n");
+        printk(XENLOG_INFO "%pd: extended regions support is disabled\n", d);
         nr_ext_regions = 0;
     }
     else if ( is_32bit_domain(d) )
     {
-        printk(XENLOG_WARNING "The extended regions are only supported for 64-bit guest currently\n");
+        printk(XENLOG_WARNING
+               "%pd: extended regions not supported for 32-bit guests\n", d);
         nr_ext_regions = 0;
     }
     else
@@ -1374,13 +1408,21 @@ static int __init make_hypervisor_node(struct domain *d,
         if ( !ext_regions )
             return -ENOMEM;
 
-        if ( !is_iommu_enabled(d) )
-            res = find_unallocated_memory(kinfo, ext_regions);
+        if ( is_domain_direct_mapped(d) )
+        {
+            if ( !is_iommu_enabled(d) )
+                res = find_unallocated_memory(kinfo, ext_regions);
+            else
+                res = find_memory_holes(kinfo, ext_regions);
+        }
         else
-            res = find_memory_holes(kinfo, ext_regions);
+        {
+            res = find_domU_holes(kinfo, ext_regions);
+        }
 
         if ( res )
-            printk(XENLOG_WARNING "Failed to allocate extended regions\n");
+            printk(XENLOG_WARNING "%pd: failed to allocate extended regions\n",
+                   d);
         nr_ext_regions = ext_regions->nr_banks;
     }
 
@@ -1401,8 +1443,8 @@ static int __init make_hypervisor_node(struct domain *d,
         u64 start = ext_regions->bank[i].start;
         u64 size = ext_regions->bank[i].size;
 
-        printk("Extended region %d: %#"PRIx64"->%#"PRIx64"\n",
-               i, start, start + size);
+        printk("%pd: extended region %d: %#"PRIx64"->%#"PRIx64"\n",
+               d, i, start, start + size);
 
         dt_child_set_range(&cells, addrcells, sizecells, start, size);
     }
-- 
2.25.1

[PATCH v6 3/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.)

The configurable option is for domUs only. For dom0 we always set the
corresponding property in the Xen code to true (PV interfaces enabled.)

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

Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
CC: Julien Grall <julien@xen.org>
CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
CC: Bertrand Marquis <bertrand.marquis@arm.com>
---
Changes in v4:
- move xen,enhanced to the bottom of the list
- do not set kinfo.dom0less_enhanced for dom0

Changes in v3:
- improve commit message

Changes in v2:
- rename kinfo.enhanced to kinfo.dom0less_enhanced
- set kinfo.dom0less_enhanced to true for dom0
- handle -ENODATA in addition to -EILSEQ
---
 docs/misc/arm/device-tree/booting.txt | 18 ++++++++++++++++++
 xen/arch/arm/domain_build.c           |  7 +++++++
 xen/arch/arm/include/asm/kernel.h     |  3 +++
 3 files changed, 28 insertions(+)

diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt
index a94125394e..92097c4969 100644
--- a/docs/misc/arm/device-tree/booting.txt
+++ b/docs/misc/arm/device-tree/booting.txt
@@ -188,6 +188,24 @@ with the following properties:
     An empty property to request the memory of the domain to be
     direct-map (guest physical address == physical address).
 
+- 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.
+
 Under the "xen,domain" compatible node, one or more sub-nodes are present
 for the DomU kernel and ramdisk.
 
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index f22450b4b7..016f56a99f 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -3154,6 +3154,7 @@ static int __init construct_domU(struct domain *d,
                                  const struct dt_device_node *node)
 {
     struct kernel_info kinfo = {};
+    const char *dom0less_enhanced;
     int rc;
     u64 mem;
 
@@ -3169,6 +3170,12 @@ static int __init construct_domU(struct domain *d,
 
     kinfo.vpl011 = dt_property_read_bool(node, "vpl011");
 
+    rc = dt_property_read_string(node, "xen,enhanced", &dom0less_enhanced);
+    if ( rc == -EILSEQ ||
+         rc == -ENODATA ||
+         (rc == 0 && !strcmp(dom0less_enhanced, "enabled")) )
+        kinfo.dom0less_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..c4dc039b54 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 dom0less_enhanced;
+
     /* GIC phandle */
     uint32_t phandle_gic;
 
-- 
2.25.1

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

[Stefano Stabellini]

From: Luca Miccio <lucmiccio@gmail.com>

Export evtchn_alloc_unbound and make it __must_check.

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>
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
CC: Julien Grall <julien@xen.org>
CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
CC: Bertrand Marquis <bertrand.marquis@arm.com>
CC: jbeulich@suse.com

---
Changes in v5:
- merge with "xen: export evtchn_alloc_unbound"
- __must_check

Changes in v3:
- use evtchn_alloc_unbound

Changes in v2:
- set HVM_PARAM_STORE_PFN to ~0ULL at domain creation
- in alloc_xenstore_evtchn do not call _evtchn_alloc_unbound

xen: export evtchn_alloc_unbound

It will be used during dom0less domains construction.

Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
---
 xen/arch/arm/domain_build.c | 37 +++++++++++++++++++++++++++++++++++++
 xen/common/event_channel.c  |  2 +-
 xen/include/xen/event.h     |  3 +++
 3 files changed, 41 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 016f56a99f..bb430f2189 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>
@@ -2810,6 +2811,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;
@@ -2884,6 +2887,13 @@ static int __init prepare_dtb_domU(struct domain *d, struct kernel_info *kinfo)
             goto err;
     }
 
+    if ( kinfo->dom0less_enhanced )
+    {
+        ret = make_hypervisor_node(d, kinfo, addrcells, sizecells);
+        if ( ret )
+            goto err;
+    }
+
     ret = fdt_end_node(kinfo->fdt);
     if ( ret < 0 )
         goto err;
@@ -3150,6 +3160,25 @@ static int __init construct_domain(struct domain *d, struct kernel_info *kinfo)
     return 0;
 }
 
+static int __init alloc_xenstore_evtchn(struct domain *d)
+{
+    evtchn_alloc_unbound_t alloc;
+    int rc;
+
+    alloc.dom = d->domain_id;
+    alloc.remote_dom = hardware_domain->domain_id;
+    rc = evtchn_alloc_unbound(&alloc);
+    if ( rc )
+    {
+        printk("Failed allocating event channel for domain\n");
+        return rc;
+    }
+
+    d->arch.hvm.params[HVM_PARAM_STORE_EVTCHN] = alloc.port;
+
+    return 0;
+}
+
 static int __init construct_domU(struct domain *d,
                                  const struct dt_device_node *node)
 {
@@ -3214,6 +3243,14 @@ static int __init construct_domU(struct domain *d,
     if ( rc < 0 )
         return rc;
 
+    if ( kinfo.dom0less_enhanced )
+    {
+        rc = alloc_xenstore_evtchn(d);
+        if ( rc < 0 )
+            return rc;
+        d->arch.hvm.params[HVM_PARAM_STORE_PFN] = ~0ULL;
+    }
+
     return rc;
 }
 
diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
index 0a82eb3ac2..e60cd98d75 100644
--- a/xen/common/event_channel.c
+++ b/xen/common/event_channel.c
@@ -290,7 +290,7 @@ void evtchn_free(struct domain *d, struct evtchn *chn)
     xsm_evtchn_close_post(chn);
 }
 
-static int evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc)
+int evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc)
 {
     struct evtchn *chn;
     struct domain *d;
diff --git a/xen/include/xen/event.h b/xen/include/xen/event.h
index 21c95e14fd..f3021fe304 100644
--- a/xen/include/xen/event.h
+++ b/xen/include/xen/event.h
@@ -71,6 +71,9 @@ void evtchn_free(struct domain *d, struct evtchn *chn);
 /* Allocate a specific event channel port. */
 int evtchn_allocate_port(struct domain *d, unsigned int port);
 
+/* Allocate a new event channel */
+int __must_check evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc);
+
 /* Unmask a local event-channel port. */
 int evtchn_unmask(unsigned int port);
 
-- 
2.25.1

[PATCH v6 5/7] xenstored: send an evtchn notification on introduce_domain

[Stefano Stabellini]

From: Luca Miccio <lucmiccio@gmail.com>

When xs_introduce_domain is called, send out a notification on the
xenstore event channel so that any (dom0less) domain waiting for the
xenstore interface to be ready can continue with the initialization.
Before sending the notification, clear XENSTORE_RECONNECTING.

The extra notification is harmless for domains that don't require it.

In xs_wire.h update the commment on top of XENSTORE_RECONNECTING to
generalize its meaning to suit the dom0less use-case better.

Signed-off-by: Luca Miccio <lucmiccio@gmail.com>
Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
CC: jgross@suse.com
CC: anthony.perard@citrix.com
CC: wl@xen.org
---
If you have better suggestions for the wording in xs_wire.h please
suggest!


Changes in v6:
- use XENSTORE_CONNECTED instead of 0x0
- update xs_wire.h

Changes in v5:
- reset XS_CONNECTION_STATE_RECONNECTING before notifying the domU

Changes in v2:
- drop the new late_init parameter
---
 tools/xenstore/xenstored_domain.c | 4 ++++
 xen/include/public/io/xs_wire.h   | 2 +-
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/tools/xenstore/xenstored_domain.c b/tools/xenstore/xenstored_domain.c
index ae065fcbee..6f34af225c 100644
--- a/tools/xenstore/xenstored_domain.c
+++ b/tools/xenstore/xenstored_domain.c
@@ -493,6 +493,10 @@ static struct domain *introduce_domain(const void *ctx,
 		/* Now domain belongs to its connection. */
 		talloc_steal(domain->conn, domain);
 
+		/* Notify the domain that xenstore is available */
+		interface->connection = XENSTORE_CONNECTED;
+		xenevtchn_notify(xce_handle, domain->port);
+
 		if (!is_master_domain && !restore)
 			fire_watches(NULL, ctx, "@introduceDomain", NULL,
 				     false, NULL);
diff --git a/xen/include/public/io/xs_wire.h b/xen/include/public/io/xs_wire.h
index 953a0050a3..c1ec7c73e3 100644
--- a/xen/include/public/io/xs_wire.h
+++ b/xen/include/public/io/xs_wire.h
@@ -141,7 +141,7 @@ struct xenstore_domain_interface {
 
 /* Valid values for the connection field */
 #define XENSTORE_CONNECTED 0 /* the steady-state */
-#define XENSTORE_RECONNECT 1 /* guest has initiated a reconnect */
+#define XENSTORE_RECONNECT 1 /* reconnect in progress */
 
 /* Valid values for the error field */
 #define XENSTORE_ERROR_NONE    0 /* No error */
-- 
2.25.1

[PATCH v6 6/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.

The application sets "connection" to XENSTORE_RECONNECT on the xenstore
page before calling xs_introduce_domain to signal that the connection is
not ready yet to be used. XENSTORE_RECONNECT is reset soon after by
xenstored.

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>
---
Changes in v6:
- include xs_wire.h and use its definitions

Changes in v5:
- set XS_CONNECTION_STATE_RECONNECTING before xs_introduce_domain

Changes in v4:
- only alloc xs page (no other magic pages)
- add xenstore permissions
- check all return values
- rename restore_xenstore to create_xenstore
- set target_memkb
- set start_time properly
- close xs transaction on error
- call xc_dom_gnttab_seed instead of xc_dom_gnttab_init
- xs_open instead of xs_daemon_open

Changes in v3:
- handle xenstore errors
- add an in-code comment about xenstore entries
- less verbose output
- clean-up error path in main

Changes in v2:
- do not set HVM_PARAM_STORE_EVTCHN twice
- rename restore_xenstore to create_xenstore
- increase maxmem

connection reconnecting
---
 tools/helpers/Makefile        |  13 ++
 tools/helpers/init-dom0less.c | 340 ++++++++++++++++++++++++++++++++++
 2 files changed, 353 insertions(+)
 create mode 100644 tools/helpers/init-dom0less.c

diff --git a/tools/helpers/Makefile b/tools/helpers/Makefile
index 7f6c422440..8d78ab1e90 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) $(LDLIBS_libxenforeignmemory) $(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..bfd5ff0761
--- /dev/null
+++ b/tools/helpers/init-dom0less.c
@@ -0,0 +1,340 @@
+#include <stdbool.h>
+#include <syslog.h>
+#include <stdio.h>
+#include <err.h>
+#include <stdlib.h>
+#include <sys/time.h>
+#include <xenstore.h>
+#include <xenctrl.h>
+#include <xenguest.h>
+#include <libxl.h>
+#include <xenevtchn.h>
+#include <xenforeignmemory.h>
+#include <xen/io/xs_wire.h>
+
+#include "init-dom-json.h"
+
+#define XENSTORE_PFN_OFFSET 1
+#define STR_MAX_LENGTH 64
+
+static int alloc_xs_page(struct xc_interface_core *xch,
+                         libxl_dominfo *info,
+                         uint64_t *xenstore_pfn)
+{
+    int rc;
+    const xen_pfn_t base = GUEST_MAGIC_BASE >> XC_PAGE_SHIFT;
+    xen_pfn_t p2m = (GUEST_MAGIC_BASE >> XC_PAGE_SHIFT) + XENSTORE_PFN_OFFSET;
+
+    rc = xc_domain_setmaxmem(xch, info->domid,
+                             info->max_memkb + (XC_PAGE_SIZE/1024));
+    if (rc < 0)
+        return rc;
+
+    rc = xc_domain_populate_physmap_exact(xch, info->domid, 1, 0, 0, &p2m);
+    if (rc < 0)
+        return rc;
+
+    *xenstore_pfn = base + XENSTORE_PFN_OFFSET;
+    rc = xc_clear_domain_page(xch, info->domid, *xenstore_pfn);
+    if (rc < 0)
+        return rc;
+
+    return 0;
+}
+
+static bool 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];
+    struct xs_permissions perms[2];
+
+    perms[0].id = domid;
+    perms[0].perms = XS_PERM_NONE;
+    perms[1].id = 0;
+    perms[1].perms = XS_PERM_READ;
+
+    if (snprintf(full_path, STR_MAX_LENGTH,
+                 "/local/domain/%u/%s", domid, path) < 0)
+        return false;
+    if (!xs_write(xsh, t, full_path, val, strlen(val)))
+        return false;
+    return xs_set_permissions(xsh, t, full_path, perms, 2);
+}
+
+static bool 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];
+
+    if (snprintf(full_path, STR_MAX_LENGTH,
+                 "/libxl/%u/%s", domid, path) < 0)
+        return false;
+    return xs_write(xsh, t, full_path, val, strlen(val));
+}
+
+static bool 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];
+
+    if (snprintf(full_path, STR_MAX_LENGTH,
+                 "/vm/" LIBXL_UUID_FMT "/%s", LIBXL_UUID_BYTES(uuid), path) < 0)
+        return false;
+    return xs_write(xsh, t, full_path, val, strlen(val));
+}
+
+/*
+ * The xenstore nodes are the xenstore nodes libxl writes at domain
+ * creation.
+ *
+ * The list was retrieved by running xenstore-ls on a corresponding
+ * domain started by xl/libxl.
+ */
+static int create_xenstore(struct xs_handle *xsh,
+                           libxl_dominfo *info, libxl_uuid uuid,
+                           evtchn_port_t xenstore_port)
+{
+    domid_t domid;
+    unsigned 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 target_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;
+    struct timeval start_time;
+    char start_time_str[STR_MAX_LENGTH];
+    int rc;
+
+    if (gettimeofday(&start_time, NULL) < 0)
+        return -errno;
+    rc = snprintf(start_time_str, STR_MAX_LENGTH, "%jd.%02d",
+            (intmax_t)start_time.tv_sec, (int)start_time.tv_usec / 10000);
+    if (rc < 0)
+        return rc;
+
+    domid = info->domid;
+    rc = snprintf(id_str, STR_MAX_LENGTH, "%u", domid);
+    if (rc < 0)
+        return rc;
+    rc = snprintf(dom_name_str, STR_MAX_LENGTH, "dom0less-%u", domid);
+    if (rc < 0)
+        return rc;
+    rc = snprintf(uuid_str, STR_MAX_LENGTH, LIBXL_UUID_FMT, LIBXL_UUID_BYTES(uuid));
+    if (rc < 0)
+        return rc;
+    rc = snprintf(vm_val_str, STR_MAX_LENGTH,
+                  "vm/" LIBXL_UUID_FMT, LIBXL_UUID_BYTES(uuid));
+    if (rc < 0)
+        return rc;
+    rc = snprintf(max_memkb_str, STR_MAX_LENGTH, "%lu", info->max_memkb);
+    if (rc < 0)
+        return rc;
+    rc = snprintf(target_memkb_str, STR_MAX_LENGTH, "%lu", info->current_memkb);
+    if (rc < 0)
+        return rc;
+    rc = snprintf(ring_ref_str, STR_MAX_LENGTH, "%lld",
+                  (GUEST_MAGIC_BASE >> XC_PAGE_SHIFT) + XENSTORE_PFN_OFFSET);
+    if (rc < 0)
+        return rc;
+    rc = snprintf(xenstore_port_str, STR_MAX_LENGTH, "%u", xenstore_port);
+    if (rc < 0)
+        return rc;
+
+retry_transaction:
+    t = xs_transaction_start(xsh);
+    if (t == XBT_NULL)
+        return -errno;
+
+    rc = -EIO;
+    /* /vm */
+    if (!do_xs_write_vm(xsh, t, uuid, "name", dom_name_str)) goto err;
+    if (!do_xs_write_vm(xsh, t, uuid, "uuid", uuid_str)) goto err;
+    if (!do_xs_write_vm(xsh, t, uuid, "start_time", start_time_str)) goto err;
+
+    /* /domain */
+    if (!do_xs_write_dom(xsh, t, domid, "vm", vm_val_str)) goto err;
+    if (!do_xs_write_dom(xsh, t, domid, "name", dom_name_str)) goto err;
+    if (!do_xs_write_dom(xsh, t, domid, "cpu", "")) goto err;
+    for (i = 0; i < info->vcpu_max_id; i++) {
+        rc = snprintf(cpu_str, STR_MAX_LENGTH, "cpu/%u/availability/", i);
+        if (rc < 0)
+            goto err;
+        rc = -EIO;
+        if (!do_xs_write_dom(xsh, t, domid, cpu_str,
+                             (info->cpupool & (1 << i)) ? "online" : "offline"))
+            goto err;
+    }
+
+    if (!do_xs_write_dom(xsh, t, domid, "memory", "")) goto err;
+    if (!do_xs_write_dom(xsh, t, domid, "memory/static-max", max_memkb_str)) goto err;
+    if (!do_xs_write_dom(xsh, t, domid, "memory/target", target_memkb_str)) goto err;
+    if (!do_xs_write_dom(xsh, t, domid, "memory/videoram", "-1")) goto err;
+
+    if (!do_xs_write_dom(xsh, t, domid, "device", "")) goto err;
+    if (!do_xs_write_dom(xsh, t, domid, "device/suspend", "")) goto err;
+    if (!do_xs_write_dom(xsh, t, domid, "device/suspend/event-channel", "")) goto err;
+
+    if (!do_xs_write_dom(xsh, t, domid, "control", "")) goto err;
+    if (!do_xs_write_dom(xsh, t, domid, "control/shutdown", "")) goto err;
+    if (!do_xs_write_dom(xsh, t, domid, "control/feature-poweroff", "1")) goto err;
+    if (!do_xs_write_dom(xsh, t, domid, "control/feature-reboot", "1")) goto err;
+    if (!do_xs_write_dom(xsh, t, domid, "control/feature-suspend", "")) goto err;
+    if (!do_xs_write_dom(xsh, t, domid, "control/sysrq", "")) goto err;
+    if (!do_xs_write_dom(xsh, t, domid, "control/platform-feature-multiprocessor-suspend", "1")) goto err;
+    if (!do_xs_write_dom(xsh, t, domid, "control/platform-feature-xs_reset_watches", "1")) goto err;
+
+    if (!do_xs_write_dom(xsh, t, domid, "domid", id_str)) goto err;
+    if (!do_xs_write_dom(xsh, t, domid, "data", "")) goto err;
+    if (!do_xs_write_dom(xsh, t, domid, "drivers", "")) goto err;
+    if (!do_xs_write_dom(xsh, t, domid, "feature", "")) goto err;
+    if (!do_xs_write_dom(xsh, t, domid, "attr", "")) goto err;
+
+    if (!do_xs_write_dom(xsh, t, domid, "store/port", xenstore_port_str)) goto err;
+    if (!do_xs_write_dom(xsh, t, domid, "store/ring-ref", ring_ref_str)) goto err;
+
+    if (!do_xs_write_libxl(xsh, t, domid, "type", "pvh")) goto err;
+    if (!do_xs_write_libxl(xsh, t, domid, "dm-version", "qemu_xen")) goto err;
+
+    if (!xs_transaction_end(xsh, t, false)) {
+        if (errno == EAGAIN)
+            goto retry_transaction;
+        else
+            return -errno;
+    }
+
+    return 0;
+
+err:
+    xs_transaction_end(xsh, t, true);
+    return rc;
+}
+
+static int init_domain(struct xs_handle *xsh,
+                       struct xc_interface_core *xch,
+                       xenforeignmemory_handle *xfh,
+                       libxl_dominfo *info)
+{
+    libxl_uuid uuid;
+    uint64_t xenstore_evtchn, xenstore_pfn;
+    int rc;
+    struct xenstore_domain_interface *intf;
+
+    printf("Init dom0less domain: %u\n", info->domid);
+
+    rc = xc_hvm_param_get(xch, info->domid, HVM_PARAM_STORE_EVTCHN,
+                          &xenstore_evtchn);
+    if (rc != 0) {
+        printf("Failed to get HVM_PARAM_STORE_EVTCHN\n");
+        return 1;
+    }
+
+    /* Alloc xenstore page */
+    if (alloc_xs_page(xch, info, &xenstore_pfn) != 0) {
+        printf("Error on alloc magic pages\n");
+        return 1;
+    }
+
+    intf = xenforeignmemory_map(xfh, info->domid, XS_READ | XS_WRITE, 1,
+                                &xenstore_pfn, NULL);
+    if (!intf) {
+        printf("Error mapping xenstore page\n");
+        return 1;
+    }
+    intf->connection = XENSTORE_RECONNECT;
+    xenforeignmemory_unmap(xfh, intf, 1);
+
+    rc = xc_dom_gnttab_seed(xch, info->domid, true,
+                            (xen_pfn_t)-1, xenstore_pfn, 0, 0);
+    if (rc)
+        err(1, "xc_dom_gnttab_seed");
+
+    libxl_uuid_generate(&uuid);
+    xc_domain_sethandle(xch, info->domid, libxl_uuid_bytearray(&uuid));
+
+    rc = gen_stub_json_config(info->domid, &uuid);
+    if (rc)
+        err(1, "gen_stub_json_config");
+
+    /* Now everything is ready: set HVM_PARAM_STORE_PFN */
+    rc = xc_hvm_param_set(xch, info->domid, HVM_PARAM_STORE_PFN,
+                          xenstore_pfn);
+    if (rc < 0)
+        return rc;
+
+    rc = create_xenstore(xsh, info, uuid, xenstore_evtchn);
+    if (rc)
+        err(1, "writing to xenstore");
+
+    rc = xs_introduce_domain(xsh, info->domid,
+            (GUEST_MAGIC_BASE >> XC_PAGE_SHIFT) + XENSTORE_PFN_OFFSET,
+            xenstore_evtchn);
+    if (!rc)
+        err(1, "xs_introduce_domain");
+    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 = NULL;
+    libxl_ctx *ctx;
+    int nb_vm = 0, rc = 0, i;
+    struct xs_handle *xsh = NULL;
+    struct xc_interface_core *xch = NULL;
+    xenforeignmemory_handle *xfh = NULL;
+
+    /* TODO reuse libxl xsh connection */
+    xsh = xs_open(0);
+    xch = xc_interface_open(0, 0, 0);
+    xfh = xenforeignmemory_open(0, 0);
+    if (xsh == NULL || xch == NULL || xfh == NULL) {
+        fprintf(stderr, "Cannot open xc/xs/xenforeignmemory interfaces");
+        rc = -errno;
+        goto out;
+    }
+
+    rc = libxl_ctx_alloc(&ctx, LIBXL_VERSION, 0, NULL);
+    if (rc) {
+        fprintf(stderr, "cannot init xl context\n");
+        goto out;
+    }
+
+    info = libxl_list_domain(ctx, &nb_vm);
+    if (!info) {
+        fprintf(stderr, "libxl_list_vm failed.\n");
+        rc = -1;
+        goto out;
+    }
+
+    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)) {
+            rc = init_domain(xsh, xch, xfh, &info[i]);
+            if (rc < 0) {
+                fprintf(stderr, "init_domain failed.\n");
+                goto out;
+            }
+        } else {
+            printf("Domain %u has already been initialized\n", domid);
+        }
+    }
+out:
+    libxl_dominfo_list_free(info, nb_vm);
+    return rc;
+}
-- 
2.25.1

[PATCH v6 7/7] docs: document dom0less + PV drivers

[Stefano Stabellini]

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

Document how to use the feature and how the implementation works.

Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>
---
 docs/features/dom0less.pandoc | 43 ++++++++++++++++++++++++++++++++---
 1 file changed, 40 insertions(+), 3 deletions(-)

diff --git a/docs/features/dom0less.pandoc b/docs/features/dom0less.pandoc
index c9edb529e1..725afa0558 100644
--- a/docs/features/dom0less.pandoc
+++ b/docs/features/dom0less.pandoc
@@ -90,6 +90,46 @@ Otherwise, they may be unusable in Xen (for instance if they are compressed).
 
 See docs/misc/arm/device-tree/booting.txt for more information.
 
+PV Drivers
+----------
+
+It is possible to use PV drivers with dom0less guests with some
+restrictions:
+
+- dom0less domUs that want to use PV drivers support should have the
+  "xen,enhanced" property set under their device tree nodes (see
+  docs/misc/arm/device-tree/booting.txt)
+- a dom0 must be present (or another domain with enough privileges to
+  run the toolstack)
+- after dom0 is booted, the utility "init-dom0less" must be run
+- do not run "init-dom0less" while creating other guests with xl
+
+After the execution of init-dom0less, it is possible to use "xl" to
+hotplug PV drivers to dom0less guests. E.g. xl network-attach domU.
+
+The implementation works as follows:
+- Xen allocates the xenstore event channel for each dom0less domU that
+  has the "xen,enhanced" property, and sets HVM_PARAM_STORE_EVTCHN
+- Xen does *not* allocate the xenstore page and sets HVM_PARAM_STORE_PFN
+  to ~0ULL (invalid)
+- Dom0less domU kernels check that HVM_PARAM_STORE_PFN is set to invalid
+    - Old kernels will continue without xenstore support (Note: some old
+      buggy kernels might crash because they don't check the validity of
+      HVM_PARAM_STORE_PFN before using it! Disable "xen,enhanced" in
+      those cases)
+    - New kernels will wait for a notification on the xenstore event
+      channel (HVM_PARAM_STORE_EVTCHN) before continuing with the
+      initialization
+- Once dom0 is booted, init-dom0less is executed:
+    - it allocates the xenstore shared page and sets HVM_PARAM_STORE_PFN
+    - it calls xs_introduce_domain
+- Xenstored notices the new domain, initializes interfaces as usual, and
+  sends an event channel notification to the domain using the xenstore
+  event channel (HVM_PARAM_STORE_EVTCHN)
+- The Linux domU kernel receives the event channel notification, checks
+  HVM_PARAM_STORE_PFN again and continue with the initialization
+
+
 Limitations
 -----------
 
@@ -107,9 +147,6 @@ limitations:
   information, the GIC version exposed to the domains started by Xen at
   boot is the same as the native GIC version.
 
-- No PV drivers. There is no support for PV devices at the moment. All
-  devices need to be statically assigned to guests.
-
 - Pinning vCPUs of domains started by Xen at boot can be
   done from the control domain, using `xl vcpu-pin` as usual. It is not
   currently possible to configure vCPU pinning without a control domain.
-- 
2.25.1

Re: [PATCH v6 4/7] xen/arm: configure dom0less domain for enabling xenstore after boot

[Jan Beulich]

On 05.05.2022 02:16, Stefano Stabellini wrote:
> From: Luca Miccio <lucmiccio@gmail.com>
> 
> Export evtchn_alloc_unbound and make it __must_check.
> 
> 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>
> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
> CC: Julien Grall <julien@xen.org>
> CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
> CC: Bertrand Marquis <bertrand.marquis@arm.com>
> CC: jbeulich@suse.com

Somehow my ack given for v5 was lost?

> ---
> Changes in v5:
> - merge with "xen: export evtchn_alloc_unbound"
> - __must_check
> 
> Changes in v3:
> - use evtchn_alloc_unbound
> 
> Changes in v2:
> - set HVM_PARAM_STORE_PFN to ~0ULL at domain creation
> - in alloc_xenstore_evtchn do not call _evtchn_alloc_unbound
> 
> xen: export evtchn_alloc_unbound
> 
> It will be used during dom0less domains construction.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>

This is somewhat confusing to find in a post-commit-message remark.

Jan

Re: [PATCH v6 5/7] xenstored: send an evtchn notification on introduce_domain

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 937 bytes --]

On 05.05.22 02:16, Stefano Stabellini wrote:
> From: Luca Miccio <lucmiccio@gmail.com>
> 
> When xs_introduce_domain is called, send out a notification on the
> xenstore event channel so that any (dom0less) domain waiting for the
> xenstore interface to be ready can continue with the initialization.
> Before sending the notification, clear XENSTORE_RECONNECTING.
> 
> The extra notification is harmless for domains that don't require it.
> 
> In xs_wire.h update the commment on top of XENSTORE_RECONNECTING to
> generalize its meaning to suit the dom0less use-case better.
> 
> Signed-off-by: Luca Miccio <lucmiccio@gmail.com>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
> CC: jgross@suse.com
> CC: anthony.perard@citrix.com
> CC: wl@xen.org
> ---
> If you have better suggestions for the wording in xs_wire.h please
> suggest!

Reviewed-by: Juergen Gross <jgross@suse.com>


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

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

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 13651 bytes --]

On 05.05.22 02:16, 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.
> 
> The application sets "connection" to XENSTORE_RECONNECT on the xenstore
> page before calling xs_introduce_domain to signal that the connection is
> not ready yet to be used. XENSTORE_RECONNECT is reset soon after by
> xenstored.
> 
> 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>
> ---
> Changes in v6:
> - include xs_wire.h and use its definitions
> 
> Changes in v5:
> - set XS_CONNECTION_STATE_RECONNECTING before xs_introduce_domain
> 
> Changes in v4:
> - only alloc xs page (no other magic pages)
> - add xenstore permissions
> - check all return values
> - rename restore_xenstore to create_xenstore
> - set target_memkb
> - set start_time properly
> - close xs transaction on error
> - call xc_dom_gnttab_seed instead of xc_dom_gnttab_init
> - xs_open instead of xs_daemon_open
> 
> Changes in v3:
> - handle xenstore errors
> - add an in-code comment about xenstore entries
> - less verbose output
> - clean-up error path in main
> 
> Changes in v2:
> - do not set HVM_PARAM_STORE_EVTCHN twice
> - rename restore_xenstore to create_xenstore
> - increase maxmem
> 
> connection reconnecting
> ---
>   tools/helpers/Makefile        |  13 ++
>   tools/helpers/init-dom0less.c | 340 ++++++++++++++++++++++++++++++++++
>   2 files changed, 353 insertions(+)
>   create mode 100644 tools/helpers/init-dom0less.c
> 
> diff --git a/tools/helpers/Makefile b/tools/helpers/Makefile
> index 7f6c422440..8d78ab1e90 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) $(LDLIBS_libxenforeignmemory) $(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..bfd5ff0761
> --- /dev/null
> +++ b/tools/helpers/init-dom0less.c
> @@ -0,0 +1,340 @@
> +#include <stdbool.h>
> +#include <syslog.h>
> +#include <stdio.h>
> +#include <err.h>
> +#include <stdlib.h>
> +#include <sys/time.h>
> +#include <xenstore.h>
> +#include <xenctrl.h>
> +#include <xenguest.h>
> +#include <libxl.h>
> +#include <xenevtchn.h>
> +#include <xenforeignmemory.h>
> +#include <xen/io/xs_wire.h>
> +
> +#include "init-dom-json.h"
> +
> +#define XENSTORE_PFN_OFFSET 1
> +#define STR_MAX_LENGTH 64
> +
> +static int alloc_xs_page(struct xc_interface_core *xch,
> +                         libxl_dominfo *info,
> +                         uint64_t *xenstore_pfn)
> +{
> +    int rc;
> +    const xen_pfn_t base = GUEST_MAGIC_BASE >> XC_PAGE_SHIFT;
> +    xen_pfn_t p2m = (GUEST_MAGIC_BASE >> XC_PAGE_SHIFT) + XENSTORE_PFN_OFFSET;
> +
> +    rc = xc_domain_setmaxmem(xch, info->domid,
> +                             info->max_memkb + (XC_PAGE_SIZE/1024));
> +    if (rc < 0)
> +        return rc;
> +
> +    rc = xc_domain_populate_physmap_exact(xch, info->domid, 1, 0, 0, &p2m);
> +    if (rc < 0)
> +        return rc;
> +
> +    *xenstore_pfn = base + XENSTORE_PFN_OFFSET;
> +    rc = xc_clear_domain_page(xch, info->domid, *xenstore_pfn);
> +    if (rc < 0)
> +        return rc;
> +
> +    return 0;
> +}
> +
> +static bool 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];
> +    struct xs_permissions perms[2];
> +
> +    perms[0].id = domid;
> +    perms[0].perms = XS_PERM_NONE;
> +    perms[1].id = 0;
> +    perms[1].perms = XS_PERM_READ;
> +
> +    if (snprintf(full_path, STR_MAX_LENGTH,
> +                 "/local/domain/%u/%s", domid, path) < 0)
> +        return false;
> +    if (!xs_write(xsh, t, full_path, val, strlen(val)))
> +        return false;
> +    return xs_set_permissions(xsh, t, full_path, perms, 2);
> +}
> +
> +static bool 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];
> +
> +    if (snprintf(full_path, STR_MAX_LENGTH,
> +                 "/libxl/%u/%s", domid, path) < 0)
> +        return false;
> +    return xs_write(xsh, t, full_path, val, strlen(val));
> +}
> +
> +static bool 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];
> +
> +    if (snprintf(full_path, STR_MAX_LENGTH,
> +                 "/vm/" LIBXL_UUID_FMT "/%s", LIBXL_UUID_BYTES(uuid), path) < 0)
> +        return false;
> +    return xs_write(xsh, t, full_path, val, strlen(val));
> +}
> +
> +/*
> + * The xenstore nodes are the xenstore nodes libxl writes at domain
> + * creation.
> + *
> + * The list was retrieved by running xenstore-ls on a corresponding
> + * domain started by xl/libxl.
> + */
> +static int create_xenstore(struct xs_handle *xsh,
> +                           libxl_dominfo *info, libxl_uuid uuid,
> +                           evtchn_port_t xenstore_port)
> +{
> +    domid_t domid;
> +    unsigned 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 target_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;
> +    struct timeval start_time;
> +    char start_time_str[STR_MAX_LENGTH];
> +    int rc;
> +
> +    if (gettimeofday(&start_time, NULL) < 0)
> +        return -errno;
> +    rc = snprintf(start_time_str, STR_MAX_LENGTH, "%jd.%02d",
> +            (intmax_t)start_time.tv_sec, (int)start_time.tv_usec / 10000);
> +    if (rc < 0)
> +        return rc;
> +
> +    domid = info->domid;
> +    rc = snprintf(id_str, STR_MAX_LENGTH, "%u", domid);
> +    if (rc < 0)
> +        return rc;
> +    rc = snprintf(dom_name_str, STR_MAX_LENGTH, "dom0less-%u", domid);
> +    if (rc < 0)
> +        return rc;
> +    rc = snprintf(uuid_str, STR_MAX_LENGTH, LIBXL_UUID_FMT, LIBXL_UUID_BYTES(uuid));
> +    if (rc < 0)
> +        return rc;
> +    rc = snprintf(vm_val_str, STR_MAX_LENGTH,
> +                  "vm/" LIBXL_UUID_FMT, LIBXL_UUID_BYTES(uuid));
> +    if (rc < 0)
> +        return rc;
> +    rc = snprintf(max_memkb_str, STR_MAX_LENGTH, "%lu", info->max_memkb);
> +    if (rc < 0)
> +        return rc;
> +    rc = snprintf(target_memkb_str, STR_MAX_LENGTH, "%lu", info->current_memkb);
> +    if (rc < 0)
> +        return rc;
> +    rc = snprintf(ring_ref_str, STR_MAX_LENGTH, "%lld",
> +                  (GUEST_MAGIC_BASE >> XC_PAGE_SHIFT) + XENSTORE_PFN_OFFSET);
> +    if (rc < 0)
> +        return rc;
> +    rc = snprintf(xenstore_port_str, STR_MAX_LENGTH, "%u", xenstore_port);
> +    if (rc < 0)
> +        return rc;
> +
> +retry_transaction:
> +    t = xs_transaction_start(xsh);
> +    if (t == XBT_NULL)
> +        return -errno;
> +
> +    rc = -EIO;
> +    /* /vm */
> +    if (!do_xs_write_vm(xsh, t, uuid, "name", dom_name_str)) goto err;
> +    if (!do_xs_write_vm(xsh, t, uuid, "uuid", uuid_str)) goto err;
> +    if (!do_xs_write_vm(xsh, t, uuid, "start_time", start_time_str)) goto err;
> +
> +    /* /domain */
> +    if (!do_xs_write_dom(xsh, t, domid, "vm", vm_val_str)) goto err;
> +    if (!do_xs_write_dom(xsh, t, domid, "name", dom_name_str)) goto err;
> +    if (!do_xs_write_dom(xsh, t, domid, "cpu", "")) goto err;
> +    for (i = 0; i < info->vcpu_max_id; i++) {
> +        rc = snprintf(cpu_str, STR_MAX_LENGTH, "cpu/%u/availability/", i);
> +        if (rc < 0)
> +            goto err;
> +        rc = -EIO;
> +        if (!do_xs_write_dom(xsh, t, domid, cpu_str,
> +                             (info->cpupool & (1 << i)) ? "online" : "offline"))
> +            goto err;
> +    }
> +
> +    if (!do_xs_write_dom(xsh, t, domid, "memory", "")) goto err;
> +    if (!do_xs_write_dom(xsh, t, domid, "memory/static-max", max_memkb_str)) goto err;
> +    if (!do_xs_write_dom(xsh, t, domid, "memory/target", target_memkb_str)) goto err;
> +    if (!do_xs_write_dom(xsh, t, domid, "memory/videoram", "-1")) goto err;
> +
> +    if (!do_xs_write_dom(xsh, t, domid, "device", "")) goto err;
> +    if (!do_xs_write_dom(xsh, t, domid, "device/suspend", "")) goto err;
> +    if (!do_xs_write_dom(xsh, t, domid, "device/suspend/event-channel", "")) goto err;
> +
> +    if (!do_xs_write_dom(xsh, t, domid, "control", "")) goto err;
> +    if (!do_xs_write_dom(xsh, t, domid, "control/shutdown", "")) goto err;
> +    if (!do_xs_write_dom(xsh, t, domid, "control/feature-poweroff", "1")) goto err;
> +    if (!do_xs_write_dom(xsh, t, domid, "control/feature-reboot", "1")) goto err;
> +    if (!do_xs_write_dom(xsh, t, domid, "control/feature-suspend", "")) goto err;
> +    if (!do_xs_write_dom(xsh, t, domid, "control/sysrq", "")) goto err;
> +    if (!do_xs_write_dom(xsh, t, domid, "control/platform-feature-multiprocessor-suspend", "1")) goto err;
> +    if (!do_xs_write_dom(xsh, t, domid, "control/platform-feature-xs_reset_watches", "1")) goto err;
> +
> +    if (!do_xs_write_dom(xsh, t, domid, "domid", id_str)) goto err;
> +    if (!do_xs_write_dom(xsh, t, domid, "data", "")) goto err;
> +    if (!do_xs_write_dom(xsh, t, domid, "drivers", "")) goto err;
> +    if (!do_xs_write_dom(xsh, t, domid, "feature", "")) goto err;
> +    if (!do_xs_write_dom(xsh, t, domid, "attr", "")) goto err;
> +
> +    if (!do_xs_write_dom(xsh, t, domid, "store/port", xenstore_port_str)) goto err;
> +    if (!do_xs_write_dom(xsh, t, domid, "store/ring-ref", ring_ref_str)) goto err;
> +
> +    if (!do_xs_write_libxl(xsh, t, domid, "type", "pvh")) goto err;
> +    if (!do_xs_write_libxl(xsh, t, domid, "dm-version", "qemu_xen")) goto err;
> +
> +    if (!xs_transaction_end(xsh, t, false)) {
> +        if (errno == EAGAIN)
> +            goto retry_transaction;
> +        else
> +            return -errno;
> +    }
> +
> +    return 0;
> +
> +err:
> +    xs_transaction_end(xsh, t, true);
> +    return rc;
> +}
> +
> +static int init_domain(struct xs_handle *xsh,
> +                       struct xc_interface_core *xch,
> +                       xenforeignmemory_handle *xfh,
> +                       libxl_dominfo *info)
> +{
> +    libxl_uuid uuid;
> +    uint64_t xenstore_evtchn, xenstore_pfn;
> +    int rc;
> +    struct xenstore_domain_interface *intf;
> +
> +    printf("Init dom0less domain: %u\n", info->domid);
> +
> +    rc = xc_hvm_param_get(xch, info->domid, HVM_PARAM_STORE_EVTCHN,
> +                          &xenstore_evtchn);
> +    if (rc != 0) {
> +        printf("Failed to get HVM_PARAM_STORE_EVTCHN\n");
> +        return 1;
> +    }
> +
> +    /* Alloc xenstore page */
> +    if (alloc_xs_page(xch, info, &xenstore_pfn) != 0) {
> +        printf("Error on alloc magic pages\n");
> +        return 1;
> +    }
> +
> +    intf = xenforeignmemory_map(xfh, info->domid, XS_READ | XS_WRITE, 1,

I don't think you want to pass the xenstore wire commands here. Did you mean
PROT_READ | PROT_WRITE?

> +                                &xenstore_pfn, NULL);
> +    if (!intf) {
> +        printf("Error mapping xenstore page\n");
> +        return 1;
> +    }
> +    intf->connection = XENSTORE_RECONNECT;
> +    xenforeignmemory_unmap(xfh, intf, 1);
> +
> +    rc = xc_dom_gnttab_seed(xch, info->domid, true,
> +                            (xen_pfn_t)-1, xenstore_pfn, 0, 0);

So no support for Xenstore running in a stub-domain?


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH v6 2/7] xen/arm: implement domU extended regions

[Oleksandr]

On 05.05.22 03:16, Stefano Stabellini wrote:

Hello Stefano


> From: Stefano Stabellini <stefano.stabellini@xilinx.com>
>
> Implement extended regions for dom0less domUs. The implementation is
> based on the libxl implementation.
>
> Also update docs for the ext_regions command line option.
>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
> Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>
> CC: olekstysh@gmail.com


Reviewed-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>


Thanks!


> ---
> Changes in v6:
> - add reviewed-by
> - address 2 NITs
> - update docs
>
> Changes in v5:
> - print the domain
> - coding style
> - simplify the code in find_domU_holes
> - return error if no regions allocated in find_domU_holes
> - use ROUNDUP
> - uint64_t/paddr_t
> ---
>   docs/misc/xen-command-line.pandoc |  9 ++---
>   xen/arch/arm/domain_build.c       | 60 ++++++++++++++++++++++++++-----
>   2 files changed, 56 insertions(+), 13 deletions(-)
>
> diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
> index 1dc7e1ca07..881fe409ac 100644
> --- a/docs/misc/xen-command-line.pandoc
> +++ b/docs/misc/xen-command-line.pandoc
> @@ -1110,11 +1110,12 @@ to use the default.
>   
>   > Default : `true`
>   
> -Flag to enable or disable support for extended regions for Dom0.
> +Flag to enable or disable support for extended regions for Dom0 and
> +Dom0less DomUs.
>   
> -Extended regions are ranges of unused address space exposed to Dom0 as
> -"safe to use" for special memory mappings. Disable if your board device
> -tree is incomplete.
> +Extended regions are ranges of unused address space exposed to the guest
> +as "safe to use" for special memory mappings. Disable if your board
> +device tree is incomplete.
>   
>   ### flask
>   > `= permissive | enforcing | late | disabled`
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 1472ca4972..f22450b4b7 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -35,7 +35,10 @@
>   static unsigned int __initdata opt_dom0_max_vcpus;
>   integer_param("dom0_max_vcpus", opt_dom0_max_vcpus);
>   
> -/* If true, the extended regions support is enabled for dom0 */
> +/*
> + * If true, the extended regions support is enabled for dom0 and
> + * dom0less domUs.
> + */
>   static bool __initdata opt_ext_regions = true;
>   boolean_param("ext_regions", opt_ext_regions);
>   
> @@ -1324,6 +1327,36 @@ out:
>       return res;
>   }
>   
> +static int __init find_domU_holes(const struct kernel_info *kinfo,
> +                                  struct meminfo *ext_regions)
> +{
> +    unsigned int i;
> +    paddr_t bankend;
> +    const paddr_t bankbase[] = GUEST_RAM_BANK_BASES;
> +    const paddr_t banksize[] = GUEST_RAM_BANK_SIZES;
> +    int res = -ENOENT;
> +
> +    for ( i = 0; i < GUEST_RAM_BANKS; i++ )
> +    {
> +        struct membank *ext_bank = &(ext_regions->bank[ext_regions->nr_banks]);
> +
> +        ext_bank->start = ROUNDUP(bankbase[i] + kinfo->mem.bank[i].size, SZ_2M);
> +
> +        bankend = ~0ULL >> (64 - p2m_ipa_bits);
> +        bankend = min(bankend, bankbase[i] + banksize[i] - 1);
> +        if ( bankend > ext_bank->start )
> +            ext_bank->size = bankend - ext_bank->start + 1;
> +
> +        /* 64MB is the minimum size of an extended region */
> +        if ( ext_bank->size < MB(64) )
> +            continue;
> +        ext_regions->nr_banks++;
> +        res = 0;
> +    }
> +
> +    return res;
> +}
> +
>   static int __init make_hypervisor_node(struct domain *d,
>                                          const struct kernel_info *kinfo,
>                                          int addrcells, int sizecells)
> @@ -1360,12 +1393,13 @@ static int __init make_hypervisor_node(struct domain *d,
>   
>       if ( !opt_ext_regions )
>       {
> -        printk(XENLOG_INFO "The extended regions support is disabled\n");
> +        printk(XENLOG_INFO "%pd: extended regions support is disabled\n", d);
>           nr_ext_regions = 0;
>       }
>       else if ( is_32bit_domain(d) )
>       {
> -        printk(XENLOG_WARNING "The extended regions are only supported for 64-bit guest currently\n");
> +        printk(XENLOG_WARNING
> +               "%pd: extended regions not supported for 32-bit guests\n", d);
>           nr_ext_regions = 0;
>       }
>       else
> @@ -1374,13 +1408,21 @@ static int __init make_hypervisor_node(struct domain *d,
>           if ( !ext_regions )
>               return -ENOMEM;
>   
> -        if ( !is_iommu_enabled(d) )
> -            res = find_unallocated_memory(kinfo, ext_regions);
> +        if ( is_domain_direct_mapped(d) )
> +        {
> +            if ( !is_iommu_enabled(d) )
> +                res = find_unallocated_memory(kinfo, ext_regions);
> +            else
> +                res = find_memory_holes(kinfo, ext_regions);
> +        }
>           else
> -            res = find_memory_holes(kinfo, ext_regions);
> +        {
> +            res = find_domU_holes(kinfo, ext_regions);
> +        }
>   
>           if ( res )
> -            printk(XENLOG_WARNING "Failed to allocate extended regions\n");
> +            printk(XENLOG_WARNING "%pd: failed to allocate extended regions\n",
> +                   d);
>           nr_ext_regions = ext_regions->nr_banks;
>       }
>   
> @@ -1401,8 +1443,8 @@ static int __init make_hypervisor_node(struct domain *d,
>           u64 start = ext_regions->bank[i].start;
>           u64 size = ext_regions->bank[i].size;
>   
> -        printk("Extended region %d: %#"PRIx64"->%#"PRIx64"\n",
> -               i, start, start + size);
> +        printk("%pd: extended region %d: %#"PRIx64"->%#"PRIx64"\n",
> +               d, i, start, start + size);
>   
>           dt_child_set_range(&cells, addrcells, sizecells, start, size);
>       }

-- 
Regards,

Oleksandr Tyshchenko

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

[Stefano Stabellini]

On Thu, 5 May 2022, Juergen Gross wrote:
> On 05.05.22 02:16, 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.
> > 
> > The application sets "connection" to XENSTORE_RECONNECT on the xenstore
> > page before calling xs_introduce_domain to signal that the connection is
> > not ready yet to be used. XENSTORE_RECONNECT is reset soon after by
> > xenstored.
> > 
> > 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>
> > ---
> > Changes in v6:
> > - include xs_wire.h and use its definitions
> > 
> > Changes in v5:
> > - set XS_CONNECTION_STATE_RECONNECTING before xs_introduce_domain
> > 
> > Changes in v4:
> > - only alloc xs page (no other magic pages)
> > - add xenstore permissions
> > - check all return values
> > - rename restore_xenstore to create_xenstore
> > - set target_memkb
> > - set start_time properly
> > - close xs transaction on error
> > - call xc_dom_gnttab_seed instead of xc_dom_gnttab_init
> > - xs_open instead of xs_daemon_open
> > 
> > Changes in v3:
> > - handle xenstore errors
> > - add an in-code comment about xenstore entries
> > - less verbose output
> > - clean-up error path in main
> > 
> > Changes in v2:
> > - do not set HVM_PARAM_STORE_EVTCHN twice
> > - rename restore_xenstore to create_xenstore
> > - increase maxmem
> > 
> > connection reconnecting
> > ---
> >   tools/helpers/Makefile        |  13 ++
> >   tools/helpers/init-dom0less.c | 340 ++++++++++++++++++++++++++++++++++
> >   2 files changed, 353 insertions(+)
> >   create mode 100644 tools/helpers/init-dom0less.c
> > 
> > diff --git a/tools/helpers/Makefile b/tools/helpers/Makefile
> > index 7f6c422440..8d78ab1e90 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) $(LDLIBS_libxenforeignmemory)
> > $(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..bfd5ff0761
> > --- /dev/null
> > +++ b/tools/helpers/init-dom0less.c
> > @@ -0,0 +1,340 @@
> > +#include <stdbool.h>
> > +#include <syslog.h>
> > +#include <stdio.h>
> > +#include <err.h>
> > +#include <stdlib.h>
> > +#include <sys/time.h>
> > +#include <xenstore.h>
> > +#include <xenctrl.h>
> > +#include <xenguest.h>
> > +#include <libxl.h>
> > +#include <xenevtchn.h>
> > +#include <xenforeignmemory.h>
> > +#include <xen/io/xs_wire.h>
> > +
> > +#include "init-dom-json.h"
> > +
> > +#define XENSTORE_PFN_OFFSET 1
> > +#define STR_MAX_LENGTH 64
> > +
> > +static int alloc_xs_page(struct xc_interface_core *xch,
> > +                         libxl_dominfo *info,
> > +                         uint64_t *xenstore_pfn)
> > +{
> > +    int rc;
> > +    const xen_pfn_t base = GUEST_MAGIC_BASE >> XC_PAGE_SHIFT;
> > +    xen_pfn_t p2m = (GUEST_MAGIC_BASE >> XC_PAGE_SHIFT) +
> > XENSTORE_PFN_OFFSET;
> > +
> > +    rc = xc_domain_setmaxmem(xch, info->domid,
> > +                             info->max_memkb + (XC_PAGE_SIZE/1024));
> > +    if (rc < 0)
> > +        return rc;
> > +
> > +    rc = xc_domain_populate_physmap_exact(xch, info->domid, 1, 0, 0, &p2m);
> > +    if (rc < 0)
> > +        return rc;
> > +
> > +    *xenstore_pfn = base + XENSTORE_PFN_OFFSET;
> > +    rc = xc_clear_domain_page(xch, info->domid, *xenstore_pfn);
> > +    if (rc < 0)
> > +        return rc;
> > +
> > +    return 0;
> > +}
> > +
> > +static bool 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];
> > +    struct xs_permissions perms[2];
> > +
> > +    perms[0].id = domid;
> > +    perms[0].perms = XS_PERM_NONE;
> > +    perms[1].id = 0;
> > +    perms[1].perms = XS_PERM_READ;
> > +
> > +    if (snprintf(full_path, STR_MAX_LENGTH,
> > +                 "/local/domain/%u/%s", domid, path) < 0)
> > +        return false;
> > +    if (!xs_write(xsh, t, full_path, val, strlen(val)))
> > +        return false;
> > +    return xs_set_permissions(xsh, t, full_path, perms, 2);
> > +}
> > +
> > +static bool 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];
> > +
> > +    if (snprintf(full_path, STR_MAX_LENGTH,
> > +                 "/libxl/%u/%s", domid, path) < 0)
> > +        return false;
> > +    return xs_write(xsh, t, full_path, val, strlen(val));
> > +}
> > +
> > +static bool 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];
> > +
> > +    if (snprintf(full_path, STR_MAX_LENGTH,
> > +                 "/vm/" LIBXL_UUID_FMT "/%s", LIBXL_UUID_BYTES(uuid), path)
> > < 0)
> > +        return false;
> > +    return xs_write(xsh, t, full_path, val, strlen(val));
> > +}
> > +
> > +/*
> > + * The xenstore nodes are the xenstore nodes libxl writes at domain
> > + * creation.
> > + *
> > + * The list was retrieved by running xenstore-ls on a corresponding
> > + * domain started by xl/libxl.
> > + */
> > +static int create_xenstore(struct xs_handle *xsh,
> > +                           libxl_dominfo *info, libxl_uuid uuid,
> > +                           evtchn_port_t xenstore_port)
> > +{
> > +    domid_t domid;
> > +    unsigned 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 target_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;
> > +    struct timeval start_time;
> > +    char start_time_str[STR_MAX_LENGTH];
> > +    int rc;
> > +
> > +    if (gettimeofday(&start_time, NULL) < 0)
> > +        return -errno;
> > +    rc = snprintf(start_time_str, STR_MAX_LENGTH, "%jd.%02d",
> > +            (intmax_t)start_time.tv_sec, (int)start_time.tv_usec / 10000);
> > +    if (rc < 0)
> > +        return rc;
> > +
> > +    domid = info->domid;
> > +    rc = snprintf(id_str, STR_MAX_LENGTH, "%u", domid);
> > +    if (rc < 0)
> > +        return rc;
> > +    rc = snprintf(dom_name_str, STR_MAX_LENGTH, "dom0less-%u", domid);
> > +    if (rc < 0)
> > +        return rc;
> > +    rc = snprintf(uuid_str, STR_MAX_LENGTH, LIBXL_UUID_FMT,
> > LIBXL_UUID_BYTES(uuid));
> > +    if (rc < 0)
> > +        return rc;
> > +    rc = snprintf(vm_val_str, STR_MAX_LENGTH,
> > +                  "vm/" LIBXL_UUID_FMT, LIBXL_UUID_BYTES(uuid));
> > +    if (rc < 0)
> > +        return rc;
> > +    rc = snprintf(max_memkb_str, STR_MAX_LENGTH, "%lu", info->max_memkb);
> > +    if (rc < 0)
> > +        return rc;
> > +    rc = snprintf(target_memkb_str, STR_MAX_LENGTH, "%lu",
> > info->current_memkb);
> > +    if (rc < 0)
> > +        return rc;
> > +    rc = snprintf(ring_ref_str, STR_MAX_LENGTH, "%lld",
> > +                  (GUEST_MAGIC_BASE >> XC_PAGE_SHIFT) +
> > XENSTORE_PFN_OFFSET);
> > +    if (rc < 0)
> > +        return rc;
> > +    rc = snprintf(xenstore_port_str, STR_MAX_LENGTH, "%u", xenstore_port);
> > +    if (rc < 0)
> > +        return rc;
> > +
> > +retry_transaction:
> > +    t = xs_transaction_start(xsh);
> > +    if (t == XBT_NULL)
> > +        return -errno;
> > +
> > +    rc = -EIO;
> > +    /* /vm */
> > +    if (!do_xs_write_vm(xsh, t, uuid, "name", dom_name_str)) goto err;
> > +    if (!do_xs_write_vm(xsh, t, uuid, "uuid", uuid_str)) goto err;
> > +    if (!do_xs_write_vm(xsh, t, uuid, "start_time", start_time_str)) goto
> > err;
> > +
> > +    /* /domain */
> > +    if (!do_xs_write_dom(xsh, t, domid, "vm", vm_val_str)) goto err;
> > +    if (!do_xs_write_dom(xsh, t, domid, "name", dom_name_str)) goto err;
> > +    if (!do_xs_write_dom(xsh, t, domid, "cpu", "")) goto err;
> > +    for (i = 0; i < info->vcpu_max_id; i++) {
> > +        rc = snprintf(cpu_str, STR_MAX_LENGTH, "cpu/%u/availability/", i);
> > +        if (rc < 0)
> > +            goto err;
> > +        rc = -EIO;
> > +        if (!do_xs_write_dom(xsh, t, domid, cpu_str,
> > +                             (info->cpupool & (1 << i)) ? "online" :
> > "offline"))
> > +            goto err;
> > +    }
> > +
> > +    if (!do_xs_write_dom(xsh, t, domid, "memory", "")) goto err;
> > +    if (!do_xs_write_dom(xsh, t, domid, "memory/static-max",
> > max_memkb_str)) goto err;
> > +    if (!do_xs_write_dom(xsh, t, domid, "memory/target", target_memkb_str))
> > goto err;
> > +    if (!do_xs_write_dom(xsh, t, domid, "memory/videoram", "-1")) goto err;
> > +
> > +    if (!do_xs_write_dom(xsh, t, domid, "device", "")) goto err;
> > +    if (!do_xs_write_dom(xsh, t, domid, "device/suspend", "")) goto err;
> > +    if (!do_xs_write_dom(xsh, t, domid, "device/suspend/event-channel",
> > "")) goto err;
> > +
> > +    if (!do_xs_write_dom(xsh, t, domid, "control", "")) goto err;
> > +    if (!do_xs_write_dom(xsh, t, domid, "control/shutdown", "")) goto err;
> > +    if (!do_xs_write_dom(xsh, t, domid, "control/feature-poweroff", "1"))
> > goto err;
> > +    if (!do_xs_write_dom(xsh, t, domid, "control/feature-reboot", "1"))
> > goto err;
> > +    if (!do_xs_write_dom(xsh, t, domid, "control/feature-suspend", ""))
> > goto err;
> > +    if (!do_xs_write_dom(xsh, t, domid, "control/sysrq", "")) goto err;
> > +    if (!do_xs_write_dom(xsh, t, domid,
> > "control/platform-feature-multiprocessor-suspend", "1")) goto err;
> > +    if (!do_xs_write_dom(xsh, t, domid,
> > "control/platform-feature-xs_reset_watches", "1")) goto err;
> > +
> > +    if (!do_xs_write_dom(xsh, t, domid, "domid", id_str)) goto err;
> > +    if (!do_xs_write_dom(xsh, t, domid, "data", "")) goto err;
> > +    if (!do_xs_write_dom(xsh, t, domid, "drivers", "")) goto err;
> > +    if (!do_xs_write_dom(xsh, t, domid, "feature", "")) goto err;
> > +    if (!do_xs_write_dom(xsh, t, domid, "attr", "")) goto err;
> > +
> > +    if (!do_xs_write_dom(xsh, t, domid, "store/port", xenstore_port_str))
> > goto err;
> > +    if (!do_xs_write_dom(xsh, t, domid, "store/ring-ref", ring_ref_str))
> > goto err;
> > +
> > +    if (!do_xs_write_libxl(xsh, t, domid, "type", "pvh")) goto err;
> > +    if (!do_xs_write_libxl(xsh, t, domid, "dm-version", "qemu_xen")) goto
> > err;
> > +
> > +    if (!xs_transaction_end(xsh, t, false)) {
> > +        if (errno == EAGAIN)
> > +            goto retry_transaction;
> > +        else
> > +            return -errno;
> > +    }
> > +
> > +    return 0;
> > +
> > +err:
> > +    xs_transaction_end(xsh, t, true);
> > +    return rc;
> > +}
> > +
> > +static int init_domain(struct xs_handle *xsh,
> > +                       struct xc_interface_core *xch,
> > +                       xenforeignmemory_handle *xfh,
> > +                       libxl_dominfo *info)
> > +{
> > +    libxl_uuid uuid;
> > +    uint64_t xenstore_evtchn, xenstore_pfn;
> > +    int rc;
> > +    struct xenstore_domain_interface *intf;
> > +
> > +    printf("Init dom0less domain: %u\n", info->domid);
> > +
> > +    rc = xc_hvm_param_get(xch, info->domid, HVM_PARAM_STORE_EVTCHN,
> > +                          &xenstore_evtchn);
> > +    if (rc != 0) {
> > +        printf("Failed to get HVM_PARAM_STORE_EVTCHN\n");
> > +        return 1;
> > +    }
> > +
> > +    /* Alloc xenstore page */
> > +    if (alloc_xs_page(xch, info, &xenstore_pfn) != 0) {
> > +        printf("Error on alloc magic pages\n");
> > +        return 1;
> > +    }
> > +
> > +    intf = xenforeignmemory_map(xfh, info->domid, XS_READ | XS_WRITE, 1,
> 
> I don't think you want to pass the xenstore wire commands here. Did you mean
> PROT_READ | PROT_WRITE?

Ops, you are right. I'll fix that.


> > +                                &xenstore_pfn, NULL);
> > +    if (!intf) {
> > +        printf("Error mapping xenstore page\n");
> > +        return 1;
> > +    }
> > +    intf->connection = XENSTORE_RECONNECT;
> > +    xenforeignmemory_unmap(xfh, intf, 1);
> > +
> > +    rc = xc_dom_gnttab_seed(xch, info->domid, true,
> > +                            (xen_pfn_t)-1, xenstore_pfn, 0, 0);
> 
> So no support for Xenstore running in a stub-domain?

We don't have stub-domains on ARM yet (unfortunately), so we cannot run
xenstore in one. It would not be possible for me to test it at the moment.

Re: [PATCH v6 4/7] xen/arm: configure dom0less domain for enabling xenstore after boot

[Stefano Stabellini]

On Thu, 5 May 2022, Jan Beulich wrote:
> On 05.05.2022 02:16, Stefano Stabellini wrote:
> > From: Luca Miccio <lucmiccio@gmail.com>
> > 
> > Export evtchn_alloc_unbound and make it __must_check.
> > 
> > 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>
> > Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
> > CC: Julien Grall <julien@xen.org>
> > CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
> > CC: Bertrand Marquis <bertrand.marquis@arm.com>
> > CC: jbeulich@suse.com
> 
> Somehow my ack given for v5 was lost?

I'll put it back


> > ---
> > Changes in v5:
> > - merge with "xen: export evtchn_alloc_unbound"
> > - __must_check
> > 
> > Changes in v3:
> > - use evtchn_alloc_unbound
> > 
> > Changes in v2:
> > - set HVM_PARAM_STORE_PFN to ~0ULL at domain creation
> > - in alloc_xenstore_evtchn do not call _evtchn_alloc_unbound
> > 
> > xen: export evtchn_alloc_unbound
> > 
> > It will be used during dom0less domains construction.
> > 
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
> 
> This is somewhat confusing to find in a post-commit-message remark.

Sorry I don't know what happened there

Re: [PATCH v6 4/7] xen/arm: configure dom0less domain for enabling xenstore after boot

[Rahul Singh]

Hi Stefano,

> On 5 May 2022, at 1:16 am, Stefano Stabellini <sstabellini@kernel.org> wrote:
> 
> From: Luca Miccio <lucmiccio@gmail.com>
> 
> Export evtchn_alloc_unbound and make it __must_check.
> 
> 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>
> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
> CC: Julien Grall <julien@xen.org>
> CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
> CC: Bertrand Marquis <bertrand.marquis@arm.com>
> CC: jbeulich@suse.com
> 
> ---
> Changes in v5:
> - merge with "xen: export evtchn_alloc_unbound"
> - __must_check
> 
> Changes in v3:
> - use evtchn_alloc_unbound
> 
> Changes in v2:
> - set HVM_PARAM_STORE_PFN to ~0ULL at domain creation
> - in alloc_xenstore_evtchn do not call _evtchn_alloc_unbound
> 
> xen: export evtchn_alloc_unbound
> 
> It will be used during dom0less domains construction.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
> ---
> xen/arch/arm/domain_build.c | 37 +++++++++++++++++++++++++++++++++++++
> xen/common/event_channel.c  |  2 +-
> xen/include/xen/event.h     |  3 +++
> 3 files changed, 41 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 016f56a99f..bb430f2189 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>
> @@ -2810,6 +2811,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;
> @@ -2884,6 +2887,13 @@ static int __init prepare_dtb_domU(struct domain *d, struct kernel_info *kinfo)
>             goto err;
>     }
> 
> +    if ( kinfo->dom0less_enhanced )
> +    {
> +        ret = make_hypervisor_node(d, kinfo, addrcells, sizecells);
> +        if ( ret )
> +            goto err;
> +    }
> +
>     ret = fdt_end_node(kinfo->fdt);
>     if ( ret < 0 )
>         goto err;
> @@ -3150,6 +3160,25 @@ static int __init construct_domain(struct domain *d, struct kernel_info *kinfo)
>     return 0;
> }
> 
> +static int __init alloc_xenstore_evtchn(struct domain *d)
> +{
> +    evtchn_alloc_unbound_t alloc;
> +    int rc;
> +
> +    alloc.dom = d->domain_id;
> +    alloc.remote_dom = hardware_domain->domain_id;

I tried to test the patch series with two dom0less domUs without dom0 and oberved the below error.
This error is because there is no hardware_domain in that case.

(XEN) Data Abort Trap. Syndrome=0x6
(XEN) Walking Hypervisor VA 0x0 on CPU0 via TTBR 0x00000000f91f5000
(XEN) 0TH[0x0] = 0x00000000f91f4f7f
(XEN) 1ST[0x0] = 0x00000000f91f1f7f
(XEN) 2ND[0x0] = 0x0000000000000000
(XEN) CPU0: Unexpected Trap: Data Abort
(XEN) ----[ Xen-4.17-unstable  arm64  debug=y  Not tainted ]----
(XEN) CPU:    0
(XEN) PC:     00000000002e4180 domain_build.c#construct_domU+0xc90/0xd0c
(XEN) LR:     00000000002e4178
(XEN) SP:     000000000031e450
(XEN) CPSR:   0000000060000249 MODE:64-bit EL2h (Hypervisor, handler)
(XEN)      X0: 0000000000000000  X1: 0000000000000000  X2: 0000000000000000
(XEN)      X3: 0000000000000005  X4: 0000000000000000  X5: 0000000000000028
(XEN)      X6: 0000000000000080  X7: fefefefefefeff09  X8: 7f7f7f7f7f7f7f7f
(XEN)      X9: ff6f606c2c68726c X10: 7f7f7f7f7f7f7f7f X11: 0101010101010101
(XEN)     X12: 0000000000000008 X13: 00000000002ca000 X14: 00000000002cb000
(XEN)     X15: 6db6db6db6db6db7 X16: fffffff800000000 X17: 0000000000000001
(XEN)     X18: 0180000000000000 X19: 0000800763a34000 X20: 0000000000000000
(XEN)     X21: 000000000031e4c0 X22: 000000000031e4d0 X23: 0000000000000003
(XEN)     X24: 000000000031fdfc X25: 0000008400000000 X26: 0000000000000021
(XEN)     X27: 0000000000300d08 X28: 00000000003fe97f  FP: 000000000031e450
(XEN) 
(XEN)   VTCR_EL2: 00000000800d3590
(XEN)  VTTBR_EL2: 00000083e3a67000
(XEN) 
(XEN)  SCTLR_EL2: 0000000030cd183d
(XEN)    HCR_EL2: 0000000080000039
(XEN)  TTBR0_EL2: 00000000f91f5000
(XEN) 
(XEN)    ESR_EL2: 0000000096000006
(XEN)  HPFAR_EL2: 0000000000000000
(XEN)    FAR_EL2: 0000000000000000
(XEN) 
(XEN) Xen stack trace from sp=000000000031e450:
(XEN)    000000000031fda0 00000000002e5484 0000800763ff2390 00000000002f1ae0
(XEN)    00000000002c98c8 000000000031fde0 0000000000000003 000000000031fdfc
(XEN)    0000008400000000 0000000000000021 0000000000300d08 00000000003fe97f
(XEN)    00c2010000000000 000000000031e4e0 0000000000000000 00000000040f0000
(XEN)    0000002200000001 0010000000000000 0000020300000000 0000000100000000
(XEN)    00000000000c0000 0000000000000000 0000000000000001 0000800763a34000
(XEN)    0000800763a0c000 0000000000000000 0000000000000001 00000000a0000000
(XEN)    0000000030000000 0000000000000000 0000000000000000 0000000000000000
(XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN) Xen call trace:
(XEN)    [<00000000002e4180>] domain_build.c#construct_domU+0xc90/0xd0c (PC)
(XEN)    [<00000000002e4178>] domain_build.c#construct_domU+0xc88/0xd0c (LR)
(XEN)    [<00000000002e5484>] create_domUs+0xb4/0x1e8
(XEN)    [<00000000002e999c>] start_xen+0xaf0/0xbe8
(XEN)    [<00000000002001a0>] arm64/head.o#primary_switched+0xc/0x1c
(XEN) 
(XEN) 
(XEN) ****************************************
(XEN) Panic on CPU 0:
(XEN) CPU0: Unexpected Trap: Data Abort
(XEN) ****************************************

 
> +    rc = evtchn_alloc_unbound(&alloc);
> +    if ( rc )
> +    {
> +        printk("Failed allocating event channel for domain\n");
> +        return rc;
> +    }
> +
> +    d->arch.hvm.params[HVM_PARAM_STORE_EVTCHN] = alloc.port;
> +
> +    return 0;
> +}
> +
> static int __init construct_domU(struct domain *d,
>                                  const struct dt_device_node *node)
> {
> @@ -3214,6 +3243,14 @@ static int __init construct_domU(struct domain *d,
>     if ( rc < 0 )
>         return rc;
> 
> +    if ( kinfo.dom0less_enhanced )

I think we need to do something like this to fix the error.
 if ( hardware_domain && kinfo.dom0less_enhanced )
{

}


> +    {
> +        rc = alloc_xenstore_evtchn(d);
> +        if ( rc < 0 )
> +            return rc;
> +        d->arch.hvm.params[HVM_PARAM_STORE_PFN] = ~0ULL;
> +    }
> +
>     return rc;
> }
 
Regards,
Rahul

Re: [PATCH v6 4/7] xen/arm: configure dom0less domain for enabling xenstore after boot

[Julien Grall]

Hi Rahul,

On 10/05/2022 17:30, Rahul Singh wrote:
>> +    rc = evtchn_alloc_unbound(&alloc);
>> +    if ( rc )
>> +    {
>> +        printk("Failed allocating event channel for domain\n");
>> +        return rc;
>> +    }
>> +
>> +    d->arch.hvm.params[HVM_PARAM_STORE_EVTCHN] = alloc.port;
>> +
>> +    return 0;
>> +}
>> +
>> static int __init construct_domU(struct domain *d,
>>                                   const struct dt_device_node *node)
>> {
>> @@ -3214,6 +3243,14 @@ static int __init construct_domU(struct domain *d,
>>      if ( rc < 0 )
>>          return rc;
>>
>> +    if ( kinfo.dom0less_enhanced )
> 
> I think we need to do something like this to fix the error.
>   if ( hardware_domain && kinfo.dom0less_enhanced )
> {
> 
> }

Is there any use case to use "dom0less_enhanced" without dom0 (or a 
domain servicing Xenstored)?

If not, then I would consider to forbid this case and return an error.

Cheers,

-- 
Julien Grall

Re: [PATCH v6 4/7] xen/arm: configure dom0less domain for enabling xenstore after boot

[Bertrand Marquis]

Hi Julien,

> On 10 May 2022, at 17:35, Julien Grall <julien@xen.org> wrote:
> 
> Hi Rahul,
> 
> On 10/05/2022 17:30, Rahul Singh wrote:
>>> +    rc = evtchn_alloc_unbound(&alloc);
>>> +    if ( rc )
>>> +    {
>>> +        printk("Failed allocating event channel for domain\n");
>>> +        return rc;
>>> +    }
>>> +
>>> +    d->arch.hvm.params[HVM_PARAM_STORE_EVTCHN] = alloc.port;
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> static int __init construct_domU(struct domain *d,
>>>                                  const struct dt_device_node *node)
>>> {
>>> @@ -3214,6 +3243,14 @@ static int __init construct_domU(struct domain *d,
>>>     if ( rc < 0 )
>>>         return rc;
>>> 
>>> +    if ( kinfo.dom0less_enhanced )
>> I think we need to do something like this to fix the error.
>>  if ( hardware_domain && kinfo.dom0less_enhanced )
>> {
>> }
> 
> Is there any use case to use "dom0less_enhanced" without dom0 (or a domain servicing Xenstored)?
> 

Just being curious here but would it even be possible to have non dom0 domain servicing xenstored ?

> If not, then I would consider to forbid this case and return an error.

One way or an other we need to solve the crash but if it is forbidden we must prevent coming to this step earlier as it means the configuration is wrong.

Cheers
Bertrand

> 
> Cheers,
> 
> -- 
> Julien Grall

Re: [PATCH v6 4/7] xen/arm: configure dom0less domain for enabling xenstore after boot

[Julien Grall]

Hi Bertrand,

On 11/05/2022 08:46, Bertrand Marquis wrote:
>> On 10 May 2022, at 17:35, Julien Grall <julien@xen.org> wrote:
>>
>> Hi Rahul,
>>
>> On 10/05/2022 17:30, Rahul Singh wrote:
>>>> +    rc = evtchn_alloc_unbound(&alloc);
>>>> +    if ( rc )
>>>> +    {
>>>> +        printk("Failed allocating event channel for domain\n");
>>>> +        return rc;
>>>> +    }
>>>> +
>>>> +    d->arch.hvm.params[HVM_PARAM_STORE_EVTCHN] = alloc.port;
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>> static int __init construct_domU(struct domain *d,
>>>>                                   const struct dt_device_node *node)
>>>> {
>>>> @@ -3214,6 +3243,14 @@ static int __init construct_domU(struct domain *d,
>>>>      if ( rc < 0 )
>>>>          return rc;
>>>>
>>>> +    if ( kinfo.dom0less_enhanced )
>>> I think we need to do something like this to fix the error.
>>>   if ( hardware_domain && kinfo.dom0less_enhanced )
>>> {
>>> }
>>
>> Is there any use case to use "dom0less_enhanced" without dom0 (or a domain servicing Xenstored)?
>>
> 
> Just being curious here but would it even be possible to have non dom0 domain servicing xenstored ?

You can build Xenstored against mini-os and configure the init script to 
launch xenstored as a domain.

> 
>> If not, then I would consider to forbid this case and return an error.
> 
> One way or an other we need to solve the crash but if it is forbidden we must prevent coming to this step earlier as it means the configuration is wrong.

I think this should be checked when parsing the configuration.

Cheers,

-- 
Julien Grall

Re: [PATCH v6 4/7] xen/arm: configure dom0less domain for enabling xenstore after boot

[Bertrand Marquis]

> On 11 May 2022, at 09:38, Julien Grall <julien@xen.org> wrote:
> 
> Hi Bertrand,
> 
> On 11/05/2022 08:46, Bertrand Marquis wrote:
>>> On 10 May 2022, at 17:35, Julien Grall <julien@xen.org> wrote:
>>> 
>>> Hi Rahul,
>>> 
>>> On 10/05/2022 17:30, Rahul Singh wrote:
>>>>> +    rc = evtchn_alloc_unbound(&alloc);
>>>>> +    if ( rc )
>>>>> +    {
>>>>> +        printk("Failed allocating event channel for domain\n");
>>>>> +        return rc;
>>>>> +    }
>>>>> +
>>>>> +    d->arch.hvm.params[HVM_PARAM_STORE_EVTCHN] = alloc.port;
>>>>> +
>>>>> +    return 0;
>>>>> +}
>>>>> +
>>>>> static int __init construct_domU(struct domain *d,
>>>>>                                  const struct dt_device_node *node)
>>>>> {
>>>>> @@ -3214,6 +3243,14 @@ static int __init construct_domU(struct domain *d,
>>>>>     if ( rc < 0 )
>>>>>         return rc;
>>>>> 
>>>>> +    if ( kinfo.dom0less_enhanced )
>>>> I think we need to do something like this to fix the error.
>>>>  if ( hardware_domain && kinfo.dom0less_enhanced )
>>>> {
>>>> }
>>> 
>>> Is there any use case to use "dom0less_enhanced" without dom0 (or a domain servicing Xenstored)?
>>> 
>> Just being curious here but would it even be possible to have non dom0 domain servicing xenstored ?
> 
> You can build Xenstored against mini-os and configure the init script to launch xenstored as a domain.

So dom0 is not mandatory or should mini-os be started as Dom0 for this to work ?

> 
>>> If not, then I would consider to forbid this case and return an error.
>> One way or an other we need to solve the crash but if it is forbidden we must prevent coming to this step earlier as it means the configuration is wrong.
> 
> I think this should be checked when parsing the configuration.

If dom0 is mandatory yes, we should still make sure that this code cannot be reached so an ASSERT would be nice here at least in case someone tries to activate this code without dom0 (which might happen when we will push the serie for static event channels).

Cheers
Bertrand

> 
> Cheers,
> 
> -- 
> Julien Grall

Re: [PATCH v6 4/7] xen/arm: configure dom0less domain for enabling xenstore after boot

[Julien Grall]

Hi Bertrand,

On 11/05/2022 09:46, Bertrand Marquis wrote:
> 
> 
>> On 11 May 2022, at 09:38, Julien Grall <julien@xen.org> wrote:
>>
>> Hi Bertrand,
>>
>> On 11/05/2022 08:46, Bertrand Marquis wrote:
>>>> On 10 May 2022, at 17:35, Julien Grall <julien@xen.org> wrote:
>>>>
>>>> Hi Rahul,
>>>>
>>>> On 10/05/2022 17:30, Rahul Singh wrote:
>>>>>> +    rc = evtchn_alloc_unbound(&alloc);
>>>>>> +    if ( rc )
>>>>>> +    {
>>>>>> +        printk("Failed allocating event channel for domain\n");
>>>>>> +        return rc;
>>>>>> +    }
>>>>>> +
>>>>>> +    d->arch.hvm.params[HVM_PARAM_STORE_EVTCHN] = alloc.port;
>>>>>> +
>>>>>> +    return 0;
>>>>>> +}
>>>>>> +
>>>>>> static int __init construct_domU(struct domain *d,
>>>>>>                                   const struct dt_device_node *node)
>>>>>> {
>>>>>> @@ -3214,6 +3243,14 @@ static int __init construct_domU(struct domain *d,
>>>>>>      if ( rc < 0 )
>>>>>>          return rc;
>>>>>>
>>>>>> +    if ( kinfo.dom0less_enhanced )
>>>>> I think we need to do something like this to fix the error.
>>>>>   if ( hardware_domain && kinfo.dom0less_enhanced )
>>>>> {
>>>>> }
>>>>
>>>> Is there any use case to use "dom0less_enhanced" without dom0 (or a domain servicing Xenstored)?
>>>>
>>> Just being curious here but would it even be possible to have non dom0 domain servicing xenstored ?
>>
>> You can build Xenstored against mini-os and configure the init script to launch xenstored as a domain.
> 
> So dom0 is not mandatory or should mini-os be started as Dom0 for this to work ?

In order to allocate the event channel, you need to know the ID of the 
domain where Xenstored will run. Stefano's patch is relying on Xenstored 
to be run in Domain 0.

This would need to be updated if we want to run it in a separate domain.

> 
>>
>>>> If not, then I would consider to forbid this case and return an error.
>>> One way or an other we need to solve the crash but if it is forbidden we must prevent coming to this step earlier as it means the configuration is wrong.
>>
>> I think this should be checked when parsing the configuration.
> 
> If dom0 is mandatory yes, we should still make sure that this code cannot be reached so an ASSERT would be nice here at least in case someone tries to activate this code without dom0 (which might happen when we will push the serie for static event channels).

I am fine with an ASSERT().

Are you saying that dom0less_enhanced will be set to true for the static 
event channel series?

If yes, then I think dom0less_enhanced will need to be an enum so we 
know what part of Xen is exposed.

Cheers,

-- 
Julien Grall

Re: [PATCH v6 4/7] xen/arm: configure dom0less domain for enabling xenstore after boot

[Bertrand Marquis]

Hi Julien,

> On 11 May 2022, at 10:10, Julien Grall <julien@xen.org> wrote:
> 
> Hi Bertrand,
> 
> On 11/05/2022 09:46, Bertrand Marquis wrote:
>>> On 11 May 2022, at 09:38, Julien Grall <julien@xen.org> wrote:
>>> 
>>> Hi Bertrand,
>>> 
>>> On 11/05/2022 08:46, Bertrand Marquis wrote:
>>>>> On 10 May 2022, at 17:35, Julien Grall <julien@xen.org> wrote:
>>>>> 
>>>>> Hi Rahul,
>>>>> 
>>>>> On 10/05/2022 17:30, Rahul Singh wrote:
>>>>>>> + rc = evtchn_alloc_unbound(&alloc);
>>>>>>> + if ( rc )
>>>>>>> + {
>>>>>>> + printk("Failed allocating event channel for domain\n");
>>>>>>> + return rc;
>>>>>>> + }
>>>>>>> +
>>>>>>> + d->arch.hvm.params[HVM_PARAM_STORE_EVTCHN] = alloc.port;
>>>>>>> +
>>>>>>> + return 0;
>>>>>>> +}
>>>>>>> +
>>>>>>> static int __init construct_domU(struct domain *d,
>>>>>>> const struct dt_device_node *node)
>>>>>>> {
>>>>>>> @@ -3214,6 +3243,14 @@ static int __init construct_domU(struct domain *d,
>>>>>>> if ( rc < 0 )
>>>>>>> return rc;
>>>>>>> 
>>>>>>> + if ( kinfo.dom0less_enhanced )
>>>>>> I think we need to do something like this to fix the error.
>>>>>> if ( hardware_domain && kinfo.dom0less_enhanced )
>>>>>> {
>>>>>> }
>>>>> 
>>>>> Is there any use case to use "dom0less_enhanced" without dom0 (or a domain servicing Xenstored)?
>>>>> 
>>>> Just being curious here but would it even be possible to have non dom0 domain servicing xenstored ?
>>> 
>>> You can build Xenstored against mini-os and configure the init script to launch xenstored as a domain.
>> So dom0 is not mandatory or should mini-os be started as Dom0 for this to work ?
> 
> In order to allocate the event channel, you need to know the ID of the domain where Xenstored will run. Stefano's patch is relying on Xenstored to be run in Domain 0.
> 
> This would need to be updated if we want to run it in a separate domain.

Ok then Dom0 is mandatory at the moment, I am ok with that.

> 
>>> 
>>>>> If not, then I would consider to forbid this case and return an error.
>>>> One way or an other we need to solve the crash but if it is forbidden we must prevent coming to this step earlier as it means the configuration is wrong.
>>> 
>>> I think this should be checked when parsing the configuration.
>> If dom0 is mandatory yes, we should still make sure that this code cannot be reached so an ASSERT would be nice here at least in case someone tries to activate this code without dom0 (which might happen when we will push the serie for static event channels).
> 
> I am fine with an ASSERT().
> 
> Are you saying that dom0less_enhanced will be set to true for the static event channel series?
> 
> If yes, then I think dom0less_enhanced will need to be an enum so we know what part of Xen is exposed.

No it won’t, we just need some of the changes done but without setting dom0less_enhanced.
@Rahul: can you confirm.

Cheers
Bertrand

> 
> Cheers,
> 
> -- 
> Julien Grall

Re: [PATCH v6 4/7] xen/arm: configure dom0less domain for enabling xenstore after boot

[Rahul Singh]

Hi Julien

> On 11 May 2022, at 10:18 am, Bertrand Marquis <Bertrand.Marquis@arm.com> wrote:
> 
> Hi Julien,
> 
>> On 11 May 2022, at 10:10, Julien Grall <julien@xen.org> wrote:
>> 
>> Hi Bertrand,
>> 
>> On 11/05/2022 09:46, Bertrand Marquis wrote:
>>>> On 11 May 2022, at 09:38, Julien Grall <julien@xen.org> wrote:
>>>> 
>>>> Hi Bertrand,
>>>> 
>>>> On 11/05/2022 08:46, Bertrand Marquis wrote:
>>>>>> On 10 May 2022, at 17:35, Julien Grall <julien@xen.org> wrote:
>>>>>> 
>>>>>> Hi Rahul,
>>>>>> 
>>>>>> On 10/05/2022 17:30, Rahul Singh wrote:
>>>>>>>> + rc = evtchn_alloc_unbound(&alloc);
>>>>>>>> + if ( rc )
>>>>>>>> + {
>>>>>>>> + printk("Failed allocating event channel for domain\n");
>>>>>>>> + return rc;
>>>>>>>> + }
>>>>>>>> +
>>>>>>>> + d->arch.hvm.params[HVM_PARAM_STORE_EVTCHN] = alloc.port;
>>>>>>>> +
>>>>>>>> + return 0;
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> static int __init construct_domU(struct domain *d,
>>>>>>>> const struct dt_device_node *node)
>>>>>>>> {
>>>>>>>> @@ -3214,6 +3243,14 @@ static int __init construct_domU(struct domain *d,
>>>>>>>> if ( rc < 0 )
>>>>>>>> return rc;
>>>>>>>> 
>>>>>>>> + if ( kinfo.dom0less_enhanced )
>>>>>>> I think we need to do something like this to fix the error.
>>>>>>> if ( hardware_domain && kinfo.dom0less_enhanced )
>>>>>>> {
>>>>>>> }
>>>>>> 
>>>>>> Is there any use case to use "dom0less_enhanced" without dom0 (or a domain servicing Xenstored)?
>>>>>> 
>>>>> Just being curious here but would it even be possible to have non dom0 domain servicing xenstored ?
>>>> 
>>>> You can build Xenstored against mini-os and configure the init script to launch xenstored as a domain.
>>> So dom0 is not mandatory or should mini-os be started as Dom0 for this to work ?
>> 
>> In order to allocate the event channel, you need to know the ID of the domain where Xenstored will run. Stefano's patch is relying on Xenstored to be run in Domain 0.
>> 
>> This would need to be updated if we want to run it in a separate domain.
> 
> Ok then Dom0 is mandatory at the moment, I am ok with that.
> 
>> 
>>>> 
>>>>>> If not, then I would consider to forbid this case and return an error.
>>>>> One way or an other we need to solve the crash but if it is forbidden we must prevent coming to this step earlier as it means the configuration is wrong.
>>>> 
>>>> I think this should be checked when parsing the configuration.
>>> If dom0 is mandatory yes, we should still make sure that this code cannot be reached so an ASSERT would be nice here at least in case someone tries to activate this code without dom0 (which might happen when we will push the serie for static event channels).
>> 
>> I am fine with an ASSERT().
>> 
>> Are you saying that dom0less_enhanced will be set to true for the static event channel series?
>> 
>> If yes, then I think dom0less_enhanced will need to be an enum so we know what part of Xen is exposed.
> 
> No it won’t, we just need some of the changes done but without setting dom0less_enhanced.
> @Rahul: can you confirm.
> 

We need to set the "xen,enhanced” enabled for dom0less domU to enable
the event-channel interface in dom0less guest. If we did not set this property we can’t
use the event-channel interface in dom0less domUs guests.

Regards,
Rahul
> Cheers
> Bertrand
> 
>> 
>> Cheers,
>> 
>> --
>> Julien Grall

Re: [PATCH v6 4/7] xen/arm: configure dom0less domain for enabling xenstore after boot

[Julien Grall]

Hi Rahul,

On 11/05/2022 11:53, Rahul Singh wrote:
>> On 11 May 2022, at 10:18 am, Bertrand Marquis <Bertrand.Marquis@arm.com> wrote:
>>
>> Hi Julien,
>>
>>> On 11 May 2022, at 10:10, Julien Grall <julien@xen.org> wrote:
>>>
>>> Hi Bertrand,
>>>
>>> On 11/05/2022 09:46, Bertrand Marquis wrote:
>>>>> On 11 May 2022, at 09:38, Julien Grall <julien@xen.org> wrote:
>>>>>
>>>>> Hi Bertrand,
>>>>>
>>>>> On 11/05/2022 08:46, Bertrand Marquis wrote:
>>>>>>> On 10 May 2022, at 17:35, Julien Grall <julien@xen.org> wrote:
>>>>>>>
>>>>>>> Hi Rahul,
>>>>>>>
>>>>>>> On 10/05/2022 17:30, Rahul Singh wrote:
>>>>>>>>> + rc = evtchn_alloc_unbound(&alloc);
>>>>>>>>> + if ( rc )
>>>>>>>>> + {
>>>>>>>>> + printk("Failed allocating event channel for domain\n");
>>>>>>>>> + return rc;
>>>>>>>>> + }
>>>>>>>>> +
>>>>>>>>> + d->arch.hvm.params[HVM_PARAM_STORE_EVTCHN] = alloc.port;
>>>>>>>>> +
>>>>>>>>> + return 0;
>>>>>>>>> +}
>>>>>>>>> +
>>>>>>>>> static int __init construct_domU(struct domain *d,
>>>>>>>>> const struct dt_device_node *node)
>>>>>>>>> {
>>>>>>>>> @@ -3214,6 +3243,14 @@ static int __init construct_domU(struct domain *d,
>>>>>>>>> if ( rc < 0 )
>>>>>>>>> return rc;
>>>>>>>>>
>>>>>>>>> + if ( kinfo.dom0less_enhanced )
>>>>>>>> I think we need to do something like this to fix the error.
>>>>>>>> if ( hardware_domain && kinfo.dom0less_enhanced )
>>>>>>>> {
>>>>>>>> }
>>>>>>>
>>>>>>> Is there any use case to use "dom0less_enhanced" without dom0 (or a domain servicing Xenstored)?
>>>>>>>
>>>>>> Just being curious here but would it even be possible to have non dom0 domain servicing xenstored ?
>>>>>
>>>>> You can build Xenstored against mini-os and configure the init script to launch xenstored as a domain.
>>>> So dom0 is not mandatory or should mini-os be started as Dom0 for this to work ?
>>>
>>> In order to allocate the event channel, you need to know the ID of the domain where Xenstored will run. Stefano's patch is relying on Xenstored to be run in Domain 0.
>>>
>>> This would need to be updated if we want to run it in a separate domain.
>>
>> Ok then Dom0 is mandatory at the moment, I am ok with that.
>>
>>>
>>>>>
>>>>>>> If not, then I would consider to forbid this case and return an error.
>>>>>> One way or an other we need to solve the crash but if it is forbidden we must prevent coming to this step earlier as it means the configuration is wrong.
>>>>>
>>>>> I think this should be checked when parsing the configuration.
>>>> If dom0 is mandatory yes, we should still make sure that this code cannot be reached so an ASSERT would be nice here at least in case someone tries to activate this code without dom0 (which might happen when we will push the serie for static event channels).
>>>
>>> I am fine with an ASSERT().
>>>
>>> Are you saying that dom0less_enhanced will be set to true for the static event channel series?
>>>
>>> If yes, then I think dom0less_enhanced will need to be an enum so we know what part of Xen is exposed.
>>
>> No it won’t, we just need some of the changes done but without setting dom0less_enhanced.
>> @Rahul: can you confirm.
>>
> 
> We need to set the "xen,enhanced” enabled for dom0less domU to enable
> the event-channel interface in dom0less guest. If we did not set this property we can’t
> use the event-channel interface in dom0less domUs guests.

Is this because the domU will not know which PPI will be used for 
notification?

The property "xen,enhanced" with an empty string (or with the value 
"enabled") is meant to indicate that PV drivers will be usable in the 
domain.

AFAIU, you are suggesting to change the meaning based on dom0 whether 
has been created. I don't particularly like that because a user may 
spent a while to understand why Xenstored doesn't work.

The current proposal for xen,enhanced allows us to define new values if 
we wanted to only enabled selected interfaces. AFAIU, in your case, you 
only want to expose the event channel interface, so I would create a new 
value to indicate that the event channel interface is exposed. Xen would 
then create only the part for the event channel (i.e. no extended 
regions, grant tables...).

Cheers,

-- 
Julien Grall

Re: [PATCH v6 4/7] xen/arm: configure dom0less domain for enabling xenstore after boot

[Rahul Singh]

Hi Julien,

> On 11 May 2022, at 2:11 pm, Julien Grall <julien@xen.org> wrote:
> 
> Hi Rahul,
> 
> On 11/05/2022 11:53, Rahul Singh wrote:
>>> On 11 May 2022, at 10:18 am, Bertrand Marquis <Bertrand.Marquis@arm.com> wrote:
>>> 
>>> Hi Julien,
>>> 
>>>> On 11 May 2022, at 10:10, Julien Grall <julien@xen.org> wrote:
>>>> 
>>>> Hi Bertrand,
>>>> 
>>>> On 11/05/2022 09:46, Bertrand Marquis wrote:
>>>>>> On 11 May 2022, at 09:38, Julien Grall <julien@xen.org> wrote:
>>>>>> 
>>>>>> Hi Bertrand,
>>>>>> 
>>>>>> On 11/05/2022 08:46, Bertrand Marquis wrote:
>>>>>>>> On 10 May 2022, at 17:35, Julien Grall <julien@xen.org> wrote:
>>>>>>>> 
>>>>>>>> Hi Rahul,
>>>>>>>> 
>>>>>>>> On 10/05/2022 17:30, Rahul Singh wrote:
>>>>>>>>>> + rc = evtchn_alloc_unbound(&alloc);
>>>>>>>>>> + if ( rc )
>>>>>>>>>> + {
>>>>>>>>>> + printk("Failed allocating event channel for domain\n");
>>>>>>>>>> + return rc;
>>>>>>>>>> + }
>>>>>>>>>> +
>>>>>>>>>> + d->arch.hvm.params[HVM_PARAM_STORE_EVTCHN] = alloc.port;
>>>>>>>>>> +
>>>>>>>>>> + return 0;
>>>>>>>>>> +}
>>>>>>>>>> +
>>>>>>>>>> static int __init construct_domU(struct domain *d,
>>>>>>>>>> const struct dt_device_node *node)
>>>>>>>>>> {
>>>>>>>>>> @@ -3214,6 +3243,14 @@ static int __init construct_domU(struct domain *d,
>>>>>>>>>> if ( rc < 0 )
>>>>>>>>>> return rc;
>>>>>>>>>> 
>>>>>>>>>> + if ( kinfo.dom0less_enhanced )
>>>>>>>>> I think we need to do something like this to fix the error.
>>>>>>>>> if ( hardware_domain && kinfo.dom0less_enhanced )
>>>>>>>>> {
>>>>>>>>> }
>>>>>>>> 
>>>>>>>> Is there any use case to use "dom0less_enhanced" without dom0 (or a domain servicing Xenstored)?
>>>>>>>> 
>>>>>>> Just being curious here but would it even be possible to have non dom0 domain servicing xenstored ?
>>>>>> 
>>>>>> You can build Xenstored against mini-os and configure the init script to launch xenstored as a domain.
>>>>> So dom0 is not mandatory or should mini-os be started as Dom0 for this to work ?
>>>> 
>>>> In order to allocate the event channel, you need to know the ID of the domain where Xenstored will run. Stefano's patch is relying on Xenstored to be run in Domain 0.
>>>> 
>>>> This would need to be updated if we want to run it in a separate domain.
>>> 
>>> Ok then Dom0 is mandatory at the moment, I am ok with that.
>>> 
>>>> 
>>>>>> 
>>>>>>>> If not, then I would consider to forbid this case and return an error.
>>>>>>> One way or an other we need to solve the crash but if it is forbidden we must prevent coming to this step earlier as it means the configuration is wrong.
>>>>>> 
>>>>>> I think this should be checked when parsing the configuration.
>>>>> If dom0 is mandatory yes, we should still make sure that this code cannot be reached so an ASSERT would be nice here at least in case someone tries to activate this code without dom0 (which might happen when we will push the serie for static event channels).
>>>> 
>>>> I am fine with an ASSERT().
>>>> 
>>>> Are you saying that dom0less_enhanced will be set to true for the static event channel series?
>>>> 
>>>> If yes, then I think dom0less_enhanced will need to be an enum so we know what part of Xen is exposed.
>>> 
>>> No it won’t, we just need some of the changes done but without setting dom0less_enhanced.
>>> @Rahul: can you confirm.
>>> 
>> We need to set the "xen,enhanced” enabled for dom0less domU to enable
>> the event-channel interface in dom0less guest. If we did not set this property we can’t
>> use the event-channel interface in dom0less domUs guests.
> 
> Is this because the domU will not know which PPI will be used for notification?

Yes you are right if we don’t use "xen,enhanced” for domUs, domUs will not know which PPI will be used.
Also if we don’t use "xen,enhanced”  there is no hypervisor node created for Linux and if there is
no hypervisor node that means no xen support detected.

> 
> The property "xen,enhanced" with an empty string (or with the value "enabled") is meant to indicate that PV drivers will be usable in the domain.
> 
> AFAIU, you are suggesting to change the meaning based on dom0 whether has been created. I don't particularly like that because a user may spent a while to understand why Xenstored doesn't work.
> 
> The current proposal for xen,enhanced allows us to define new values if we wanted to only enabled selected interfaces. AFAIU, in your case, you only want to expose the event channel interface, so I would create a new value to indicate that the event channel interface is exposed. Xen would then create only the part for the event channel (i.e. no extended regions, grant tables...).

Ok. I will create the new property something like “xen,evtchn” to enable the event-channel for dom0less guests. 
Based on “xen,evtchn” property I will create the hypervisor node with only PPI interrupt property.

Regards,
Rahul

Re: [PATCH v6 4/7] xen/arm: configure dom0less domain for enabling xenstore after boot

[Rahul Singh]

Hi Julien,

> On 11 May 2022, at 3:57 pm, Rahul Singh <Rahul.Singh@arm.com> wrote:
> 
> Hi Julien,
> 
>> On 11 May 2022, at 2:11 pm, Julien Grall <julien@xen.org> wrote:
>> 
>> Hi Rahul,
>> 
>> On 11/05/2022 11:53, Rahul Singh wrote:
>>>> On 11 May 2022, at 10:18 am, Bertrand Marquis <Bertrand.Marquis@arm.com> wrote:
>>>> 
>>>> Hi Julien,
>>>> 
>>>>> On 11 May 2022, at 10:10, Julien Grall <julien@xen.org> wrote:
>>>>> 
>>>>> Hi Bertrand,
>>>>> 
>>>>> On 11/05/2022 09:46, Bertrand Marquis wrote:
>>>>>>> On 11 May 2022, at 09:38, Julien Grall <julien@xen.org> wrote:
>>>>>>> 
>>>>>>> Hi Bertrand,
>>>>>>> 
>>>>>>> On 11/05/2022 08:46, Bertrand Marquis wrote:
>>>>>>>>> On 10 May 2022, at 17:35, Julien Grall <julien@xen.org> wrote:
>>>>>>>>> 
>>>>>>>>> Hi Rahul,
>>>>>>>>> 
>>>>>>>>> On 10/05/2022 17:30, Rahul Singh wrote:
>>>>>>>>>>> + rc = evtchn_alloc_unbound(&alloc);
>>>>>>>>>>> + if ( rc )
>>>>>>>>>>> + {
>>>>>>>>>>> + printk("Failed allocating event channel for domain\n");
>>>>>>>>>>> + return rc;
>>>>>>>>>>> + }
>>>>>>>>>>> +
>>>>>>>>>>> + d->arch.hvm.params[HVM_PARAM_STORE_EVTCHN] = alloc.port;
>>>>>>>>>>> +
>>>>>>>>>>> + return 0;
>>>>>>>>>>> +}
>>>>>>>>>>> +
>>>>>>>>>>> static int __init construct_domU(struct domain *d,
>>>>>>>>>>> const struct dt_device_node *node)
>>>>>>>>>>> {
>>>>>>>>>>> @@ -3214,6 +3243,14 @@ static int __init construct_domU(struct domain *d,
>>>>>>>>>>> if ( rc < 0 )
>>>>>>>>>>> return rc;
>>>>>>>>>>> 
>>>>>>>>>>> + if ( kinfo.dom0less_enhanced )
>>>>>>>>>> I think we need to do something like this to fix the error.
>>>>>>>>>> if ( hardware_domain && kinfo.dom0less_enhanced )
>>>>>>>>>> {
>>>>>>>>>> }
>>>>>>>>> 
>>>>>>>>> Is there any use case to use "dom0less_enhanced" without dom0 (or a domain servicing Xenstored)?
>>>>>>>>> 
>>>>>>>> Just being curious here but would it even be possible to have non dom0 domain servicing xenstored ?
>>>>>>> 
>>>>>>> You can build Xenstored against mini-os and configure the init script to launch xenstored as a domain.
>>>>>> So dom0 is not mandatory or should mini-os be started as Dom0 for this to work ?
>>>>> 
>>>>> In order to allocate the event channel, you need to know the ID of the domain where Xenstored will run. Stefano's patch is relying on Xenstored to be run in Domain 0.
>>>>> 
>>>>> This would need to be updated if we want to run it in a separate domain.
>>>> 
>>>> Ok then Dom0 is mandatory at the moment, I am ok with that.
>>>> 
>>>>> 
>>>>>>> 
>>>>>>>>> If not, then I would consider to forbid this case and return an error.
>>>>>>>> One way or an other we need to solve the crash but if it is forbidden we must prevent coming to this step earlier as it means the configuration is wrong.
>>>>>>> 
>>>>>>> I think this should be checked when parsing the configuration.
>>>>>> If dom0 is mandatory yes, we should still make sure that this code cannot be reached so an ASSERT would be nice here at least in case someone tries to activate this code without dom0 (which might happen when we will push the serie for static event channels).
>>>>> 
>>>>> I am fine with an ASSERT().
>>>>> 
>>>>> Are you saying that dom0less_enhanced will be set to true for the static event channel series?
>>>>> 
>>>>> If yes, then I think dom0less_enhanced will need to be an enum so we know what part of Xen is exposed.
>>>> 
>>>> No it won’t, we just need some of the changes done but without setting dom0less_enhanced.
>>>> @Rahul: can you confirm.
>>>> 
>>> We need to set the "xen,enhanced” enabled for dom0less domU to enable
>>> the event-channel interface in dom0less guest. If we did not set this property we can’t
>>> use the event-channel interface in dom0less domUs guests.
>> 
>> Is this because the domU will not know which PPI will be used for notification?
> 
> Yes you are right if we don’t use "xen,enhanced” for domUs, domUs will not know which PPI will be used.
> Also if we don’t use "xen,enhanced” there is no hypervisor node created for Linux and if there is
> no hypervisor node that means no xen support detected.
> 
>> 
>> The property "xen,enhanced" with an empty string (or with the value "enabled") is meant to indicate that PV drivers will be usable in the domain.
>> 
>> AFAIU, you are suggesting to change the meaning based on dom0 whether has been created. I don't particularly like that because a user may spent a while to understand why Xenstored doesn't work.
>> 
>> The current proposal for xen,enhanced allows us to define new values if we wanted to only enabled selected interfaces. AFAIU, in your case, you only want to expose the event channel interface, so I would create a new value to indicate that the event channel interface is exposed. Xen would then create only the part for the event channel (i.e. no extended regions, grant tables...).
> 
> Ok. I will create the new property something like “xen,evtchn” to enable the event-channel for dom0less guests. 
> Based on “xen,evtchn” property I will create the hypervisor node with only PPI interrupt property.

If we don’t want to create the new property we can use "xen,enhanced = evtchn” to 
enable the event-channel interfacefor dom0less guests.

Regards,
Rahul

Re: [PATCH v6 4/7] xen/arm: configure dom0less domain for enabling xenstore after boot

[Julien Grall]

On 11/05/2022 16:05, Rahul Singh wrote:
>>> The property "xen,enhanced" with an empty string (or with the value "enabled") is meant to indicate that PV drivers will be usable in the domain.
>>>
>>> AFAIU, you are suggesting to change the meaning based on dom0 whether has been created. I don't particularly like that because a user may spent a while to understand why Xenstored doesn't work.
>>>
>>> The current proposal for xen,enhanced allows us to define new values if we wanted to only enabled selected interfaces. AFAIU, in your case, you only want to expose the event channel interface, so I would create a new value to indicate that the event channel interface is exposed. Xen would then create only the part for the event channel (i.e. no extended regions, grant tables...).
>>
>> Ok. I will create the new property something like “xen,evtchn” to enable the event-channel for dom0less guests.
>> Based on “xen,evtchn” property I will create the hypervisor node with only PPI interrupt property.
> 
> If we don’t want to create the new property we can use "xen,enhanced = evtchn” to
> enable the event-channel interfacefor dom0less guests.

I would prefer the "xen,enhanced = evtchn" because to avoid corner cases 
such as xen,enhanced without xen,evtchn

Cheers,

-- 
Julien Grall

Re: [PATCH v6 2/7] xen/arm: implement domU extended regions

[Julien Grall]

On 05/05/2022 01:16, Stefano Stabellini wrote:
> From: Stefano Stabellini <stefano.stabellini@xilinx.com>
> 
> Implement extended regions for dom0less domUs. The implementation is
> based on the libxl implementation.
> 
> Also update docs for the ext_regions command line option.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
> Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>

Acked-by: Julien Grall <jgrall@amazon.com>

Cheers,

-- 
Julien Grall

Re: [PATCH v6 4/7] xen/arm: configure dom0less domain for enabling xenstore after boot

[Julien Grall]

Hi Stefano,

On 05/05/2022 01:16, Stefano Stabellini wrote:
> From: Luca Miccio <lucmiccio@gmail.com>
> 
> Export evtchn_alloc_unbound and make it __must_check.
> 
> 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>
> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
> CC: Julien Grall <julien@xen.org>
> CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
> CC: Bertrand Marquis <bertrand.marquis@arm.com>
> CC: jbeulich@suse.com
> 
> ---
> Changes in v5:
> - merge with "xen: export evtchn_alloc_unbound"
> - __must_check
> 
> Changes in v3:
> - use evtchn_alloc_unbound
> 
> Changes in v2:
> - set HVM_PARAM_STORE_PFN to ~0ULL at domain creation
> - in alloc_xenstore_evtchn do not call _evtchn_alloc_unbound
> 
> xen: export evtchn_alloc_unbound
> 
> It will be used during dom0less domains construction.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
> ---
>   xen/arch/arm/domain_build.c | 37 +++++++++++++++++++++++++++++++++++++
>   xen/common/event_channel.c  |  2 +-
>   xen/include/xen/event.h     |  3 +++
>   3 files changed, 41 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 016f56a99f..bb430f2189 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>
> @@ -2810,6 +2811,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;
> @@ -2884,6 +2887,13 @@ static int __init prepare_dtb_domU(struct domain *d, struct kernel_info *kinfo)
>               goto err;
>       }
>   
> +    if ( kinfo->dom0less_enhanced )
> +    {
> +        ret = make_hypervisor_node(d, kinfo, addrcells, sizecells);
> +        if ( ret )
> +            goto err;
> +    }
> +
>       ret = fdt_end_node(kinfo->fdt);
>       if ( ret < 0 )
>           goto err;
> @@ -3150,6 +3160,25 @@ static int __init construct_domain(struct domain *d, struct kernel_info *kinfo)
>       return 0;
>   }
>   
> +static int __init alloc_xenstore_evtchn(struct domain *d)
> +{
> +    evtchn_alloc_unbound_t alloc;
> +    int rc;
> +
> +    alloc.dom = d->domain_id;
> +    alloc.remote_dom = hardware_domain->domain_id;
> +    rc = evtchn_alloc_unbound(&alloc);
> +    if ( rc )
> +    {
> +        printk("Failed allocating event channel for domain\n");
> +        return rc;
> +    }
> +
> +    d->arch.hvm.params[HVM_PARAM_STORE_EVTCHN] = alloc.port;
> +
> +    return 0;
> +}
> +
>   static int __init construct_domU(struct domain *d,
>                                    const struct dt_device_node *node)
>   {
> @@ -3214,6 +3243,14 @@ static int __init construct_domU(struct domain *d,
>       if ( rc < 0 )
>           return rc;
>   
> +    if ( kinfo.dom0less_enhanced )
> +    {
> +        rc = alloc_xenstore_evtchn(d);
> +        if ( rc < 0 )
> +            return rc;
> +        d->arch.hvm.params[HVM_PARAM_STORE_PFN] = ~0ULL;

This sounds a little bit odd that we set the value to ~0ULL with 
dom0less_enhanced but keep it to 0 outside of dom0less.

If there are any rationale for that, then I would suggest to add a 
comment. (Can be done on commit).

For the common part:

Acked-by: Julien Grall <jgrall@amazon.com> # common

Cheers,

-- 
Julien Grall

Re: [PATCH v6 5/7] xenstored: send an evtchn notification on introduce_domain

[Julien Grall]

Hi,

On 05/05/2022 01:16, Stefano Stabellini wrote:
> From: Luca Miccio <lucmiccio@gmail.com>
> 
> When xs_introduce_domain is called, send out a notification on the
> xenstore event channel so that any (dom0less) domain waiting for the
> xenstore interface to be ready can continue with the initialization.
> Before sending the notification, clear XENSTORE_RECONNECTING.
> 
> The extra notification is harmless for domains that don't require it.
> 
> In xs_wire.h update the commment on top of XENSTORE_RECONNECTING to
> generalize its meaning to suit the dom0less use-case better.
> 
> Signed-off-by: Luca Miccio <lucmiccio@gmail.com>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
> CC: jgross@suse.com
> CC: anthony.perard@citrix.com
> CC: wl@xen.org
> ---
> If you have better suggestions for the wording in xs_wire.h please
> suggest!
> 
> 
> Changes in v6:
> - use XENSTORE_CONNECTED instead of 0x0
> - update xs_wire.h
> 
> Changes in v5:
> - reset XS_CONNECTION_STATE_RECONNECTING before notifying the domU
> 
> Changes in v2:
> - drop the new late_init parameter
> ---
>   tools/xenstore/xenstored_domain.c | 4 ++++
>   xen/include/public/io/xs_wire.h   | 2 +-

I am not entirely sure this is the right place to mention it. But I 
couldn't find a better one.

The documentation (docs/misc/xenstore-misc.txt) states that the field is 
valid when the server advertised ``Connection State``.

Is there any guarantee the field will be 0 for any previous {C, 
O}xenstored implementation? If not, then I think we need to set the 
feature flag so Linux knows the field can be used.

If yes, then the documentation should be relaxed so an OS knows it can 
safely use the field without checking the feature flag.

>   2 files changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/xenstore/xenstored_domain.c b/tools/xenstore/xenstored_domain.c
> index ae065fcbee..6f34af225c 100644
> --- a/tools/xenstore/xenstored_domain.c
> +++ b/tools/xenstore/xenstored_domain.c
> @@ -493,6 +493,10 @@ static struct domain *introduce_domain(const void *ctx,
>   		/* Now domain belongs to its connection. */
>   		talloc_steal(domain->conn, domain);
>   
> +		/* Notify the domain that xenstore is available */
> +		interface->connection = XENSTORE_CONNECTED;
> +		xenevtchn_notify(xce_handle, domain->port);
> +
>   		if (!is_master_domain && !restore)
>   			fire_watches(NULL, ctx, "@introduceDomain", NULL,
>   				     false, NULL);
> diff --git a/xen/include/public/io/xs_wire.h b/xen/include/public/io/xs_wire.h
> index 953a0050a3..c1ec7c73e3 100644
> --- a/xen/include/public/io/xs_wire.h
> +++ b/xen/include/public/io/xs_wire.h
> @@ -141,7 +141,7 @@ struct xenstore_domain_interface {
>   
>   /* Valid values for the connection field */
>   #define XENSTORE_CONNECTED 0 /* the steady-state */
> -#define XENSTORE_RECONNECT 1 /* guest has initiated a reconnect */
> +#define XENSTORE_RECONNECT 1 /* reconnect in progress */

The definition in the docs needs to be updated.

>   
>   /* Valid values for the error field */
>   #define XENSTORE_ERROR_NONE    0 /* No error */

Cheers,

-- 
Julien Grall

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

[Julien Grall]

Hi Stefano,

On 05/05/2022 01:16, 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.
> 
> The application sets "connection" to XENSTORE_RECONNECT on the xenstore
> page before calling xs_introduce_domain to signal that the connection is
> not ready yet to be used. XENSTORE_RECONNECT is reset soon after by
> xenstored.
> 
> 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>
> ---
> Changes in v6:
> - include xs_wire.h and use its definitions
> 
> Changes in v5:
> - set XS_CONNECTION_STATE_RECONNECTING before xs_introduce_domain
> 
> Changes in v4:
> - only alloc xs page (no other magic pages)
> - add xenstore permissions
> - check all return values
> - rename restore_xenstore to create_xenstore
> - set target_memkb
> - set start_time properly
> - close xs transaction on error
> - call xc_dom_gnttab_seed instead of xc_dom_gnttab_init
> - xs_open instead of xs_daemon_open
> 
> Changes in v3:
> - handle xenstore errors
> - add an in-code comment about xenstore entries
> - less verbose output
> - clean-up error path in main
> 
> Changes in v2:
> - do not set HVM_PARAM_STORE_EVTCHN twice
> - rename restore_xenstore to create_xenstore
> - increase maxmem
> 
> connection reconnecting
> ---
>   tools/helpers/Makefile        |  13 ++
>   tools/helpers/init-dom0less.c | 340 ++++++++++++++++++++++++++++++++++
>   2 files changed, 353 insertions(+)
>   create mode 100644 tools/helpers/init-dom0less.c
> 
> diff --git a/tools/helpers/Makefile b/tools/helpers/Makefile
> index 7f6c422440..8d78ab1e90 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) $(LDLIBS_libxenforeignmemory) $(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..bfd5ff0761
> --- /dev/null
> +++ b/tools/helpers/init-dom0less.c
> @@ -0,0 +1,340 @@
> +#include <stdbool.h>
> +#include <syslog.h>
> +#include <stdio.h>
> +#include <err.h>
> +#include <stdlib.h>
> +#include <sys/time.h>
> +#include <xenstore.h>
> +#include <xenctrl.h>
> +#include <xenguest.h>
> +#include <libxl.h>
> +#include <xenevtchn.h>
> +#include <xenforeignmemory.h>
> +#include <xen/io/xs_wire.h>
> +
> +#include "init-dom-json.h"
> +
> +#define XENSTORE_PFN_OFFSET 1
> +#define STR_MAX_LENGTH 64
> +
> +static int alloc_xs_page(struct xc_interface_core *xch,
> +                         libxl_dominfo *info,
> +                         uint64_t *xenstore_pfn)
> +{
> +    int rc;
> +    const xen_pfn_t base = GUEST_MAGIC_BASE >> XC_PAGE_SHIFT;
> +    xen_pfn_t p2m = (GUEST_MAGIC_BASE >> XC_PAGE_SHIFT) + XENSTORE_PFN_OFFSET;
> +
> +    rc = xc_domain_setmaxmem(xch, info->domid,
> +                             info->max_memkb + (XC_PAGE_SIZE/1024));
> +    if (rc < 0)
> +        return rc;
> +
> +    rc = xc_domain_populate_physmap_exact(xch, info->domid, 1, 0, 0, &p2m);
> +    if (rc < 0)
> +        return rc;
> +
> +    *xenstore_pfn = base + XENSTORE_PFN_OFFSET;
> +    rc = xc_clear_domain_page(xch, info->domid, *xenstore_pfn);
> +    if (rc < 0)
> +        return rc;
> +
> +    return 0;
> +}
> +
> +static bool 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];
> +    struct xs_permissions perms[2];
> +
> +    perms[0].id = domid;
> +    perms[0].perms = XS_PERM_NONE;
> +    perms[1].id = 0;
> +    perms[1].perms = XS_PERM_READ;
> +
> +    if (snprintf(full_path, STR_MAX_LENGTH,
> +                 "/local/domain/%u/%s", domid, path) < 0)
> +        return false;
> +    if (!xs_write(xsh, t, full_path, val, strlen(val)))
> +        return false;
> +    return xs_set_permissions(xsh, t, full_path, perms, 2);
> +}
> +
> +static bool 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];
> +
> +    if (snprintf(full_path, STR_MAX_LENGTH,
> +                 "/libxl/%u/%s", domid, path) < 0)
> +        return false;
> +    return xs_write(xsh, t, full_path, val, strlen(val));
> +}
> +
> +static bool 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];
> +
> +    if (snprintf(full_path, STR_MAX_LENGTH,
> +                 "/vm/" LIBXL_UUID_FMT "/%s", LIBXL_UUID_BYTES(uuid), path) < 0)
> +        return false;
> +    return xs_write(xsh, t, full_path, val, strlen(val));
> +}
> +
> +/*
> + * The xenstore nodes are the xenstore nodes libxl writes at domain
> + * creation.
> + *
> + * The list was retrieved by running xenstore-ls on a corresponding
> + * domain started by xl/libxl.
> + */
> +static int create_xenstore(struct xs_handle *xsh,
> +                           libxl_dominfo *info, libxl_uuid uuid,
> +                           evtchn_port_t xenstore_port)
> +{
> +    domid_t domid;
> +    unsigned 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 target_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;
> +    struct timeval start_time;
> +    char start_time_str[STR_MAX_LENGTH];
> +    int rc;
> +
> +    if (gettimeofday(&start_time, NULL) < 0)
> +        return -errno;
> +    rc = snprintf(start_time_str, STR_MAX_LENGTH, "%jd.%02d",
> +            (intmax_t)start_time.tv_sec, (int)start_time.tv_usec / 10000);
> +    if (rc < 0)
> +        return rc;
> +
> +    domid = info->domid;
> +    rc = snprintf(id_str, STR_MAX_LENGTH, "%u", domid);
> +    if (rc < 0)
> +        return rc;
> +    rc = snprintf(dom_name_str, STR_MAX_LENGTH, "dom0less-%u", domid);
> +    if (rc < 0)
> +        return rc;
> +    rc = snprintf(uuid_str, STR_MAX_LENGTH, LIBXL_UUID_FMT, LIBXL_UUID_BYTES(uuid));
> +    if (rc < 0)
> +        return rc;
> +    rc = snprintf(vm_val_str, STR_MAX_LENGTH,
> +                  "vm/" LIBXL_UUID_FMT, LIBXL_UUID_BYTES(uuid));
> +    if (rc < 0)
> +        return rc;
> +    rc = snprintf(max_memkb_str, STR_MAX_LENGTH, "%lu", info->max_memkb);
> +    if (rc < 0)
> +        return rc;
> +    rc = snprintf(target_memkb_str, STR_MAX_LENGTH, "%lu", info->current_memkb);
> +    if (rc < 0)
> +        return rc;
> +    rc = snprintf(ring_ref_str, STR_MAX_LENGTH, "%lld",
> +                  (GUEST_MAGIC_BASE >> XC_PAGE_SHIFT) + XENSTORE_PFN_OFFSET);
> +    if (rc < 0)
> +        return rc;
> +    rc = snprintf(xenstore_port_str, STR_MAX_LENGTH, "%u", xenstore_port);
> +    if (rc < 0)
> +        return rc;
> +
> +retry_transaction:
> +    t = xs_transaction_start(xsh);
> +    if (t == XBT_NULL)
> +        return -errno;
> +
> +    rc = -EIO;
> +    /* /vm */
> +    if (!do_xs_write_vm(xsh, t, uuid, "name", dom_name_str)) goto err;
> +    if (!do_xs_write_vm(xsh, t, uuid, "uuid", uuid_str)) goto err;
> +    if (!do_xs_write_vm(xsh, t, uuid, "start_time", start_time_str)) goto err;
> +
> +    /* /domain */
> +    if (!do_xs_write_dom(xsh, t, domid, "vm", vm_val_str)) goto err;
> +    if (!do_xs_write_dom(xsh, t, domid, "name", dom_name_str)) goto err;
> +    if (!do_xs_write_dom(xsh, t, domid, "cpu", "")) goto err;
> +    for (i = 0; i < info->vcpu_max_id; i++) {
> +        rc = snprintf(cpu_str, STR_MAX_LENGTH, "cpu/%u/availability/", i);
> +        if (rc < 0)
> +            goto err;
> +        rc = -EIO;
> +        if (!do_xs_write_dom(xsh, t, domid, cpu_str,
> +                             (info->cpupool & (1 << i)) ? "online" : "offline"))
> +            goto err;
> +    }
> +
> +    if (!do_xs_write_dom(xsh, t, domid, "memory", "")) goto err;
> +    if (!do_xs_write_dom(xsh, t, domid, "memory/static-max", max_memkb_str)) goto err;
> +    if (!do_xs_write_dom(xsh, t, domid, "memory/target", target_memkb_str)) goto err;
> +    if (!do_xs_write_dom(xsh, t, domid, "memory/videoram", "-1")) goto err;
> +
> +    if (!do_xs_write_dom(xsh, t, domid, "device", "")) goto err;
> +    if (!do_xs_write_dom(xsh, t, domid, "device/suspend", "")) goto err;
> +    if (!do_xs_write_dom(xsh, t, domid, "device/suspend/event-channel", "")) goto err;
> +
> +    if (!do_xs_write_dom(xsh, t, domid, "control", "")) goto err;
> +    if (!do_xs_write_dom(xsh, t, domid, "control/shutdown", "")) goto err;
> +    if (!do_xs_write_dom(xsh, t, domid, "control/feature-poweroff", "1")) goto err;
> +    if (!do_xs_write_dom(xsh, t, domid, "control/feature-reboot", "1")) goto err;
> +    if (!do_xs_write_dom(xsh, t, domid, "control/feature-suspend", "")) goto err;
> +    if (!do_xs_write_dom(xsh, t, domid, "control/sysrq", "")) goto err;
> +    if (!do_xs_write_dom(xsh, t, domid, "control/platform-feature-multiprocessor-suspend", "1")) goto err;
> +    if (!do_xs_write_dom(xsh, t, domid, "control/platform-feature-xs_reset_watches", "1")) goto err;
> +
> +    if (!do_xs_write_dom(xsh, t, domid, "domid", id_str)) goto err;
> +    if (!do_xs_write_dom(xsh, t, domid, "data", "")) goto err;
> +    if (!do_xs_write_dom(xsh, t, domid, "drivers", "")) goto err;
> +    if (!do_xs_write_dom(xsh, t, domid, "feature", "")) goto err;
> +    if (!do_xs_write_dom(xsh, t, domid, "attr", "")) goto err;
> +
> +    if (!do_xs_write_dom(xsh, t, domid, "store/port", xenstore_port_str)) goto err;
> +    if (!do_xs_write_dom(xsh, t, domid, "store/ring-ref", ring_ref_str)) goto err;
> +
> +    if (!do_xs_write_libxl(xsh, t, domid, "type", "pvh")) goto err;
> +    if (!do_xs_write_libxl(xsh, t, domid, "dm-version", "qemu_xen")) goto err;
> +
> +    if (!xs_transaction_end(xsh, t, false)) {
> +        if (errno == EAGAIN)
> +            goto retry_transaction;
> +        else
> +            return -errno;
> +    }
> +
> +    return 0;
> +
> +err:
> +    xs_transaction_end(xsh, t, true);
> +    return rc;
> +}
> +
> +static int init_domain(struct xs_handle *xsh,
> +                       struct xc_interface_core *xch,
> +                       xenforeignmemory_handle *xfh,
> +                       libxl_dominfo *info)
> +{
> +    libxl_uuid uuid;
> +    uint64_t xenstore_evtchn, xenstore_pfn;
> +    int rc;
> +    struct xenstore_domain_interface *intf;
> +
> +    printf("Init dom0less domain: %u\n", info->domid);
> +
> +    rc = xc_hvm_param_get(xch, info->domid, HVM_PARAM_STORE_EVTCHN,
> +                          &xenstore_evtchn);
> +    if (rc != 0) {
> +        printf("Failed to get HVM_PARAM_STORE_EVTCHN\n");
> +        return 1;
> +    }
> +
> +    /* Alloc xenstore page */
> +    if (alloc_xs_page(xch, info, &xenstore_pfn) != 0) {
> +        printf("Error on alloc magic pages\n");
> +        return 1;
> +    }
> +
> +    intf = xenforeignmemory_map(xfh, info->domid, XS_READ | XS_WRITE, 1,
> +                                &xenstore_pfn, NULL);
> +    if (!intf) {
> +        printf("Error mapping xenstore page\n");
> +        return 1;
> +    }
> +    intf->connection = XENSTORE_RECONNECT;
> +    xenforeignmemory_unmap(xfh, intf, 1);
> +
> +    rc = xc_dom_gnttab_seed(xch, info->domid, true,
> +                            (xen_pfn_t)-1, xenstore_pfn, 0, 0);
> +    if (rc)
> +        err(1, "xc_dom_gnttab_seed");
> +
> +    libxl_uuid_generate(&uuid);
> +    xc_domain_sethandle(xch, info->domid, libxl_uuid_bytearray(&uuid));
> +
> +    rc = gen_stub_json_config(info->domid, &uuid);
> +    if (rc)
> +        err(1, "gen_stub_json_config");
> +
> +    /* Now everything is ready: set HVM_PARAM_STORE_PFN */
> +    rc = xc_hvm_param_set(xch, info->domid, HVM_PARAM_STORE_PFN,
> +                          xenstore_pfn);
> +    if (rc < 0)
> +        return rc;
> +
> +    rc = create_xenstore(xsh, info, uuid, xenstore_evtchn);
> +    if (rc)
> +        err(1, "writing to xenstore");
> +
> +    rc = xs_introduce_domain(xsh, info->domid,
> +            (GUEST_MAGIC_BASE >> XC_PAGE_SHIFT) + XENSTORE_PFN_OFFSET,
> +            xenstore_evtchn);

I might be missing something here. In an ealier version, I pointed out 
that xs_introduce_domain() would fail in the case the dom0less domain 
doesn't have "xen,enhanced".

AFAICT, you agreed that the (part?) of initialization should be skipped. 
But I don't see the change in the code. Regarding the placement, we 
could either fully skip init_domain() or just xs_introduce_domain(). The 
latter might be better so all the domains are listed using xenstore-ls.

So something like below should work:

if ( xenstore_evtchn )
{
     rc = xs_introduce...();
     ...
}


Cheers,

-- 
Julien Grall

Re: [PATCH v6 7/7] docs: document dom0less + PV drivers

[Julien Grall]

Hi,

On 05/05/2022 01:16, Stefano Stabellini wrote:
> From: Stefano Stabellini <stefano.stabellini@xilinx.com>
> 
> Document how to use the feature and how the implementation works.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
> Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>

AFAICT, this match the code in Xen. So:

Acked-by: Julien Grall <jgrall@amazon.com>

> ---
>   docs/features/dom0less.pandoc | 43 ++++++++++++++++++++++++++++++++---
>   1 file changed, 40 insertions(+), 3 deletions(-)
> 
> diff --git a/docs/features/dom0less.pandoc b/docs/features/dom0less.pandoc
> index c9edb529e1..725afa0558 100644
> --- a/docs/features/dom0less.pandoc
> +++ b/docs/features/dom0less.pandoc
> @@ -90,6 +90,46 @@ Otherwise, they may be unusable in Xen (for instance if they are compressed).
>   
>   See docs/misc/arm/device-tree/booting.txt for more information.
>   
> +PV Drivers
> +----------
> +
> +It is possible to use PV drivers with dom0less guests with some
> +restrictions:
> +
> +- dom0less domUs that want to use PV drivers support should have the
> +  "xen,enhanced" property set under their device tree nodes (see
> +  docs/misc/arm/device-tree/booting.txt)
> +- a dom0 must be present (or another domain with enough privileges to
> +  run the toolstack)
> +- after dom0 is booted, the utility "init-dom0less" must be run
> +- do not run "init-dom0less" while creating other guests with xl
> +
> +After the execution of init-dom0less, it is possible to use "xl" to
> +hotplug PV drivers to dom0less guests. E.g. xl network-attach domU.
> +
> +The implementation works as follows:
> +- Xen allocates the xenstore event channel for each dom0less domU that
> +  has the "xen,enhanced" property, and sets HVM_PARAM_STORE_EVTCHN
> +- Xen does *not* allocate the xenstore page and sets HVM_PARAM_STORE_PFN
> +  to ~0ULL (invalid)
> +- Dom0less domU kernels check that HVM_PARAM_STORE_PFN is set to invalid
> +    - Old kernels will continue without xenstore support (Note: some old
> +      buggy kernels might crash because they don't check the validity of
> +      HVM_PARAM_STORE_PFN before using it! Disable "xen,enhanced" in
> +      those cases)
> +    - New kernels will wait for a notification on the xenstore event
> +      channel (HVM_PARAM_STORE_EVTCHN) before continuing with the
> +      initialization
> +- Once dom0 is booted, init-dom0less is executed:
> +    - it allocates the xenstore shared page and sets HVM_PARAM_STORE_PFN
> +    - it calls xs_introduce_domain
> +- Xenstored notices the new domain, initializes interfaces as usual, and
> +  sends an event channel notification to the domain using the xenstore
> +  event channel (HVM_PARAM_STORE_EVTCHN)
> +- The Linux domU kernel receives the event channel notification, checks
> +  HVM_PARAM_STORE_PFN again and continue with the initialization
> +
> +
>   Limitations
>   -----------
>   
> @@ -107,9 +147,6 @@ limitations:
>     information, the GIC version exposed to the domains started by Xen at
>     boot is the same as the native GIC version.
>   
> -- No PV drivers. There is no support for PV devices at the moment. All
> -  devices need to be statically assigned to guests.
> -
>   - Pinning vCPUs of domains started by Xen at boot can be
>     done from the control domain, using `xl vcpu-pin` as usual. It is not
>     currently possible to configure vCPU pinning without a control domain.

-- 
Julien Grall

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

[Stefano Stabellini]

On Wed, 11 May 2022, Julien Grall wrote:
> > +    rc = xs_introduce_domain(xsh, info->domid,
> > +            (GUEST_MAGIC_BASE >> XC_PAGE_SHIFT) + XENSTORE_PFN_OFFSET,
> > +            xenstore_evtchn);
> 
> I might be missing something here. In an ealier version, I pointed out that
> xs_introduce_domain() would fail in the case the dom0less domain doesn't have
> "xen,enhanced".
> 
> AFAICT, you agreed that the (part?) of initialization should be skipped. But I
> don't see the change in the code. Regarding the placement, we could either
> fully skip init_domain() or just xs_introduce_domain(). The latter might be
> better so all the domains are listed using xenstore-ls.
> 
> So something like below should work:
> 
> if ( xenstore_evtchn )
> {
>     rc = xs_introduce...();
>     ...
> }

Yes, good point. xenstore_evtchn could be zero validly (first evtchn is
zero), so instead I'll check on xenstore_pfn to be zero (xenstore_pfn is
0 for non-enhanced domUs.)

Re: [PATCH v6 5/7] xenstored: send an evtchn notification on introduce_domain

[Stefano Stabellini]

On Wed, 11 May 2022, Julien Grall wrote:
> On 05/05/2022 01:16, Stefano Stabellini wrote:
> > From: Luca Miccio <lucmiccio@gmail.com>
> > 
> > When xs_introduce_domain is called, send out a notification on the
> > xenstore event channel so that any (dom0less) domain waiting for the
> > xenstore interface to be ready can continue with the initialization.
> > Before sending the notification, clear XENSTORE_RECONNECTING.
> > 
> > The extra notification is harmless for domains that don't require it.
> > 
> > In xs_wire.h update the commment on top of XENSTORE_RECONNECTING to
> > generalize its meaning to suit the dom0less use-case better.
> > 
> > Signed-off-by: Luca Miccio <lucmiccio@gmail.com>
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
> > CC: jgross@suse.com
> > CC: anthony.perard@citrix.com
> > CC: wl@xen.org
> > ---
> > If you have better suggestions for the wording in xs_wire.h please
> > suggest!
> > 
> > 
> > Changes in v6:
> > - use XENSTORE_CONNECTED instead of 0x0
> > - update xs_wire.h
> > 
> > Changes in v5:
> > - reset XS_CONNECTION_STATE_RECONNECTING before notifying the domU
> > 
> > Changes in v2:
> > - drop the new late_init parameter
> > ---
> >   tools/xenstore/xenstored_domain.c | 4 ++++
> >   xen/include/public/io/xs_wire.h   | 2 +-
> 
> I am not entirely sure this is the right place to mention it. But I couldn't
> find a better one.
> 
> The documentation (docs/misc/xenstore-misc.txt) states that the field is valid
> when the server advertised ``Connection State``.
> 
> Is there any guarantee the field will be 0 for any previous {C, O}xenstored
> implementation? If not, then I think we need to set the feature flag so Linux
> knows the field can be used.
> 
> If yes, then the documentation should be relaxed so an OS knows it can safely
> use the field without checking the feature flag.

The xenstore page is allocated by the toolstack which zeros the page,
*xenstored wouldn't set it, so I think we can assume the field has
always been zero.


> > diff --git a/xen/include/public/io/xs_wire.h
> > b/xen/include/public/io/xs_wire.h
> > index 953a0050a3..c1ec7c73e3 100644
> > --- a/xen/include/public/io/xs_wire.h
> > +++ b/xen/include/public/io/xs_wire.h
> > @@ -141,7 +141,7 @@ struct xenstore_domain_interface {
> >     /* Valid values for the connection field */
> >   #define XENSTORE_CONNECTED 0 /* the steady-state */
> > -#define XENSTORE_RECONNECT 1 /* guest has initiated a reconnect */
> > +#define XENSTORE_RECONNECT 1 /* reconnect in progress */
> 
> The definition in the docs needs to be updated.

I wanted to do that but I am very unfamiliar with the xenstore docs.
Can you point me to the place where I need to change the definition? I
cannot find where XENSTORE_RECONNECT is defined...

Re: [PATCH v6 4/7] xen/arm: configure dom0less domain for enabling xenstore after boot

[Stefano Stabellini]

On Wed, 11 May 2022, Julien Grall wrote:
> > If dom0 is mandatory yes, we should still make sure that this code cannot be
> > reached so an ASSERT would be nice here at least in case someone tries to
> > activate this code without dom0 (which might happen when we will push the
> > serie for static event channels).
> 
> I am fine with an ASSERT().

I added an ASSERT(hardware_domain).

Re: [PATCH v6 5/7] xenstored: send an evtchn notification on introduce_domain

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 2977 bytes --]

On 13.05.22 03:16, Stefano Stabellini wrote:
> On Wed, 11 May 2022, Julien Grall wrote:
>> On 05/05/2022 01:16, Stefano Stabellini wrote:
>>> From: Luca Miccio <lucmiccio@gmail.com>
>>>
>>> When xs_introduce_domain is called, send out a notification on the
>>> xenstore event channel so that any (dom0less) domain waiting for the
>>> xenstore interface to be ready can continue with the initialization.
>>> Before sending the notification, clear XENSTORE_RECONNECTING.
>>>
>>> The extra notification is harmless for domains that don't require it.
>>>
>>> In xs_wire.h update the commment on top of XENSTORE_RECONNECTING to
>>> generalize its meaning to suit the dom0less use-case better.
>>>
>>> Signed-off-by: Luca Miccio <lucmiccio@gmail.com>
>>> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
>>> CC: jgross@suse.com
>>> CC: anthony.perard@citrix.com
>>> CC: wl@xen.org
>>> ---
>>> If you have better suggestions for the wording in xs_wire.h please
>>> suggest!
>>>
>>>
>>> Changes in v6:
>>> - use XENSTORE_CONNECTED instead of 0x0
>>> - update xs_wire.h
>>>
>>> Changes in v5:
>>> - reset XS_CONNECTION_STATE_RECONNECTING before notifying the domU
>>>
>>> Changes in v2:
>>> - drop the new late_init parameter
>>> ---
>>>    tools/xenstore/xenstored_domain.c | 4 ++++
>>>    xen/include/public/io/xs_wire.h   | 2 +-
>>
>> I am not entirely sure this is the right place to mention it. But I couldn't
>> find a better one.
>>
>> The documentation (docs/misc/xenstore-misc.txt) states that the field is valid
>> when the server advertised ``Connection State``.
>>
>> Is there any guarantee the field will be 0 for any previous {C, O}xenstored
>> implementation? If not, then I think we need to set the feature flag so Linux
>> knows the field can be used.
>>
>> If yes, then the documentation should be relaxed so an OS knows it can safely
>> use the field without checking the feature flag.
> 
> The xenstore page is allocated by the toolstack which zeros the page,
> *xenstored wouldn't set it, so I think we can assume the field has
> always been zero.
> 
> 
>>> diff --git a/xen/include/public/io/xs_wire.h
>>> b/xen/include/public/io/xs_wire.h
>>> index 953a0050a3..c1ec7c73e3 100644
>>> --- a/xen/include/public/io/xs_wire.h
>>> +++ b/xen/include/public/io/xs_wire.h
>>> @@ -141,7 +141,7 @@ struct xenstore_domain_interface {
>>>      /* Valid values for the connection field */
>>>    #define XENSTORE_CONNECTED 0 /* the steady-state */
>>> -#define XENSTORE_RECONNECT 1 /* guest has initiated a reconnect */
>>> +#define XENSTORE_RECONNECT 1 /* reconnect in progress */
>>
>> The definition in the docs needs to be updated.
> 
> I wanted to do that but I am very unfamiliar with the xenstore docs.
> Can you point me to the place where I need to change the definition? I
> cannot find where XENSTORE_RECONNECT is defined...
> 

See docs/misc/xenstore-ring.txt


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH v6 4/7] xen/arm: configure dom0less domain for enabling xenstore after boot

[Julien Grall]

Hi Stefano,

On 13/05/2022 02:23, Stefano Stabellini wrote:
> On Wed, 11 May 2022, Julien Grall wrote:
>>> If dom0 is mandatory yes, we should still make sure that this code cannot be
>>> reached so an ASSERT would be nice here at least in case someone tries to
>>> activate this code without dom0 (which might happen when we will push the
>>> serie for static event channels).
>>
>> I am fine with an ASSERT().
> 
> I added an ASSERT(hardware_domain).

Just to clarify and avoid a round trip. The ASSERT() is not sufficient 
here. We also need to forbid the user to set xen,enhanced when the HW 
domain is not NULL, at least until Rahul's pre-allocated event channel 
series.

This check would have to be done when parsing the configuration. The 
ASSERT() would be here to ensure that if someone is reworking the 
parsing, it would get caught nicely rather than a NULL dereference.

Cheers,

-- 
Julien Grall

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

[Julien Grall]

Hi Stefano,

On 13/05/2022 02:09, Stefano Stabellini wrote:
> On Wed, 11 May 2022, Julien Grall wrote:
>>> +    rc = xs_introduce_domain(xsh, info->domid,
>>> +            (GUEST_MAGIC_BASE >> XC_PAGE_SHIFT) + XENSTORE_PFN_OFFSET,
>>> +            xenstore_evtchn);
>>
>> I might be missing something here. In an ealier version, I pointed out that
>> xs_introduce_domain() would fail in the case the dom0less domain doesn't have
>> "xen,enhanced".
>>
>> AFAICT, you agreed that the (part?) of initialization should be skipped. But I
>> don't see the change in the code. Regarding the placement, we could either
>> fully skip init_domain() or just xs_introduce_domain(). The latter might be
>> better so all the domains are listed using xenstore-ls.
>>
>> So something like below should work:
>>
>> if ( xenstore_evtchn )
>> {
>>      rc = xs_introduce...();
>>      ...
>> }
> 
> Yes, good point. xenstore_evtchn could be zero validly (first evtchn is
> zero),

Event channel 0 is always reserved when initialization the event channel 
subsystem (evtchn_init()):

evtchn_from_port(d, 0)->state = ECS_RESERVED;

> so instead I'll check on xenstore_pfn to be zero (xenstore_pfn is
> 0 for non-enhanced domUs.)

I spotted that difference but decided to not comment on it as Linux is 
already considering the values 0 and ~0 as invalid. However, I am not in 
favor on any code to rely on 0 means Xenstore will never be available 
while ~0 means that it might be available.

Anyway, as I wrote above, the event channel 0 is always reserved. So you 
can safely use this value to detect whether we allocated the event 
channel for Xenstore.

Cheers,

-- 
Julien Grall

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

[Stefano Stabellini]

On Fri, 13 May 2022, Julien Grall wrote:
> On 13/05/2022 02:09, Stefano Stabellini wrote:
> > On Wed, 11 May 2022, Julien Grall wrote:
> > > > +    rc = xs_introduce_domain(xsh, info->domid,
> > > > +            (GUEST_MAGIC_BASE >> XC_PAGE_SHIFT) + XENSTORE_PFN_OFFSET,
> > > > +            xenstore_evtchn);
> > > 
> > > I might be missing something here. In an ealier version, I pointed out
> > > that
> > > xs_introduce_domain() would fail in the case the dom0less domain doesn't
> > > have
> > > "xen,enhanced".
> > > 
> > > AFAICT, you agreed that the (part?) of initialization should be skipped.
> > > But I
> > > don't see the change in the code. Regarding the placement, we could either
> > > fully skip init_domain() or just xs_introduce_domain(). The latter might
> > > be
> > > better so all the domains are listed using xenstore-ls.
> > > 
> > > So something like below should work:
> > > 
> > > if ( xenstore_evtchn )
> > > {
> > >      rc = xs_introduce...();
> > >      ...
> > > }
> > 
> > Yes, good point. xenstore_evtchn could be zero validly (first evtchn is
> > zero),
> 
> Event channel 0 is always reserved when initialization the event channel
> subsystem (evtchn_init()):
> 
> evtchn_from_port(d, 0)->state = ECS_RESERVED;
> 
> > so instead I'll check on xenstore_pfn to be zero (xenstore_pfn is
> > 0 for non-enhanced domUs.)
> 
> I spotted that difference but decided to not comment on it as Linux is already
> considering the values 0 and ~0 as invalid. However, I am not in favor on any
> code to rely on 0 means Xenstore will never be available while ~0 means that
> it might be available.
> 
> Anyway, as I wrote above, the event channel 0 is always reserved. So you can
> safely use this value to detect whether we allocated the event channel for
> Xenstore.

Thanks I didn't know that. In that case, using xenstore_evtchn is
better, I'll do that.

Re: [PATCH v6 5/7] xenstored: send an evtchn notification on introduce_domain

[Stefano Stabellini]

On Fri, 13 May 2022, Juergen Gross wrote:
> On 13.05.22 03:16, Stefano Stabellini wrote:
> > On Wed, 11 May 2022, Julien Grall wrote:
> > > On 05/05/2022 01:16, Stefano Stabellini wrote:
> > > > From: Luca Miccio <lucmiccio@gmail.com>
> > > > 
> > > > When xs_introduce_domain is called, send out a notification on the
> > > > xenstore event channel so that any (dom0less) domain waiting for the
> > > > xenstore interface to be ready can continue with the initialization.
> > > > Before sending the notification, clear XENSTORE_RECONNECTING.
> > > > 
> > > > The extra notification is harmless for domains that don't require it.
> > > > 
> > > > In xs_wire.h update the commment on top of XENSTORE_RECONNECTING to
> > > > generalize its meaning to suit the dom0less use-case better.
> > > > 
> > > > Signed-off-by: Luca Miccio <lucmiccio@gmail.com>
> > > > Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
> > > > CC: jgross@suse.com
> > > > CC: anthony.perard@citrix.com
> > > > CC: wl@xen.org
> > > > ---
> > > > If you have better suggestions for the wording in xs_wire.h please
> > > > suggest!
> > > > 
> > > > 
> > > > Changes in v6:
> > > > - use XENSTORE_CONNECTED instead of 0x0
> > > > - update xs_wire.h
> > > > 
> > > > Changes in v5:
> > > > - reset XS_CONNECTION_STATE_RECONNECTING before notifying the domU
> > > > 
> > > > Changes in v2:
> > > > - drop the new late_init parameter
> > > > ---
> > > >    tools/xenstore/xenstored_domain.c | 4 ++++
> > > >    xen/include/public/io/xs_wire.h   | 2 +-
> > > 
> > > I am not entirely sure this is the right place to mention it. But I
> > > couldn't
> > > find a better one.
> > > 
> > > The documentation (docs/misc/xenstore-misc.txt) states that the field is
> > > valid
> > > when the server advertised ``Connection State``.
> > > 
> > > Is there any guarantee the field will be 0 for any previous {C,
> > > O}xenstored
> > > implementation? If not, then I think we need to set the feature flag so
> > > Linux
> > > knows the field can be used.
> > > 
> > > If yes, then the documentation should be relaxed so an OS knows it can
> > > safely
> > > use the field without checking the feature flag.
> > 
> > The xenstore page is allocated by the toolstack which zeros the page,
> > *xenstored wouldn't set it, so I think we can assume the field has
> > always been zero.
> > 
> > 
> > > > diff --git a/xen/include/public/io/xs_wire.h
> > > > b/xen/include/public/io/xs_wire.h
> > > > index 953a0050a3..c1ec7c73e3 100644
> > > > --- a/xen/include/public/io/xs_wire.h
> > > > +++ b/xen/include/public/io/xs_wire.h
> > > > @@ -141,7 +141,7 @@ struct xenstore_domain_interface {
> > > >      /* Valid values for the connection field */
> > > >    #define XENSTORE_CONNECTED 0 /* the steady-state */
> > > > -#define XENSTORE_RECONNECT 1 /* guest has initiated a reconnect */
> > > > +#define XENSTORE_RECONNECT 1 /* reconnect in progress */
> > > 
> > > The definition in the docs needs to be updated.
> > 
> > I wanted to do that but I am very unfamiliar with the xenstore docs.
> > Can you point me to the place where I need to change the definition? I
> > cannot find where XENSTORE_RECONNECT is defined...
> > 
> 
> See docs/misc/xenstore-ring.txt

Found it, thanks! I'll add:

In certain circumstances (e.g. dom0less guests with PV drivers support)
it is possible for the guest to find the Connection state already set to
1 by someone else during xenstore initialization. In that case, like in
the previous case, the guest must ignore all fields except the
Connection state and wait for it to be set to 0 before proceeding.

Re: [PATCH v6 4/7] xen/arm: configure dom0less domain for enabling xenstore after boot

[Stefano Stabellini]

On Fri, 13 May 2022, Julien Grall wrote:
> On 13/05/2022 02:23, Stefano Stabellini wrote:
> > On Wed, 11 May 2022, Julien Grall wrote:
> > > > If dom0 is mandatory yes, we should still make sure that this code
> > > > cannot be
> > > > reached so an ASSERT would be nice here at least in case someone tries
> > > > to
> > > > activate this code without dom0 (which might happen when we will push
> > > > the
> > > > serie for static event channels).
> > > 
> > > I am fine with an ASSERT().
> > 
> > I added an ASSERT(hardware_domain).
> 
> Just to clarify and avoid a round trip. The ASSERT() is not sufficient here.
> We also need to forbid the user to set xen,enhanced when the HW domain is not
> NULL, at least until Rahul's pre-allocated event channel series.
> 
> This check would have to be done when parsing the configuration. The ASSERT()
> would be here to ensure that if someone is reworking the parsing, it would get
> caught nicely rather than a NULL dereference.

Thanks for avoiding a roundtrip. I added a check when parsing device
tree if xen,enhanced is specified but dom0 is missing. Initially I wrote
it as a "panic" but then I changed it as a regular printk. I am OK
either way in case you prefer otherwise.

Re: [PATCH v6 4/7] xen/arm: configure dom0less domain for enabling xenstore after boot

[Julien Grall]

Hi Stefano,

On 13/05/2022 21:52, Stefano Stabellini wrote:
> On Fri, 13 May 2022, Julien Grall wrote:
>> On 13/05/2022 02:23, Stefano Stabellini wrote:
>>> On Wed, 11 May 2022, Julien Grall wrote:
>>>>> If dom0 is mandatory yes, we should still make sure that this code
>>>>> cannot be
>>>>> reached so an ASSERT would be nice here at least in case someone tries
>>>>> to
>>>>> activate this code without dom0 (which might happen when we will push
>>>>> the
>>>>> serie for static event channels).
>>>>
>>>> I am fine with an ASSERT().
>>>
>>> I added an ASSERT(hardware_domain).
>>
>> Just to clarify and avoid a round trip. The ASSERT() is not sufficient here.
>> We also need to forbid the user to set xen,enhanced when the HW domain is not
>> NULL, at least until Rahul's pre-allocated event channel series.
>>
>> This check would have to be done when parsing the configuration. The ASSERT()
>> would be here to ensure that if someone is reworking the parsing, it would get
>> caught nicely rather than a NULL dereference.
> 
> Thanks for avoiding a roundtrip. I added a check when parsing device
> tree if xen,enhanced is specified but dom0 is missing. Initially I wrote
> it as a "panic" but then I changed it as a regular printk. I am OK
> either way in case you prefer otherwise.

This is a configuration issue from the user. So I think we shouldn't 
continue (i.e panic()) if we can't honor what the user requested. This 
make the problem a lot more obvious to the user (printk() can be easily 
overlooked).

Cheers,

-- 
Julien Grall