[PATCH v4 1/9] xen/dt: dt_property_read_string should return -ENODATA on !length

[PATCH v4 1/9] xen/dt: dt_property_read_string should return -ENODATA on !length

[Stefano Stabellini]

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

When the length is zero (pp->length == 0), dt_property_read_string
should return -ENODATA, but actually currently returns -EILSEQ because
there is no specific check for lenght == 0.

Add a check now.

Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
---
 xen/common/device_tree.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
index 4aae281e89..db67fb5fb4 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->value || !pp->length )
         return -ENODATA;
     if ( strnlen(pp->value, pp->length) >= pp->length )
         return -EILSEQ;
-- 
2.25.1

[PATCH v4 2/9] 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.

Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
---
 xen/arch/arm/domain_build.c | 42 ++++++++++++++++++++++++++++++++++---
 1 file changed, 39 insertions(+), 3 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 8be01678de..b6189b935d 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -1324,6 +1324,35 @@ out:
     return res;
 }
 
+#define ALIGN_UP_TO_2MB(x)   (((x) + MB(2) - 1) & (~(MB(2) - 1)))
+
+static int __init find_domU_holes(const struct kernel_info *kinfo,
+                                  struct meminfo *ext_regions)
+{
+    unsigned int i;
+    uint64_t bankend[GUEST_RAM_BANKS];
+    const uint64_t bankbase[] = GUEST_RAM_BANK_BASES;
+    const uint64_t banksize[] = GUEST_RAM_BANK_SIZES;
+
+    for ( i = 0; i < GUEST_RAM_BANKS; i++ )
+    {
+        ext_regions->bank[ext_regions->nr_banks].start =
+            ALIGN_UP_TO_2MB(bankbase[i] + kinfo->mem.bank[i].size);
+
+        bankend[i] = ~0ULL >> (64 - p2m_ipa_bits);
+        bankend[i] = min(bankend[i], bankbase[i] + banksize[i] - 1);
+        if (bankend[i] > ext_regions->bank[ext_regions->nr_banks].start)
+            ext_regions->bank[ext_regions->nr_banks].size =
+                bankend[i] - ext_regions->bank[ext_regions->nr_banks].start + 1;
+
+        /* 64MB is the minimum size of an extended region */
+        if ( ext_regions->bank[ext_regions->nr_banks].size < MB(64) )
+            continue;
+        ext_regions->nr_banks++;
+    }
+    return 0;
+}
+
 static int __init make_hypervisor_node(struct domain *d,
                                        const struct kernel_info *kinfo,
                                        int addrcells, int sizecells)
@@ -1374,10 +1403,17 @@ 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");
-- 
2.25.1

[PATCH v4 0/9] 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 "Introduce XSM ability
for domain privilege escalation".

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 (6):
      xen/dt: dt_property_read_string should return -ENODATA on !length
      xen/arm: implement domU extended regions
      xen/arm: temporarily elevate idle_domain privileged during create_domUs
      xen: export evtchn_alloc_unbound
      xen: introduce xen,enhanced dom0less property
      docs: document dom0less + PV drivers

 docs/features/dom0less.pandoc         |  43 ++++-
 docs/misc/arm/device-tree/booting.txt |  18 ++
 tools/helpers/Makefile                |  13 ++
 tools/helpers/init-dom0less.c         | 323 ++++++++++++++++++++++++++++++++++
 tools/xenstore/xenstored_domain.c     |   3 +
 xen/arch/arm/domain_build.c           |  91 +++++++++-
 xen/arch/arm/include/asm/kernel.h     |   3 +
 xen/common/device_tree.c              |   2 +-
 xen/common/event_channel.c            |   2 +-
 xen/include/xen/event.h               |   3 +
 10 files changed, 493 insertions(+), 8 deletions(-)
 create mode 100644 tools/helpers/init-dom0less.c

[PATCH v4 3/9] xen/arm: temporarily elevate idle_domain privileged during create_domUs

[Stefano Stabellini]

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

create_domUs might call functions that perform XSM checks on the current
domain, which is idle_domain at this time. Temporarily elevate
idle_domain privileges in create_domUs.

Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
---
 xen/arch/arm/domain_build.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index b6189b935d..100a4959a8 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 <xsm/xsm.h>
 
 #include <xen/irq.h>
 #include <xen/grant_table.h>
@@ -3210,6 +3211,8 @@ void __init create_domUs(void)
     struct dt_device_node *node;
     const struct dt_device_node *chosen = dt_find_node_by_path("/chosen");
 
+    xsm_elevate_priv(current->domain);
+
     BUG_ON(chosen == NULL);
     dt_for_each_child_node(chosen, node)
     {
@@ -3291,6 +3294,8 @@ void __init create_domUs(void)
         if ( construct_domU(d, node) != 0 )
             panic("Could not set up domain %s\n", dt_node_name(node));
     }
+
+    xsm_demote_priv(current->domain);
 }
 
 static int __init construct_dom0(struct domain *d)
-- 
2.25.1

[PATCH v4 4/9] xen: export evtchn_alloc_unbound

[Stefano Stabellini]

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

It will be used during dom0less domains construction.

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

diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
index ffb042a241..2f6a89f52d 100644
--- a/xen/common/event_channel.c
+++ b/xen/common/event_channel.c
@@ -289,7 +289,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..987e88623a 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 evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc);
+
 /* Unmask a local event-channel port. */
 int evtchn_unmask(unsigned int port);
 
-- 
2.25.1

[PATCH v4 5/9] 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 100a4959a8..b22fe95d92 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -3150,6 +3150,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;
 
@@ -3165,6 +3166,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 v4 6/9] xen/arm: configure dom0less domain for enabling xenstore after boot

[Stefano Stabellini]

From: Luca Miccio <lucmiccio@gmail.com>

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

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

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

Signed-off-by: Luca Miccio <lucmiccio@gmail.com>
Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
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 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/arch/arm/domain_build.c | 37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index b22fe95d92..a8ad95ce40 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -28,6 +28,7 @@
 #include <asm/cpufeature.h>
 #include <asm/domain_build.h>
 #include <xsm/xsm.h>
