[PATCH v7 0/7] dom0less PV drivers

[PATCH v7 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 +-
 docs/misc/xenstore-ring.txt           |   8 +-
 tools/helpers/Makefile                |  13 ++
 tools/helpers/init-dom0less.c         | 345 ++++++++++++++++++++++++++++++++++
 tools/xenstore/xenstored_domain.c     |   4 +
 xen/arch/arm/domain_build.c           | 110 ++++++++++-
 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 +
 14 files changed, 545 insertions(+), 20 deletions(-)
 create mode 100644 tools/helpers/init-dom0less.c

[PATCH v7 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(-)
---
 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 v7 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>
Reviewed-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
Acked-by: Julien Grall <jgrall@amazon.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(-)
---
 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 aa777741bd..c4dd211b91 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);
 
@@ -1327,6 +1330,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)
@@ -1363,12 +1396,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
@@ -1377,13 +1411,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;
     }
 
@@ -1404,8 +1446,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 v7 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.) Dom0 presence is required for now
to use "xen,enhanced" on a domU.

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>
CC: Julien Grall <julien@xen.org>
CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
CC: Bertrand Marquis <bertrand.marquis@arm.com>
---
Changes in v7:
- add a check if dom0 is missing and xen,enhanced was specified
- Bertrand gave his reviewed-by but I removed it due to the new check

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(+)
---
 docs/misc/arm/device-tree/booting.txt | 18 ++++++++++++++++++
 xen/arch/arm/domain_build.c           | 12 ++++++++++++
 xen/arch/arm/include/asm/kernel.h     |  3 +++
 3 files changed, 33 insertions(+)

diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt
index 7b4a29a2c2..98253414b8 100644
--- a/docs/misc/arm/device-tree/booting.txt
+++ b/docs/misc/arm/device-tree/booting.txt
@@ -193,6 +193,24 @@ with the following properties:
     Optional. Handle to a xen,cpupool device tree node that identifies the
     cpupool where the guest will be started at boot.
 
+- 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 c4dd211b91..8d148b209d 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -3157,6 +3157,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;
 
@@ -3172,6 +3173,17 @@ 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")) )
+    {
+        if ( hardware_domain )
+            kinfo.dom0less_enhanced = true;
+        else
+            printk("Error: tried to use xen,enhanced without dom0\n");
+    }
+
     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

Re: [PATCH v7 3/7] xen: introduce xen,enhanced dom0less property

[Julien Grall]

Hi Stefano,