+#include <xen/event.h>
 
 #include <xen/irq.h>
 #include <xen/grant_table.h>
@@ -2806,6 +2807,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;
@@ -2880,6 +2883,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;
@@ -3146,6 +3156,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)
 {
@@ -3210,6 +3239,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;
 }
 
-- 
2.25.1

[PATCH v4 7/9] 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.

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

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>
---
Changes in v2:
- drop the new late_init parameter
---
 tools/xenstore/xenstored_domain.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tools/xenstore/xenstored_domain.c b/tools/xenstore/xenstored_domain.c
index ae065fcbee..0543f49670 100644
--- a/tools/xenstore/xenstored_domain.c
+++ b/tools/xenstore/xenstored_domain.c
@@ -493,6 +493,9 @@ 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 */
+		xenevtchn_notify(xce_handle, domain->port);
+
 		if (!is_master_domain && !restore)
 			fire_watches(NULL, ctx, "@introduceDomain", NULL,
 				     false, NULL);
-- 
2.25.1

[PATCH v4 8/9] tools: add example application to initialize dom0less PV drivers

[Stefano Stabellini]

From: Luca Miccio <lucmiccio@gmail.com>

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

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

diff --git a/tools/helpers/Makefile b/tools/helpers/Makefile
index 7f6c422440..8e42997052 100644
--- a/tools/helpers/Makefile
+++ b/tools/helpers/Makefile
@@ -10,6 +10,9 @@ ifeq ($(CONFIG_Linux),y)
 ifeq ($(CONFIG_X86),y)
 PROGS += init-xenstore-domain
 endif
+ifeq ($(CONFIG_ARM),y)
+PROGS += init-dom0less
+endif
 endif
 
 XEN_INIT_DOM0_OBJS = xen-init-dom0.o init-dom-json.o
@@ -26,6 +29,13 @@ $(INIT_XENSTORE_DOMAIN_OBJS): CFLAGS += $(CFLAGS_libxenstore)
 $(INIT_XENSTORE_DOMAIN_OBJS): CFLAGS += $(CFLAGS_libxenlight)
 $(INIT_XENSTORE_DOMAIN_OBJS): CFLAGS += -include $(XEN_ROOT)/tools/config.h
 
+INIT_DOM0LESS_OBJS = init-dom0less.o init-dom-json.o
+$(INIT_DOM0LESS_OBJS): CFLAGS += $(CFLAGS_libxentoollog)
+$(INIT_DOM0LESS_OBJS): CFLAGS += $(CFLAGS_libxenstore)
+$(INIT_DOM0LESS_OBJS): CFLAGS += $(CFLAGS_libxenlight)
+$(INIT_DOM0LESS_OBJS): CFLAGS += $(CFLAGS_libxenctrl)
+$(INIT_DOM0LESS_OBJS): CFLAGS += $(CFLAGS_libxenevtchn)
+
 .PHONY: all
 all: $(PROGS)
 
@@ -35,6 +45,9 @@ xen-init-dom0: $(XEN_INIT_DOM0_OBJS)
 init-xenstore-domain: $(INIT_XENSTORE_DOMAIN_OBJS)
 	$(CC) $(LDFLAGS) -o $@ $(INIT_XENSTORE_DOMAIN_OBJS) $(LDLIBS_libxentoollog) $(LDLIBS_libxenstore) $(LDLIBS_libxenctrl) $(LDLIBS_libxenguest) $(LDLIBS_libxenlight) $(APPEND_LDFLAGS)
 
+init-dom0less: $(INIT_DOM0LESS_OBJS)
+	$(CC) $(LDFLAGS) -o $@ $(INIT_DOM0LESS_OBJS) $(LDLIBS_libxenctrl) $(LDLIBS_libxenevtchn) $(LDLIBS_libxentoollog) $(LDLIBS_libxenstore) $(LDLIBS_libxenlight) $(LDLIBS_libxenguest)  $(APPEND_LDFLAGS)
+
 .PHONY: install
 install: all
 	$(INSTALL_DIR) $(DESTDIR)$(LIBEXEC_BIN)
diff --git a/tools/helpers/init-dom0less.c b/tools/helpers/init-dom0less.c
new file mode 100644
index 0000000000..dc9ccee868
--- /dev/null
+++ b/tools/helpers/init-dom0less.c
@@ -0,0 +1,323 @@
+#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 "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, libxl_dominfo *info)
+{
+    struct xc_interface_core *xch;
+    libxl_uuid uuid;
+    uint64_t xenstore_evtchn, xenstore_pfn;
+    int rc;
+
+    printf("Init dom0less domain: %u\n", info->domid);
+    xch = xc_interface_open(0, 0, 0);
+
+    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;
+    }
+
+    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;
+
+    /* TODO reuse libxl xsh connection */
+    xsh = xs_open(0);
+    if (xsh == NULL) {
+        fprintf(stderr, "Could not contact XenStore");
+        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, &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 v4 9/9] 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>
---
 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 v4 1/9] xen/dt: dt_property_read_string should return -ENODATA on !length

[Henry Wang]

Hi Stefano,

> -----Original Message-----
> From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of
> Stefano Stabellini
> Sent: Friday, April 1, 2022 8:39 AM
> To: xen-devel@lists.xenproject.org
> Cc: sstabellini@kernel.org; jgross@suse.com; Bertrand Marquis
> <Bertrand.Marquis@arm.com>; julien@xen.org;
> Volodymyr_Babchuk@epam.com; Stefano Stabellini
> <stefano.stabellini@xilinx.com>
> Subject: [PATCH v4 1/9] xen/dt: dt_property_read_string should return -
> ENODATA on !length
> 
> From: Stefano Stabellini <stefano.stabellini@xilinx.com>
> 
> When the length is zero (pp->length == 0), dt_property_read_string
> should return -ENODATA, but actually currently returns -EILSEQ because
> there is no specific check for lenght == 0.
> 
> Add a check now.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>

Reviewed-by: Henry Wang <Henry.Wang@arm.com>

> ---
>  xen/common/device_tree.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> index 4aae281e89..db67fb5fb4 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->value || !pp->length )
>          return -ENODATA;
>      if ( strnlen(pp->value, pp->length) >= pp->length )
>          return -EILSEQ;
> --
> 2.25.1
> 

Re: [PATCH v4 8/9] tools: add example application to initialize dom0less PV drivers

[Julien Grall]

Hi,

I have posted some comments in v3 after you sent this version. Please 
have a look.

On 01/04/2022 01:38, Stefano Stabellini wrote:
> +static int init_domain(struct xs_handle *xsh, libxl_dominfo *info)
> +{
> +    struct xc_interface_core *xch;
> +    libxl_uuid uuid;
> +    uint64_t xenstore_evtchn, xenstore_pfn;
> +    int rc;
> +
> +    printf("Init dom0less domain: %u\n", info->domid);
> +    xch = xc_interface_open(0, 0, 0);
> +
> +    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;
> +    }
> +
> +    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);

On patch #1, you told me you didn't want to allocate the page in Xen 
because it wouldn't be initialized by Xenstored. But this is what we are 
doing here.

This would be a problem if Linux is still booting and hasn't yet call 
xenbus_probe_initcall().

I understand we need to have the page setup before raising the event 
channel. I don't think we can allow Xenstored to set the HVM_PARAM (it 
may run in a domain with less privilege). So I think we may need to 
create a separate command to kick the client (not great).

Juergen, any thoughts?

Cheers,

-- 
Julien Grall

Re: [PATCH v4 8/9] tools: add example application to initialize dom0less PV drivers

[Juergen Gross]

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

On 01.04.22 12:21, Julien Grall wrote:
> Hi,
> 
> I have posted some comments in v3 after you sent this version. Please have a look.
> 
> On 01/04/2022 01:38, Stefano Stabellini wrote:
>> +static int init_domain(struct xs_handle *xsh, libxl_dominfo *info)
>> +{
>> +    struct xc_interface_core *xch;
>> +    libxl_uuid uuid;
>> +    uint64_t xenstore_evtchn, xenstore_pfn;
>> +    int rc;
>> +
>> +    printf("Init dom0less domain: %u\n", info->domid);
>> +    xch = xc_interface_open(0, 0, 0);
>> +
>> +    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;
>> +    }
>> +
>> +    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);
> 
> On patch #1, you told me you didn't want to allocate the page in Xen because it 
> wouldn't be initialized by Xenstored. But this is what we are doing here.

Xenstore (at least the C variant) is only using the fixed grant ref
GNTTAB_RESERVED_XENSTORE, so it doesn't need the page to be advertised
to the guest. And the mapping is done only when the domain is being
introduced to Xenstore.

> 
> This would be a problem if Linux is still booting and hasn't yet call 
> xenbus_probe_initcall().
> 
> I understand we need to have the page setup before raising the event channel. I 
> don't think we can allow Xenstored to set the HVM_PARAM (it may run in a domain 
> with less privilege). So I think we may need to create a separate command to 
> kick the client (not great).
> 
> Juergen, any thoughts?

I think it should work like that:

- setup the grant via xc_dom_gnttab_seed()
- introduce the domain to Xenstore
- call xc_hvm_param_set()

When the guest is receiving the event, it should wait for the xenstore
page to appear.


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 v4 9/9] docs: document dom0less + PV drivers

[Luca Fancellu]

> On 1 Apr 2022, at 01:38, Stefano Stabellini <sstabellini@kernel.org> 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>

Hi Stefano,

Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>

Re: [PATCH v4 7/9] xenstored: send an evtchn notification on introduce_domain

[Juergen Gross]

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

On 01.04.22 02:38, 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.
> 
> The extra notification is harmless for domains that don't require it.
> 
> 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>

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 v4 8/9] tools: add example application to initialize dom0less PV drivers

[Julien Grall]

Hi Juergen,

On 01/04/2022 11:46, Juergen Gross wrote:
> On 01.04.22 12:21, Julien Grall wrote:
>> Hi,
>>
>> I have posted some comments in v3 after you sent this version. Please 
>> have a look.
>>
>> On 01/04/2022 01:38, Stefano Stabellini wrote:
>>> +static int init_domain(struct xs_handle *xsh, libxl_dominfo *info)
>>> +{
>>> +    struct xc_interface_core *xch;
>>> +    libxl_uuid uuid;
>>> +    uint64_t xenstore_evtchn, xenstore_pfn;
>>> +    int rc;
>>> +
>>> +    printf("Init dom0less domain: %u\n", info->domid);
>>> +    xch = xc_interface_open(0, 0, 0);
>>> +
>>> +    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;
>>> +    }
>>> +
>>> +    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);
>>
>> On patch #1, you told me you didn't want to allocate the page in Xen 
>> because it wouldn't be initialized by Xenstored. But this is what we 
>> are doing here.
> 
> Xenstore (at least the C variant) is only using the fixed grant ref
> GNTTAB_RESERVED_XENSTORE, so it doesn't need the page to be advertised
> to the guest. And the mapping is done only when the domain is being
> introduced to Xenstore.

And we don't expect the guest to use the grant entry to find the 
xenstore page?
> 
>>
>> This would be a problem if Linux is still booting and hasn't yet call 
>> xenbus_probe_initcall().
>>
>> I understand we need to have the page setup before raising the event 
>> channel. I don't think we can allow Xenstored to set the HVM_PARAM (it 
>> may run in a domain with less privilege). So I think we may need to 
>> create a separate command to kick the client (not great).
>>
>> Juergen, any thoughts?
> 
> I think it should work like that:
> 
> - setup the grant via xc_dom_gnttab_seed()
> - introduce the domain to Xenstore
> - call xc_hvm_param_set()
> 
> When the guest is receiving the event, it should wait for the xenstore
> page to appear.
IIUC, this would mean the guest would need to do some sort of busy loop 
until the xenstore page to appear.

If so, this doesn't sound great to me. I think it would be better to 
have a flag in the page to indicate whether the page is not ready.