On 13/05/2022 22:07, Stefano Stabellini wrote:
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index c4dd211b91..8d148b209d 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -3157,6 +3157,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;
>   
> @@ -3172,6 +3173,17 @@ 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")) )
> +    {
> +        if ( hardware_domain )
> +            kinfo.dom0less_enhanced = true;
> +        else
> +            printk("Error: tried to use xen,enhanced without dom0\n");

In general, I prefer if we fail early for configuration error because 
this makes a lot more obvious what the issue is.

So I would switch to panic() and drop "Error:". This can be done on commit:

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

Cheers,

-- 
Julien Grall

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

---
Changes in v7:
- add an ASSERT, hardware_domain is required

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.
---
 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(-)
---
 xen/arch/arm/domain_build.c | 38 +++++++++++++++++++++++++++++++++++++
 xen/common/event_channel.c  |  2 +-
 xen/include/xen/event.h     |  3 +++
 3 files changed, 42 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 8d148b209d..27b2c7ec3f 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>
@@ -2813,6 +2814,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;
@@ -2887,6 +2890,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;
@@ -3153,6 +3163,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)
 {
@@ -3222,6 +3251,15 @@ static int __init construct_domU(struct domain *d,
     if ( rc < 0 )
         return rc;
 
+    if ( kinfo.dom0less_enhanced )
+    {
+        ASSERT(hardware_domain);
+        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 v7 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. Also
improve docs/misc/xenstore-ring.txt.

Signed-off-by: Luca Miccio <lucmiccio@gmail.com>
Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
---
Changes in v7:
- add documentation to xenstore-ring.txt
- Juergen already gave his reviewed-by, I dropped it due to the change
  to xenstore-ring.txt, but everything else is still the same

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(-)
---
 docs/misc/xenstore-ring.txt       | 8 +++++++-
 tools/xenstore/xenstored_domain.c | 4 ++++
 xen/include/public/io/xs_wire.h   | 2 +-
 3 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/docs/misc/xenstore-ring.txt b/docs/misc/xenstore-ring.txt
index b338b21b19..f3d6ca4264 100644
--- a/docs/misc/xenstore-ring.txt
+++ b/docs/misc/xenstore-ring.txt
@@ -111,7 +111,13 @@ Assuming the server has advertised the feature, the guest can initiate
 a reconnection by setting the the Connection state to 1 ("Ring close
 and reconnect is in progress") and signalling the event channel.
 The guest must now ignore all fields except the Connection state and
-wait for it to be set to 0 ("Ring is connected")
+wait for it to be set to 0 ("Ring is connected").
+
+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.
 
 The server will guarantee to
 
diff --git a/tools/xenstore/xenstored_domain.c b/tools/xenstore/xenstored_domain.c
index 80ba1d627b..de88bf2a68 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,
 				     true, 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

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

[Juergen Gross]

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

On 13.05.22 23:07, 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. Also
> improve docs/misc/xenstore-ring.txt.
> 
> Signed-off-by: Luca Miccio <lucmiccio@gmail.com>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>

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 --]

[PATCH v7 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 v8:
- check on HVM_PARAM_STORE_EVTCHN to see if the domain is xen,enhanced
- fix ubuntu native build, #include <sys/mman.h> for PROT_* definitions

Changes in v7:
- use PROT_READ | PROT_WRITE instead XS_READ | XS_WRITE

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
---
 tools/helpers/Makefile        |  13 ++
 tools/helpers/init-dom0less.c | 345 ++++++++++++++++++++++++++++++++++
 2 files changed, 358 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..3e7ad54da7
--- /dev/null
+++ b/tools/helpers/init-dom0less.c
@@ -0,0 +1,345 @@
+#include <stdbool.h>
+#include <syslog.h>
+#include <stdio.h>
+#include <err.h>
+#include <stdlib.h>
+#include <sys/mman.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;
+    }
+
+    /* no xen,enhanced; nothing to do */
+    if (!xenstore_evtchn)
+        return 0;
+
+    /* 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, PROT_READ | PROT_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

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

[Julien Grall]

Hi Stefano,

On 13/05/2022 22:07, Stefano Stabellini wrote:
> diff --git a/tools/helpers/init-dom0less.c b/tools/helpers/init-dom0less.c
> new file mode 100644
> index 0000000000..3e7ad54da7
> --- /dev/null
> +++ b/tools/helpers/init-dom0less.c
> @@ -0,0 +1,345 @@
> +#include <stdbool.h>
> +#include <syslog.h>
> +#include <stdio.h>
> +#include <err.h>
> +#include <stdlib.h>
> +#include <sys/mman.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

Sorry, I should have spotted this earlier. Looking at the nodes below, 
the node control/platform-feature-multiprocessor-suspend would result to 
63 characters without even the domid:

42sh> echo -n 
'/local/domain//control/platform-feature-multiprocessor-suspend' | wc -c
62

So I think it would be wiser to bump the value to 128 here.

> +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)

The issue I mentionned above would not have been spotted because you 
only check the value is negative. From glibc version 2.1,
snprintf() returns the number of character (excluding the NUL bytes) it 
would have written if the buffer is big enough.

So to avoid writing a truncated node, you will want to check the return 
value is > 0 && < (STR_MAX_LENGTH - 1).

Looking at the code below, there are a few wrong use of snprintf(). To 
avoid another round (we are at v7 already), I would be OK if they are 
dealt after so long we bump the size of the buffer.

The rest of the code looks ok:

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

Cheers,

-- 
Julien Grall

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

[Stefano Stabellini]

On Sat, 14 May 2022, Julien Grall wrote:
> On 13/05/2022 22:07, Stefano Stabellini wrote:
> > diff --git a/tools/helpers/init-dom0less.c b/tools/helpers/init-dom0less.c
> > new file mode 100644
> > index 0000000000..3e7ad54da7
> > --- /dev/null
> > +++ b/tools/helpers/init-dom0less.c
> > @@ -0,0 +1,345 @@
> > +#include <stdbool.h>
> > +#include <syslog.h>
> > +#include <stdio.h>
> > +#include <err.h>
> > +#include <stdlib.h>
> > +#include <sys/mman.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
> 
> Sorry, I should have spotted this earlier. Looking at the nodes below, the
> node control/platform-feature-multiprocessor-suspend would result to 63
> characters without even the domid:
> 
> 42sh> echo -n '/local/domain//control/platform-feature-multiprocessor-suspend'
> | wc -c
> 62
> 
> So I think it would be wiser to bump the value to 128 here.
> 
> > +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)
> 
> The issue I mentionned above would not have been spotted because you only
> check the value is negative. From glibc version 2.1,
> snprintf() returns the number of character (excluding the NUL bytes) it would
> have written if the buffer is big enough.
> 
> So to avoid writing a truncated node, you will want to check the return value
> is > 0 && < (STR_MAX_LENGTH - 1).
> 
> Looking at the code below, there are a few wrong use of snprintf(). To avoid
> another round (we are at v7 already), I would be OK if they are dealt after so
> long we bump the size of the buffer.
> 
> The rest of the code looks ok:
> 
> Acked-by: Julien Grall <jgrall@amazon.com>

Thank you that is very reasonable. I committed the series with
STR_MAX_LENGTH set to 128. I'll send a separate patch to improve the
snprintf checks.

[PATCH v7 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>
Acked-by: Julien Grall <jgrall@amazon.com>
---
 docs/features/dom0less.pandoc | 43 ++++++++++++++++++++++++++++++++---
 1 file changed, 40 insertions(+), 3 deletions(-)
---
 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