Xenstored could then clear the flag before raising the event channel.

Cheers,

-- 
Julien Grall

Re: [PATCH v4 8/9] tools: add example application to initialize dom0less PV drivers

[Juergen Gross]

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

On 01.04.22 15:35, Julien Grall wrote:
> Hi Juergen,
> 
> On 01/04/2022 11:46, Juergen Gross wrote:
>> On 01.04.22 12:21, Julien Grall wrote:
>>> Hi,
>>>
>>> I have posted some comments in v3 after you sent this version. Please have a 
>>> look.
>>>
>>> On 01/04/2022 01:38, Stefano Stabellini wrote:
>>>> +static int init_domain(struct xs_handle *xsh, libxl_dominfo *info)
>>>> +{
>>>> +    struct xc_interface_core *xch;
>>>> +    libxl_uuid uuid;
>>>> +    uint64_t xenstore_evtchn, xenstore_pfn;
>>>> +    int rc;
>>>> +
>>>> +    printf("Init dom0less domain: %u\n", info->domid);
>>>> +    xch = xc_interface_open(0, 0, 0);
>>>> +
>>>> +    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;
>>>> +    }
>>>> +
>>>> +    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);
>>>
>>> On patch #1, you told me you didn't want to allocate the page in Xen because 
>>> it wouldn't be initialized by Xenstored. But this is what we are doing here.
>>
>> Xenstore (at least the C variant) is only using the fixed grant ref
>> GNTTAB_RESERVED_XENSTORE, so it doesn't need the page to be advertised
>> to the guest. And the mapping is done only when the domain is being
>> introduced to Xenstore.
> 
> And we don't expect the guest to use the grant entry to find the xenstore page?
>>
>>>
>>> This would be a problem if Linux is still booting and hasn't yet call 
>>> xenbus_probe_initcall().
>>>
>>> I understand we need to have the page setup before raising the event channel. 
>>> I don't think we can allow Xenstored to set the HVM_PARAM (it may run in a 
>>> domain with less privilege). So I think we may need to create a separate 
>>> command to kick the client (not great).
>>>
>>> Juergen, any thoughts?
>>
>> I think it should work like that:
>>
>> - setup the grant via xc_dom_gnttab_seed()
>> - introduce the domain to Xenstore
>> - call xc_hvm_param_set()
>>
>> When the guest is receiving the event, it should wait for the xenstore
>> page to appear.
> IIUC, this would mean the guest would need to do some sort of busy loop until 
> the xenstore page to appear.

Looking for it every second or so would be enough.

> If so, this doesn't sound great to me. I think it would be better to have a flag 
> in the page to indicate whether the page is not ready.
> 
> Xenstored could then clear the flag before raising the event channel.

Hmm, the "connection" field could be used for that.

It would mean, though, that e.g. libxl would need to initialize the
page accordingly before calling xs_introduce().


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 v4 2/9] xen/arm: implement domU extended regions

[Luca Fancellu]

Hi Stefano,

> +#define ALIGN_UP_TO_2MB(x)   (((x) + MB(2) - 1) & (~(MB(2) - 1)))
> +
> +static int __init find_domU_holes(const struct kernel_info *kinfo,
> +                                  struct meminfo *ext_regions)
> +{
> +    unsigned int i;
> +    uint64_t bankend[GUEST_RAM_BANKS];
> +    const uint64_t bankbase[] = GUEST_RAM_BANK_BASES;
> +    const uint64_t banksize[] = GUEST_RAM_BANK_SIZES;
> +
> +    for ( i = 0; i < GUEST_RAM_BANKS; i++ )
> +    {
> +        ext_regions->bank[ext_regions->nr_banks].start =
> +            ALIGN_UP_TO_2MB(bankbase[i] + kinfo->mem.bank[i].size);
> +
> +        bankend[i] = ~0ULL >> (64 - p2m_ipa_bits);
> +        bankend[i] = min(bankend[i], bankbase[i] + banksize[i] - 1);
> +        if (bankend[i] > ext_regions->bank[ext_regions->nr_banks].start)

Just a code style issue, the if needs a space before and after the condition

With this fixed:

Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>

Cheers,
Luca

Re: [PATCH v4 8/9] tools: add example application to initialize dom0less PV drivers

[Julien Grall]

Hi Juergen,

On 01/04/2022 14:52, Juergen Gross wrote:
> On 01.04.22 15:35, Julien Grall wrote:
>> Hi Juergen,
>>
>> On 01/04/2022 11:46, Juergen Gross wrote:
>>> On 01.04.22 12:21, Julien Grall wrote:
>>>> Hi,
>>>>
>>>> I have posted some comments in v3 after you sent this version. 
>>>> Please have a look.
>>>>
>>>> On 01/04/2022 01:38, Stefano Stabellini wrote:
>>>>> +static int init_domain(struct xs_handle *xsh, libxl_dominfo *info)
>>>>> +{
>>>>> +    struct xc_interface_core *xch;
>>>>> +    libxl_uuid uuid;
>>>>> +    uint64_t xenstore_evtchn, xenstore_pfn;
>>>>> +    int rc;
>>>>> +
>>>>> +    printf("Init dom0less domain: %u\n", info->domid);
>>>>> +    xch = xc_interface_open(0, 0, 0);
>>>>> +
>>>>> +    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;
>>>>> +    }
>>>>> +
>>>>> +    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);
>>>>
>>>> On patch #1, you told me you didn't want to allocate the page in Xen 
>>>> because it wouldn't be initialized by Xenstored. But this is what we 
>>>> are doing here.
>>>
>>> Xenstore (at least the C variant) is only using the fixed grant ref
>>> GNTTAB_RESERVED_XENSTORE, so it doesn't need the page to be advertised
>>> to the guest. And the mapping is done only when the domain is being
>>> introduced to Xenstore.
>>
>> And we don't expect the guest to use the grant entry to find the 
>> xenstore page?
>>>
>>>>
>>>> This would be a problem if Linux is still booting and hasn't yet 
>>>> call xenbus_probe_initcall().
>>>>
>>>> I understand we need to have the page setup before raising the event 
>>>> channel. I don't think we can allow Xenstored to set the HVM_PARAM 
>>>> (it may run in a domain with less privilege). So I think we may need 
>>>> to create a separate command to kick the client (not great).
>>>>
>>>> Juergen, any thoughts?
>>>
>>> I think it should work like that:
>>>
>>> - setup the grant via xc_dom_gnttab_seed()
>>> - introduce the domain to Xenstore
>>> - call xc_hvm_param_set()
>>>
>>> When the guest is receiving the event, it should wait for the xenstore
>>> page to appear.
>> IIUC, this would mean the guest would need to do some sort of busy 
>> loop until the xenstore page to appear.
> 
> Looking for it every second or so would be enough.

This is a better than a busy loop but not by much. I would argue a 
design that requires to poll after receiving an interrupt is broken.

> 
>> If so, this doesn't sound great to me. I think it would be better to 
>> have a flag in the page to indicate whether the page is not ready.
>>
>> Xenstored could then clear the flag before raising the event channel.
> 
> Hmm, the "connection" field could be used for that.

I thought about the field but the description doesn't entirely match 
what we want. In particular, the spec says only the guest should set the 
value to 1 (i.e. reconnect). Maybe this could be relaxed?

> 
> It would mean, though, that e.g. libxl would need to initialize the
> page accordingly before calling xs_introduce()

libxl only create domain paused. So I don't think it would be necessary 
to update it.

Cheers,

-- 
Julien Grall

Re: [PATCH v4 8/9] tools: add example application to initialize dom0less PV drivers

[Juergen Gross]

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

On 01.04.22 16:04, Julien Grall wrote:
> Hi Juergen,
> 
> On 01/04/2022 14:52, Juergen Gross wrote:
>> On 01.04.22 15:35, Julien Grall wrote:
>>> Hi Juergen,
>>>
>>> On 01/04/2022 11:46, Juergen Gross wrote:
>>>> On 01.04.22 12:21, Julien Grall wrote:
>>>>> Hi,
>>>>>
>>>>> I have posted some comments in v3 after you sent this version. Please have 
>>>>> a look.
>>>>>
>>>>> On 01/04/2022 01:38, Stefano Stabellini wrote:
>>>>>> +static int init_domain(struct xs_handle *xsh, libxl_dominfo *info)
>>>>>> +{
>>>>>> +    struct xc_interface_core *xch;
>>>>>> +    libxl_uuid uuid;
>>>>>> +    uint64_t xenstore_evtchn, xenstore_pfn;
>>>>>> +    int rc;
>>>>>> +
>>>>>> +    printf("Init dom0less domain: %u\n", info->domid);
>>>>>> +    xch = xc_interface_open(0, 0, 0);
>>>>>> +
>>>>>> +    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;
>>>>>> +    }
>>>>>> +
>>>>>> +    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);
>>>>>
>>>>> On patch #1, you told me you didn't want to allocate the page in Xen 
>>>>> because it wouldn't be initialized by Xenstored. But this is what we are 
>>>>> doing here.
>>>>
>>>> Xenstore (at least the C variant) is only using the fixed grant ref
>>>> GNTTAB_RESERVED_XENSTORE, so it doesn't need the page to be advertised
>>>> to the guest. And the mapping is done only when the domain is being
>>>> introduced to Xenstore.
>>>
>>> And we don't expect the guest to use the grant entry to find the xenstore page?
>>>>
>>>>>
>>>>> This would be a problem if Linux is still booting and hasn't yet call 
>>>>> xenbus_probe_initcall().
>>>>>
>>>>> I understand we need to have the page setup before raising the event 
>>>>> channel. I don't think we can allow Xenstored to set the HVM_PARAM (it may 
>>>>> run in a domain with less privilege). So I think we may need to create a 
>>>>> separate command to kick the client (not great).
>>>>>
>>>>> Juergen, any thoughts?
>>>>
>>>> I think it should work like that:
>>>>
>>>> - setup the grant via xc_dom_gnttab_seed()
>>>> - introduce the domain to Xenstore
>>>> - call xc_hvm_param_set()
>>>>
>>>> When the guest is receiving the event, it should wait for the xenstore
>>>> page to appear.
>>> IIUC, this would mean the guest would need to do some sort of busy loop until 
>>> the xenstore page to appear.
>>
>> Looking for it every second or so would be enough.
> 
> This is a better than a busy loop but not by much. I would argue a design that 
> requires to poll after receiving an interrupt is broken.
> 
>>
>>> If so, this doesn't sound great to me. I think it would be better to have a 
>>> flag in the page to indicate whether the page is not ready.
>>>
>>> Xenstored could then clear the flag before raising the event channel.
>>
>> Hmm, the "connection" field could be used for that.
> 
> I thought about the field but the description doesn't entirely match what we 
> want. In particular, the spec says only the guest should set the value to 1 
> (i.e. reconnect). Maybe this could be relaxed?
> 
>>
>> It would mean, though, that e.g. libxl would need to initialize the
>> page accordingly before calling xs_introduce()
> 
> libxl only create domain paused. So I don't think it would be necessary to 
> update it.

Maybe not libxl, but whoever is calling xc_dom_gnttab_seed(),
xc_hvm_param_set() and/or xs_introduce() needs to set the field, in
order to have an effect of Xenstore resetting it.


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 v4 2/9] xen/arm: implement domU extended regions

[Julien Grall]

Hi,

On 01/04/2022 01:38, Stefano Stabellini wrote:
> From: Stefano Stabellini <stefano.stabellini@xilinx.com>
> 
> Implement extended regions for dom0less domUs. The implementation is
> based on the libxl implementation.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
> ---
>   xen/arch/arm/domain_build.c | 42 ++++++++++++++++++++++++++++++++++---
>   1 file changed, 39 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 8be01678de..b6189b935d 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -1324,6 +1324,35 @@ out:
>       return res;
>   }
>   
> +#define ALIGN_UP_TO_2MB(x)   (((x) + MB(2) - 1) & (~(MB(2) - 1)))

I think this is the same as ROUNDUP(x, SZ_2M).

> +

> +static int __init find_domU_holes(const struct kernel_info *kinfo,
> +                                  struct meminfo *ext_regions)
> +{
> +    unsigned int i;
> +    uint64_t bankend[GUEST_RAM_BANKS];

Looking, you only seem to use one bankend at the time. So why do you 
need to store all the bankend?

This should also be s/uint64_t/paddr_t/. Same for two other instances below.

> +    const uint64_t bankbase[] = GUEST_RAM_BANK_BASES;
> +    const uint64_t banksize[] = GUEST_RAM_BANK_SIZES;
> +
> +    for ( i = 0; i < GUEST_RAM_BANKS; i++ )
> +    {
> +        ext_regions->bank[ext_regions->nr_banks].start =

The code would be easier to read if you define a local variable ext_bank 
that points to &(ext_regions->bank[ext_regions->nr_banks]).

> +            ALIGN_UP_TO_2MB(bankbase[i] + kinfo->mem.bank[i].size);
> +
> +        bankend[i] = ~0ULL >> (64 - p2m_ipa_bits);
> +        bankend[i] = min(bankend[i], bankbase[i] + banksize[i] - 1); > +        if (bankend[i] > ext_regions->bank[ext_regions->nr_banks].start)

Coding style:

if ( ... )

> +            ext_regions->bank[ext_regions->nr_banks].size =
> +                bankend[i] - ext_regions->bank[ext_regions->nr_banks].start + 1;

This is one of the line that could greatly benefits from the local 
variable I suggested above. It would look like:

ext_bank->size = bankend[i] - ext_bank->start + 1;

> +
> +        /* 64MB is the minimum size of an extended region */
> +        if ( ext_regions->bank[ext_regions->nr_banks].size < MB(64) )
> +            continue;
> +        ext_regions->nr_banks++;
> +    }

NIT: We tend to add a newline before the last return.

> +    return 0;

find_memory_holes() and find_unallocated_memory() will return an error 
if there are no banks allocated. I think we should do the same here at 
least for consistency.

In which case, the check should be moved in make_hypervisor_node().

> +}
> +
>   static int __init make_hypervisor_node(struct domain *d,
>                                          const struct kernel_info *kinfo,
>                                          int addrcells, int sizecells)
> @@ -1374,10 +1403,17 @@ 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) )

I believe the code in the 'if' part would work properly for a dom0 that 
is not direct mapped (e.g. in the cache coloring case).

If it doesn't, I think we need...

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

... and ASSERT() here so we the person that will introduce non direct 
mapped dom0 can easily notice before the domain get corrupted.

> +            res = find_domU_holes(kinfo, ext_regions);
> +        }
>   
>           if ( res )
>               printk(XENLOG_WARNING "Failed to allocate extended regions\n");

This printk() and the others in the function should print the domain.

Cheers,

-- 
Julien Grall

Re: [PATCH v4 3/9] xen/arm: temporarily elevate idle_domain privileged during create_domUs

[Julien Grall]

Hi,

On 01/04/2022 01:38, Stefano Stabellini wrote:
> From: Stefano Stabellini <stefano.stabellini@xilinx.com>
> 
> create_domUs might call functions that perform XSM checks on the current
> domain, which is idle_domain at this time. Temporarily elevate
> idle_domain privileges in create_domUs.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
> ---
>   xen/arch/arm/domain_build.c | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index b6189b935d..100a4959a8 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 <xsm/xsm.h>
>   
>   #include <xen/irq.h>
>   #include <xen/grant_table.h>
> @@ -3210,6 +3211,8 @@ void __init create_domUs(void)
>       struct dt_device_node *node;
>       const struct dt_device_node *chosen = dt_find_node_by_path("/chosen");
>   
> +    xsm_elevate_priv(current->domain);

Please check the return of this function and...

> +
>       BUG_ON(chosen == NULL);
>       dt_for_each_child_node(chosen, node)
>       {
> @@ -3291,6 +3294,8 @@ void __init create_domUs(void)
>           if ( construct_domU(d, node) != 0 )
>               panic("Could not set up domain %s\n", dt_node_name(node));
>       }
> +
> +    xsm_demote_priv(current->domain);

... this. For us, it should hopefully be 0. But it is a good practice to 
confirm.

Cheers,

-- 
Julien Grall

Re: [PATCH v4 4/9] xen: export evtchn_alloc_unbound

[Jan Beulich]

On 01.04.2022 02:38, Stefano Stabellini wrote:
> From: Stefano Stabellini <stefano.stabellini@xilinx.com>
> 
> It will be used during dom0less domains construction.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>

I think this better wouldn't be a patch of its own. Functions should
be non-static only when they have a user outside of their defining TU.

> --- 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 evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc);

I wonder whether while exposing it the function should also become
__must_check.

Jan

Re: [PATCH v4 8/9] tools: add example application to initialize dom0less PV drivers

[Stefano Stabellini]

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

On Fri, 1 Apr 2022, Juergen Gross wrote:
> On 01.04.22 12:21, Julien Grall wrote:
> > Hi,
> > 
> > I have posted some comments in v3 after you sent this version. Please have a
> > look.
> > 
> > On 01/04/2022 01:38, Stefano Stabellini wrote:
> > > +static int init_domain(struct xs_handle *xsh, libxl_dominfo *info)
> > > +{
> > > +    struct xc_interface_core *xch;
> > > +    libxl_uuid uuid;
> > > +    uint64_t xenstore_evtchn, xenstore_pfn;
> > > +    int rc;
> > > +
> > > +    printf("Init dom0less domain: %u\n", info->domid);
> > > +    xch = xc_interface_open(0, 0, 0);
> > > +
> > > +    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;
> > > +    }
> > > +
> > > +    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);
> > 
> > On patch #1, you told me you didn't want to allocate the page in Xen because
> > it wouldn't be initialized by Xenstored. But this is what we are doing here.
> 
> Xenstore (at least the C variant) is only using the fixed grant ref
> GNTTAB_RESERVED_XENSTORE, so it doesn't need the page to be advertised
> to the guest. And the mapping is done only when the domain is being
> introduced to Xenstore.
> 
> > 
> > This would be a problem if Linux is still booting and hasn't yet call
> > xenbus_probe_initcall().
> > 
> > I understand we need to have the page setup before raising the event
> > channel. I don't think we can allow Xenstored to set the HVM_PARAM (it may
> > run in a domain with less privilege). So I think we may need to create a
> > separate command to kick the client (not great).
> > 
> > Juergen, any thoughts?
> 
> I think it should work like that:
> 
> - setup the grant via xc_dom_gnttab_seed()
> - introduce the domain to Xenstore
> - call xc_hvm_param_set()
> 
> When the guest is receiving the event, it should wait for the xenstore
> page to appear.


I am OK with what you wrote above, and I understand Julien's concerns
about "waiting". Before discussing that, I would like to make sure I
understood why setting HVM_PARAM_STORE_PFN first (before
xs_introduce_domain) is not possible.

In a previous reply to Julien I wrote that it is not a good idea to
set HVM_PARAM_STORE_PFN in Xen before creating the domains because it
would cause Linux to hang at boot. That is true, Linux hangs on
drivers/xen/xenbus/xenbus_comms.c:xb_init_comms waiting on xb_waitq.
It could wait a very long time as domUs are typically a lot faster than
dom0 to boot.

However, if we set HVM_PARAM_STORE_PFN before calling
xs_introduce_domain in init-dom0less, for Linux to see it before
xs_introduce_domain is done, Linux would need to be racing against
init-dom0less. In that case, the wait in xb_init_comms would be minimal
anyway. It shouldn't be a problem. There would be no "hang", just a wait
a bit longer than usual.

Is that right?

Re: [PATCH v4 8/9] tools: add example application to initialize dom0less PV drivers

[Julien Grall]

Hi Stefano,

On 06/04/2022 03:21, Stefano Stabellini wrote:
> On Fri, 1 Apr 2022, Juergen Gross wrote:
>> On 01.04.22 12:21, Julien Grall wrote:
>>> This would be a problem if Linux is still booting and hasn't yet call
>>> xenbus_probe_initcall().
>>>
>>> I understand we need to have the page setup before raising the event
>>> channel. I don't think we can allow Xenstored to set the HVM_PARAM (it may
>>> run in a domain with less privilege). So I think we may need to create a
>>> separate command to kick the client (not great).
>>>
>>> Juergen, any thoughts?
>>
>> I think it should work like that:
>>
>> - setup the grant via xc_dom_gnttab_seed()
>> - introduce the domain to Xenstore
>> - call xc_hvm_param_set()
>>
>> When the guest is receiving the event, it should wait for the xenstore
>> page to appear.
> 
> 
> I am OK with what you wrote above, and I understand Julien's concerns
> about "waiting". Before discussing that, I would like to make sure I
> understood why setting HVM_PARAM_STORE_PFN first (before
> xs_introduce_domain) is not possible.
> 
> In a previous reply to Julien I wrote that it is not a good idea to
> set HVM_PARAM_STORE_PFN in Xen before creating the domains because it
> would cause Linux to hang at boot. That is true, Linux hangs on
> drivers/xen/xenbus/xenbus_comms.c:xb_init_comms waiting on xb_waitq.
I looked at the implementation of xb_init_comms in 5.17 and I couldn't 
find out how it would block here. Can you clarify?

> It could wait a very long time as domUs are typically a lot faster than
> dom0 to boot.
> 
> However, if we set HVM_PARAM_STORE_PFN before calling
> xs_introduce_domain in init-dom0less, for Linux to see it before
> xs_introduce_domain is done, Linux would need to be racing against
> init-dom0less. In that case, the wait in xb_init_comms would be minimal
> anyway. It shouldn't be a problem. There would be no "hang", just a wait
> a bit longer than usual.

 From my understanding, Linux may send commands as soon as it sees 
HVM_PARAM_STORE_PFN. With your proposal, this may happen before 
xs_introduce_domain() has updated the features supported by Xenstored.

With the recent proposal from Juergen [1], an OS will need to read the 
features field to understand whether Xenstored support the extended 
version of a command.

This means, any commands sent before xs_introduce_domain() would not be 
able to take advantage of the new features. Therefore, we would need to 
wait until Xenstored has advertised the features.

With your approach, the wait would be a busy loop. Although, I am not 
entirely sure what you would be waiting on?

But if we use the 'connection status' field, then you could just delay 
the initialization until you receive an event and the connection status 
is connected.

Cheers,

[1] https://lore.kernel.org/xen-devel/20220316161017.3579-1-jgross@suse.com/

-- 
Julien Grall

Re: [PATCH v4 8/9] tools: add example application to initialize dom0less PV drivers

[Stefano Stabellini]

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

On Wed, 6 Apr 2022, Julien Grall wrote:
> But if we use the 'connection status' field, then you could just delay the
> initialization until you receive an event and the connection status is
> connected.

I prototyped this approach and I managed to validate it successfully.
See attached patches for xen and linux (on top of existing patches). I
allocated the page from init-dom0less (instead of Xen) to achieve best
compatibility with older kernels.

There are some rough edges in the two patches but let me know if you
have any comments on the overall approach.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/x-diff; NAME=xen.patch, Size: 3145 bytes --]

diff --git a/tools/helpers/Makefile b/tools/helpers/Makefile
index 8e42997052..8d78ab1e90 100644
--- a/tools/helpers/Makefile
+++ b/tools/helpers/Makefile
@@ -46,7 +46,7 @@ init-xenstore-domain: $(INIT_XENSTORE_DOMAIN_OBJS)
 	$(CC) $(LDFLAGS) -o $@ $(INIT_XENSTORE_DOMAIN_OBJS) $(LDLIBS_libxentoollog) $(LDLIBS_libxenstore) $(LDLIBS_libxenctrl) $(LDLIBS_libxenguest) $(LDLIBS_libxenlight) $(APPEND_LDFLAGS)
 
 init-dom0less: $(INIT_DOM0LESS_OBJS)
-	$(CC) $(LDFLAGS) -o $@ $(INIT_DOM0LESS_OBJS) $(LDLIBS_libxenctrl) $(LDLIBS_libxenevtchn) $(LDLIBS_libxentoollog) $(LDLIBS_libxenstore) $(LDLIBS_libxenlight) $(LDLIBS_libxenguest)  $(APPEND_LDFLAGS)
+	$(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
diff --git a/tools/helpers/init-dom0less.c b/tools/helpers/init-dom0less.c
index dc9ccee868..329aa44da6 100644
--- a/tools/helpers/init-dom0less.c
+++ b/tools/helpers/init-dom0less.c
@@ -9,9 +9,12 @@
 #include <xenguest.h>
 #include <libxl.h>
 #include <xenevtchn.h>
+#include <xenforeignmemory.h>
 
 #include "init-dom-json.h"
 
+#define XS_CONNECTION_STATE_OFFSET       (2068/4)
+#define XS_CONNECTION_STATE_RECONNECTING 0x1
 #define XENSTORE_PFN_OFFSET 1
 #define STR_MAX_LENGTH 64
 
@@ -215,12 +218,18 @@ err:
 static int init_domain(struct xs_handle *xsh, libxl_dominfo *info)
 {
     struct xc_interface_core *xch;
+    xenforeignmemory_handle *xfh;
     libxl_uuid uuid;
     uint64_t xenstore_evtchn, xenstore_pfn;
     int rc;
+    uint32_t *page;
 
     printf("Init dom0less domain: %u\n", info->domid);
     xch = xc_interface_open(0, 0, 0);
+    xfh = xenforeignmemory_open(0, 0);
+
+    if (xch == NULL || xfh == NULL)
+        err(1, "Cannot open xc/xenforeignmemory interfaces\n");
 
     rc = xc_hvm_param_get(xch, info->domid, HVM_PARAM_STORE_EVTCHN,
                           &xenstore_evtchn);
@@ -235,6 +244,14 @@ static int init_domain(struct xs_handle *xsh, libxl_dominfo *info)
         return 1;
     }
 
+    page = xenforeignmemory_map(xfh, info->domid, XS_READ | XS_WRITE, 1, &xenstore_pfn, NULL);
+    if (!page) {
+        printf("Error mapping xenstore page\n");
+        return 1;
+    }
+    page[XS_CONNECTION_STATE_OFFSET] = XS_CONNECTION_STATE_RECONNECTING;
+    xenforeignmemory_unmap(xfh, page, 1);
+
     rc = xc_dom_gnttab_seed(xch, info->domid, true,
                             (xen_pfn_t)-1, xenstore_pfn, 0, 0);
     if (rc)
diff --git a/tools/xenstore/xenstored_domain.c b/tools/xenstore/xenstored_domain.c
index 0543f49670..7bb8c64d33 100644
--- a/tools/xenstore/xenstored_domain.c
+++ b/tools/xenstore/xenstored_domain.c
@@ -494,6 +494,7 @@ static struct domain *introduce_domain(const void *ctx,
 		talloc_steal(domain->conn, domain);
 
 		/* Notify the domain that xenstore is available */
+		interface->connection = 0x0;
 		xenevtchn_notify(xce_handle, domain->port);
 
 		if (!is_master_domain && !restore)

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: Type: text/x-diff; NAME=linux.patch, Size: 2076 bytes --]

diff --git a/drivers/xen/xenbus/xenbus_probe.c b/drivers/xen/xenbus/xenbus_probe.c
index 51e52e175892..dc046d25789e 100644
--- a/drivers/xen/xenbus/xenbus_probe.c
+++ b/drivers/xen/xenbus/xenbus_probe.c
@@ -939,6 +939,7 @@ static int __init xenbus_init(void)
 {
 	int err;
 	uint64_t v = 0;
+	bool wait = false;
 	xen_store_domain_type = XS_UNKNOWN;
 
 	if (!xen_domain())
@@ -992,17 +993,7 @@ static int __init xenbus_init(void)
 			goto out_error;
 		}
 		if (v == ~0ULL) {
-			err = bind_evtchn_to_irqhandler(xen_store_evtchn,
-							xenbus_late_init,
-							0, "xenstore_late_init",
-							&xb_waitq);
-			if (err < 0) {
-				pr_err("xenstore_late_init couldn't bind irq err=%d\n",
-				       err);
-				return err;
-			}
-
-			xs_init_irq = err;
+			wait = true;
 		} else {
 			/* Avoid truncation on 32-bit. */
 #if BITS_PER_LONG == 32
@@ -1017,6 +1008,21 @@ static int __init xenbus_init(void)
 			xen_store_interface =
 				xen_remap(xen_store_gfn << XEN_PAGE_SHIFT,
 					  XEN_PAGE_SIZE);
+			if (xen_store_interface->connection != 0)
+				wait = true;
+		}
+		if (wait) {
+			err = bind_evtchn_to_irqhandler(xen_store_evtchn,
+							xenbus_late_init,
+							0, "xenstore_late_init",
+							&xb_waitq);
+			if (err < 0) {
+				pr_err("xenstore_late_init couldn't bind irq err=%d\n",
+				       err);
+				return err;
+			}
+
+			xs_init_irq = err;
 		}
 		break;
 	default:
diff --git a/include/xen/interface/io/xs_wire.h b/include/xen/interface/io/xs_wire.h
index d40a44f09b16..cd7ae5ebb133 100644
--- a/include/xen/interface/io/xs_wire.h
+++ b/include/xen/interface/io/xs_wire.h
@@ -87,6 +87,9 @@ struct xenstore_domain_interface {
     char rsp[XENSTORE_RING_SIZE]; /* Replies and async watch events. */
     XENSTORE_RING_IDX req_cons, req_prod;
     XENSTORE_RING_IDX rsp_cons, rsp_prod;
+    uint32_t server_features; /* Bitmap of features supported by the server */
+    uint32_t connection;
+    uint32_t error;
 };
 
 /* Violating this is very bad.  See docs/misc/xenstore.txt. */