[XEN PATCH v2 0/5] dom0less PV drivers

[XEN PATCH v2 0/5] 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.

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 (2):
      xen: introduce xen,enhanced dom0less property
      xen: export get_free_port

 docs/misc/arm/device-tree/booting.txt |  18 +++
 tools/helpers/Makefile                |  13 ++
 tools/helpers/init-dom0less.c         | 266 ++++++++++++++++++++++++++++++++++
 tools/xenstore/xenstored_domain.c     |   3 +
 xen/arch/arm/domain_build.c           |  51 +++++++
 xen/arch/arm/include/asm/kernel.h     |   3 +
 xen/common/event_channel.c            |   2 +-
 xen/include/xen/event.h               |   3 +
 8 files changed, 358 insertions(+), 1 deletion(-)
 create mode 100644 tools/helpers/init-dom0less.c

[XEN PATCH v2 1/5] xen: introduce xen,enhanced dom0less property

[Stefano Stabellini]

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

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

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

Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
CC: Julien Grall <julien@xen.org>
CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
CC: Bertrand Marquis <bertrand.marquis@arm.com>
---
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           |  8 ++++++++
 xen/arch/arm/include/asm/kernel.h     |  3 +++
 3 files changed, 29 insertions(+)

diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt
index 71895663a4..38c29fb3d8 100644
--- a/docs/misc/arm/device-tree/booting.txt
+++ b/docs/misc/arm/device-tree/booting.txt
@@ -169,6 +169,24 @@ with the following properties:
     Please note that the SPI used for the virtual pl011 could clash with the
     physical SPI of a physical device assigned to the guest.
 
+- xen,enhanced
+
+    A string property. Possible property values are:
+
+    - "enabled" (or missing property value)
+    Xen PV interfaces, including grant-table and xenstore, will be
+    enabled for the VM.
+
+    - "disabled"
+    Xen PV interfaces are disabled.
+
+    If the xen,enhanced property is present with no value, it defaults
+    to "enabled". If the xen,enhanced property is not present, PV
+    interfaces are disabled.
+
+    In the future other possible property values might be added to
+    enable only selected interfaces.
+
 - nr_spis
 
     Optional. A 32-bit integer specifying the number of SPIs (Shared
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 6931c022a2..9144d6c0b6 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -2963,6 +2963,7 @@ static int __init construct_domU(struct domain *d,
                                  const struct dt_device_node *node)
 {
     struct kernel_info kinfo = {};
+    const char *dom0less_enhanced;
     int rc;
     u64 mem;
 
@@ -2978,6 +2979,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;
 
@@ -3095,6 +3102,7 @@ static int __init construct_dom0(struct domain *d)
 
     kinfo.unassigned_mem = dom0_mem;
     kinfo.d = d;
+    kinfo.dom0less_enhanced = true;
 
     rc = kernel_probe(&kinfo, NULL);
     if ( rc < 0 )
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

[XEN PATCH v2 2/5] xen: export get_free_port

[Stefano Stabellini]

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

get_free_port will soon be used to allocate the xenstore event channel
for dom0less domains.

Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
CC: Julien Grall <julien@xen.org>
CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
CC: Bertrand Marquis <bertrand.marquis@arm.com>
CC: Jan Beulich <jbeulich@suse.com>
CC: Andrew Cooper <andrew.cooper3@citrix.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 da88ad141a..5b0bcaaad4 100644
--- a/xen/common/event_channel.c
+++ b/xen/common/event_channel.c
@@ -232,7 +232,7 @@ int evtchn_allocate_port(struct domain *d, evtchn_port_t port)
     return 0;
 }
 
-static int get_free_port(struct domain *d)
+int get_free_port(struct domain *d)
 {
     int            port;
 
diff --git a/xen/include/xen/event.h b/xen/include/xen/event.h
index 21c95e14fd..0b35d9d4d2 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);
 
+/* Get fix free event channel port */
+int get_free_port(struct domain *d);
+
 /* Unmask a local event-channel port. */
 int evtchn_unmask(unsigned int port);
 
-- 
2.25.1

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

[Stefano Stabellini]

From: Luca Miccio <lucmiccio@gmail.com>

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

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

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

Signed-off-by: Luca Miccio <lucmiccio@gmail.com>
Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
CC: Julien Grall <julien@xen.org>
CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
CC: Bertrand Marquis <bertrand.marquis@arm.com>

---
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 | 43 +++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 9144d6c0b6..251d933c8e 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -27,6 +27,7 @@
 #include <asm/setup.h>
 #include <asm/cpufeature.h>
 #include <asm/domain_build.h>
+#include <xen/event.h>
 
 #include <xen/irq.h>
 #include <xen/grant_table.h>
@@ -2619,6 +2620,8 @@ static int __init prepare_dtb_domU(struct domain *d, struct kernel_info *kinfo)
     int ret;
 
     kinfo->phandle_gic = GUEST_PHANDLE_GIC;
+    kinfo->gnttab_start = GUEST_GNTTAB_BASE;
+    kinfo->gnttab_size = GUEST_GNTTAB_SIZE;
 
     addrcells = GUEST_ROOT_ADDRESS_CELLS;
     sizecells = GUEST_ROOT_SIZE_CELLS;
@@ -2693,6 +2696,13 @@ static int __init prepare_dtb_domU(struct domain *d, struct kernel_info *kinfo)
             goto err;
     }
 
+    if ( kinfo->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;
@@ -2959,6 +2969,27 @@ static int __init construct_domain(struct domain *d, struct kernel_info *kinfo)
     return 0;
 }
 
+static int __init alloc_xenstore_evtchn(struct domain *d)
+{
+    struct evtchn *chn;
+    int port;
+
+    if ( (port = get_free_port(d)) < 0 )
+    {
+        printk("Failed allocating event channel for domain\n");
+        return port;
+    }
+    chn = evtchn_from_port(d, port);
+
+    chn->state = ECS_UNBOUND;
+    chn->u.unbound.remote_domid = hardware_domain->domain_id;
+    evtchn_port_init(d, chn);
+
+    d->arch.hvm.params[HVM_PARAM_STORE_EVTCHN] = chn->port;
+
+    return 0;
+}
+
 static int __init construct_domU(struct domain *d,
                                  const struct dt_device_node *node)
 {
@@ -3014,7 +3045,19 @@ static int __init construct_domU(struct domain *d,
         return rc;
 
     if ( kinfo.vpl011 )
+    {
         rc = domain_vpl011_init(d, NULL);
+        if ( rc < 0 )
+            return rc;
+    }
+
+    if ( kinfo.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

[XEN PATCH v2 4/5] 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>
---
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 d03c7d93a9..7487129e47 100644
--- a/tools/xenstore/xenstored_domain.c
+++ b/tools/xenstore/xenstored_domain.c
@@ -461,6 +461,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

[XEN PATCH v2 5/5] 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 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 | 266 ++++++++++++++++++++++++++++++++++
 2 files changed, 279 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..0434ca064f
--- /dev/null
+++ b/tools/helpers/init-dom0less.c
@@ -0,0 +1,266 @@
+#include <stdbool.h>
+#include <syslog.h>
+#include <stdio.h>
+#include <err.h>
+#include <stdlib.h>
+#include <xenstore.h>
+#include <xenctrl.h>
+#include <xenguest.h>
+#include <libxl.h>
+#include <xenevtchn.h>
+
+#include "init-dom-json.h"
+
+#define NR_MAGIC_PAGES 4
+#define CONSOLE_PFN_OFFSET 0
+#define XENSTORE_PFN_OFFSET 1
+#define STR_MAX_LENGTH 64
+
+static int alloc_magic_pages(libxl_dominfo *info, struct xc_dom_image *dom)
+{
+    int rc, i;
+    const xen_pfn_t base = GUEST_MAGIC_BASE >> XC_PAGE_SHIFT;
+    xen_pfn_t p2m[NR_MAGIC_PAGES];
+
+    rc = xc_domain_setmaxmem(dom->xch, dom->guest_domid,
+                             info->max_memkb + NR_MAGIC_PAGES * 4);
+    if (rc < 0)
+        return rc;
+
+    for (i = 0; i < NR_MAGIC_PAGES; i++)
+        p2m[i] = base + i;
+
+    rc = xc_domain_populate_physmap_exact(dom->xch, dom->guest_domid,
+                                          NR_MAGIC_PAGES, 0, 0, p2m);
+    if (rc < 0)
+        return rc;
+
+    dom->xenstore_pfn = base + XENSTORE_PFN_OFFSET;
+
+    xc_clear_domain_page(dom->xch, dom->guest_domid, dom->xenstore_pfn);
+
+    xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_STORE_PFN,
+                     dom->xenstore_pfn);
+    return 0;
+}
+
+static void do_xs_write(struct xs_handle *xsh, xs_transaction_t t,
+                        char *path, char *val)
+{
+    if (!xs_write(xsh, t, path, val, strlen(val)))
+        fprintf(stderr, "writing %s to xenstore failed.\n", path);
+}
+
+static void do_xs_write_dom(struct xs_handle *xsh, xs_transaction_t t,
+                            domid_t domid, char *path, char *val)
+{
+    char full_path[STR_MAX_LENGTH];
+
+    snprintf(full_path, STR_MAX_LENGTH,
+             "/local/domain/%d/%s", domid, path);
+    do_xs_write(xsh, t, full_path, val);
+}
+
+static void do_xs_write_libxl(struct xs_handle *xsh, xs_transaction_t t,
+                              domid_t domid, char *path, char *val)
+{
+    char full_path[STR_MAX_LENGTH];
+
+    snprintf(full_path, STR_MAX_LENGTH,
+             "/libxl/%d/%s", domid, path);
+    do_xs_write(xsh, t, full_path, val);
+}
+
+static void do_xs_write_vm(struct xs_handle *xsh, xs_transaction_t t,
+                           libxl_uuid uuid, char *path, char *val)
+{
+    char full_path[STR_MAX_LENGTH];
+
+    snprintf(full_path, STR_MAX_LENGTH,
+             "/vm/" LIBXL_UUID_FMT "/%s", LIBXL_UUID_BYTES(uuid), path);
+    do_xs_write(xsh, t, full_path, val);
+}
+
+static int restore_xenstore(struct xs_handle *xsh,
+                            libxl_dominfo *info, libxl_uuid uuid,
+                            evtchn_port_t xenstore_port)
+{
+    domid_t domid;
+    int i;
+    char uuid_str[STR_MAX_LENGTH];
+    char dom_name_str[STR_MAX_LENGTH];
+    char vm_val_str[STR_MAX_LENGTH];
+    char id_str[STR_MAX_LENGTH];
+    char max_memkb_str[STR_MAX_LENGTH];
+    char cpu_str[STR_MAX_LENGTH];
+    char xenstore_port_str[STR_MAX_LENGTH];
+    char ring_ref_str[STR_MAX_LENGTH];
+    xs_transaction_t t;
+
+    domid = info->domid;
+    snprintf(id_str, STR_MAX_LENGTH, "%d", domid);
+    snprintf(dom_name_str, STR_MAX_LENGTH, "dom0less-%d", domid);
+    snprintf(uuid_str, STR_MAX_LENGTH, LIBXL_UUID_FMT, LIBXL_UUID_BYTES(uuid));
+    snprintf(vm_val_str, STR_MAX_LENGTH,
+             "vm/" LIBXL_UUID_FMT, LIBXL_UUID_BYTES(uuid));
+    snprintf(max_memkb_str, STR_MAX_LENGTH, "%lu", info->max_memkb);
+    snprintf(ring_ref_str, STR_MAX_LENGTH, "%lld",
+             (GUEST_MAGIC_BASE >> XC_PAGE_SHIFT) + XENSTORE_PFN_OFFSET);
+    snprintf(xenstore_port_str, STR_MAX_LENGTH, "%d", xenstore_port);
+
+retry_transaction:
+    t = xs_transaction_start(xsh);
+    if (t == XBT_NULL)
+        return errno;
+
+    /* /vm */
+    do_xs_write_vm(xsh, t, uuid, "name", dom_name_str);
+    do_xs_write_vm(xsh, t, uuid, "uuid", uuid_str);
+    do_xs_write_vm(xsh, t, uuid, "start_time", "0");
+
+    /* /domain */
+    do_xs_write_dom(xsh, t, domid, "vm", vm_val_str);
+    do_xs_write_dom(xsh, t, domid, "name", dom_name_str);
+    do_xs_write_dom(xsh, t, domid, "cpu", "");
+    for (i = 0; i < info->vcpu_max_id; i++) {
+        snprintf(cpu_str, STR_MAX_LENGTH, "cpu/%d/availability/", i);
+        do_xs_write_dom(xsh, t, domid, cpu_str,
+                        (info->cpupool & (1 << i)) ? "online" : "offline");
+    }
+    do_xs_write_dom(xsh, t, domid, "cpu/0", "");
+    do_xs_write_dom(xsh, t, domid, "cpu/availability", "online");
+
+    do_xs_write_dom(xsh, t, domid, "memory", "");
+    do_xs_write_dom(xsh, t, domid, "memory/static-max", max_memkb_str);
+    do_xs_write_dom(xsh, t, domid, "memory/videoram", "-1");
+
+    do_xs_write_dom(xsh, t, domid, "device", "");
+    do_xs_write_dom(xsh, t, domid, "device/suspend", "");
+    do_xs_write_dom(xsh, t, domid, "device/suspend/event-channel", "");
+
+    do_xs_write_dom(xsh, t, domid, "control", "");
+    do_xs_write_dom(xsh, t, domid, "control/shutdown", "");
+    do_xs_write_dom(xsh, t, domid, "control/feature-poweroff", "1");
+    do_xs_write_dom(xsh, t, domid, "control/feature-reboot", "1");
+    do_xs_write_dom(xsh, t, domid, "control/feature-suspend", "");
+    do_xs_write_dom(xsh, t, domid, "control/sysrq", "");
+    do_xs_write_dom(xsh, t, domid, "control/platform-feature-multiprocessor-suspend", "1");
+    do_xs_write_dom(xsh, t, domid, "control", "platform-feature-xs_reset_watches");
+
+    do_xs_write_dom(xsh, t, domid, "domid", id_str);
+    do_xs_write_dom(xsh, t, domid, "data", "");
+    do_xs_write_dom(xsh, t, domid, "drivers", "");
+    do_xs_write_dom(xsh, t, domid, "feature", "");
+    do_xs_write_dom(xsh, t, domid, "attr", "");
+
+    do_xs_write_dom(xsh, t, domid, "store/port", xenstore_port_str);
+    do_xs_write_dom(xsh, t, domid, "store/ring-ref", ring_ref_str);
+
+    do_xs_write_libxl(xsh, t, domid, "type", "pvh");
+    do_xs_write_libxl(xsh, t, domid, "dm-version", "qemu_xen");
+
+    if (!xs_transaction_end(xsh, t, false))
+        if (errno == EAGAIN)
+            goto retry_transaction;
+
+    return 0;
+}
+
+static int init_domain(struct xs_handle *xsh, libxl_dominfo *info)
+{
+    struct xc_dom_image dom;
+    libxl_uuid uuid;
+    uint64_t v;
+    int rc;
+
+    printf("#### Init dom0less domain: %d ####\n", info->domid);
+    dom.guest_domid = info->domid;
+    dom.xenstore_domid = 0;
+    dom.xch = xc_interface_open(0, 0, 0);
+
+    rc = xc_hvm_param_get(dom.xch, info->domid, HVM_PARAM_STORE_EVTCHN, &v);
+    if (rc != 0) {
+        printf("Failed to get HVM_PARAM_STORE_EVTCHN\n");
+        return 1;
+    }
+    dom.xenstore_evtchn = v;
+
+    /* Console won't be initialized but set its data for completeness */
+    dom.console_domid = 0;
+
+    /* Alloc magic pages */
+    printf("Allocating magic pages\n");
+    if (alloc_magic_pages(info, &dom) != 0) {
+        printf("Error on alloc magic pages\n");
+        return 1;
+    }
+
+    printf("Setup Grant Tables\n");
+    xc_dom_gnttab_init(&dom);
+
+    printf("Setup UUID\n");
+    libxl_uuid_generate(&uuid);
+    xc_domain_sethandle(dom.xch, info->domid, libxl_uuid_bytearray(&uuid));
+
+    printf("Creating JSON\n");
+    rc = gen_stub_json_config(info->domid, &uuid);
+    if (rc)
+        err(1, "gen_stub_json_config");
+
+    printf("Restoring Xenstore values\n");
+    restore_xenstore(xsh, info, uuid, dom.xenstore_evtchn);
+
+    printf("Introducing domain\n");
+    xs_introduce_domain(xsh, info->domid,
+            (GUEST_MAGIC_BASE >> XC_PAGE_SHIFT) + XENSTORE_PFN_OFFSET,
+            dom.xenstore_evtchn);
+    return 0;
+}
+
+/* Check if domain has been configured in XS */
+static bool domain_exists(struct xs_handle *xsh, int domid)
+{
+    return xs_is_domain_introduced(xsh, domid);
+}
+
+int main(int argc, char **argv)
+{
+    libxl_dominfo *info;
+    libxl_ctx *ctx;
+    int nb_vm, i;
+    struct xs_handle *xsh;
+
+    xsh = xs_daemon_open();
+    if (xsh == NULL) {
+        fprintf(stderr, "Could not contact XenStore");
+        exit(1);
+    }
+
+    if (libxl_ctx_alloc(&ctx, LIBXL_VERSION, 0, NULL)) {
+        fprintf(stderr, "cannot init xl context\n");
+        exit(1);
+    }
+
+    info = libxl_list_domain(ctx, &nb_vm);
+    if (!info) {
+        fprintf(stderr, "libxl_list_vm failed.\n");
+        exit(EXIT_FAILURE);
+    }
+
+    for (i = 0; i < nb_vm; i++) {
+        domid_t domid = info[i].domid;
+
+        /* Don't need to check for Dom0 */
+        if (!domid)
+            continue;
+
+        printf("Checking domid: %u\n", domid);
+        if (!domain_exists(xsh, domid))
+            init_domain(xsh, &info[i]);
+        else
+            printf("Domain %d has already been initialized\n", domid);
+    }
+    libxl_dominfo_list_free(info, nb_vm);
+    xs_close(xsh);
+    return 0;
+}
-- 
2.25.1

Re: [XEN PATCH v2 2/5] xen: export get_free_port

[Jan Beulich]

On 13.01.2022 01:58, Stefano Stabellini wrote:
> --- a/xen/common/event_channel.c
> +++ b/xen/common/event_channel.c
> @@ -232,7 +232,7 @@ int evtchn_allocate_port(struct domain *d, evtchn_port_t port)
>      return 0;
>  }
>  
> -static int get_free_port(struct domain *d)
> +int get_free_port(struct domain *d)

The name of the function isn't really suitable for being non-static.
Can't we fold its functionality back into evtchn_allocate_port() (or
the other way around, depending on the perspective you want to take)
in case the caller passes in port 0? (Btw., it is imo wrong for the
loop over ports to start at 0, when it is part of the ABI that port
0 is always invalid. evtchn_init() also better wouldn't depend on it
being the only party to successfully invoke the function getting back
port 0.)

Jan

Re: [XEN PATCH v2 1/5] xen: introduce xen,enhanced dom0less property

[Bertrand Marquis]

Hi Stefano,

> On 13 Jan 2022, at 00:58, Stefano Stabellini <sstabellini@kernel.org> wrote:
> 
> 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.)

You should also say here that you set this option to true in the code for dom0
and that the option is only for DomUs.

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

With the previous added in commit message:
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>

Cheers
Bertrand

> ---
> 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           |  8 ++++++++
> xen/arch/arm/include/asm/kernel.h     |  3 +++
> 3 files changed, 29 insertions(+)
> 
> diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt
> index 71895663a4..38c29fb3d8 100644
> --- a/docs/misc/arm/device-tree/booting.txt
> +++ b/docs/misc/arm/device-tree/booting.txt
> @@ -169,6 +169,24 @@ with the following properties:
>     Please note that the SPI used for the virtual pl011 could clash with the
>     physical SPI of a physical device assigned to the guest.
> 
> +- xen,enhanced
> +
> +    A string property. Possible property values are:
> +
> +    - "enabled" (or missing property value)
> +    Xen PV interfaces, including grant-table and xenstore, will be
> +    enabled for the VM.
> +
> +    - "disabled"
> +    Xen PV interfaces are disabled.
> +
> +    If the xen,enhanced property is present with no value, it defaults
> +    to "enabled". If the xen,enhanced property is not present, PV
> +    interfaces are disabled.
> +
> +    In the future other possible property values might be added to
> +    enable only selected interfaces.
> +
> - nr_spis
> 
>     Optional. A 32-bit integer specifying the number of SPIs (Shared
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 6931c022a2..9144d6c0b6 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -2963,6 +2963,7 @@ static int __init construct_domU(struct domain *d,
>                                  const struct dt_device_node *node)
> {
>     struct kernel_info kinfo = {};
> +    const char *dom0less_enhanced;
>     int rc;
>     u64 mem;
> 
> @@ -2978,6 +2979,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;
> 
> @@ -3095,6 +3102,7 @@ static int __init construct_dom0(struct domain *d)
> 
>     kinfo.unassigned_mem = dom0_mem;
>     kinfo.d = d;
> +    kinfo.dom0less_enhanced = true;
> 
>     rc = kernel_probe(&kinfo, NULL);
>     if ( rc < 0 )
> 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: [XEN PATCH v2 1/5] xen: introduce xen,enhanced dom0less property

[Luca Fancellu]

> On 13 Jan 2022, at 00:58, Stefano Stabellini <sstabellini@kernel.org> wrote:
> 
> From: Stefano Stabellini <stefano.stabellini@xilinx.com>
> 
> Introduce a new "xen,enhanced" dom0less property to enable/disable PV
> driver interfaces for dom0less guests. Currently only "enabled" and
> "disabled" are supported property values (and empty). Leave the option
> open to implement further possible values in the future (e.g.
> "xenstore" to enable only xenstore.)
> 
> This patch only parses the property. Next patches will make use of it.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
> CC: Julien Grall <julien@xen.org>
> CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
> CC: Bertrand Marquis <bertrand.marquis@arm.com>

Hi Stefano,

Subject to Bertrand’s comment on commit message:

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

Just a small curiosity, why we use the prefix “xen,” for the property? I thought since the node uses
a “xen,domain" compatible we could use just “enhanced” just like the other properties “vpl011”, “nr_spis”, ...

Cheers,
Luca

> ---
> 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           |  8 ++++++++
> xen/arch/arm/include/asm/kernel.h     |  3 +++
> 3 files changed, 29 insertions(+)
> 
> diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt
> index 71895663a4..38c29fb3d8 100644
> --- a/docs/misc/arm/device-tree/booting.txt
> +++ b/docs/misc/arm/device-tree/booting.txt
> @@ -169,6 +169,24 @@ with the following properties:
>     Please note that the SPI used for the virtual pl011 could clash with the
>     physical SPI of a physical device assigned to the guest.
> 
> +- xen,enhanced
> +
> +    A string property. Possible property values are:
> +
> +    - "enabled" (or missing property value)
> +    Xen PV interfaces, including grant-table and xenstore, will be
> +    enabled for the VM.
> +
> +    - "disabled"
> +    Xen PV interfaces are disabled.
> +
> +    If the xen,enhanced property is present with no value, it defaults
> +    to "enabled". If the xen,enhanced property is not present, PV
> +    interfaces are disabled.
> +
> +    In the future other possible property values might be added to
> +    enable only selected interfaces.
> +
> - nr_spis
> 
>     Optional. A 32-bit integer specifying the number of SPIs (Shared
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 6931c022a2..9144d6c0b6 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -2963,6 +2963,7 @@ static int __init construct_domU(struct domain *d,
>                                  const struct dt_device_node *node)
> {
>     struct kernel_info kinfo = {};
> +    const char *dom0less_enhanced;
>     int rc;
>     u64 mem;
> 
> @@ -2978,6 +2979,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;
> 
> @@ -3095,6 +3102,7 @@ static int __init construct_dom0(struct domain *d)
> 
>     kinfo.unassigned_mem = dom0_mem;
>     kinfo.d = d;
> +    kinfo.dom0less_enhanced = true;
> 
>     rc = kernel_probe(&kinfo, NULL);
>     if ( rc < 0 )
> 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: [XEN PATCH v2 3/5] xen/arm: configure dom0less domain for enabling xenstore after boot

[Bertrand Marquis]

Hi Stefano,

+ Penny in CC for the question.

> On 13 Jan 2022, at 00:58, Stefano Stabellini <sstabellini@kernel.org> wrote:
> 
> From: Luca Miccio <lucmiccio@gmail.com>
> 
> If "xen,enhanced" is enabled, then add to dom0less domains:
> 
> - the hypervisor node in device tree
> - the xenstore event channel
> 
> The xenstore event channel is also used for the first notification to
> let the guest know that xenstore has become available.
> 
> Signed-off-by: Luca Miccio <lucmiccio@gmail.com>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
> CC: Julien Grall <julien@xen.org>
> CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
> CC: Bertrand Marquis <bertrand.marquis@arm.com>

Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>

Just one question: GUEST_GNTTAB_BASE is fixed but could it be a problem for a direct map guest in the future ?

Regards
Bertrand

> 
> ---
> 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 | 43 +++++++++++++++++++++++++++++++++++++
> 1 file changed, 43 insertions(+)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 9144d6c0b6..251d933c8e 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -27,6 +27,7 @@
> #include <asm/setup.h>
> #include <asm/cpufeature.h>
> #include <asm/domain_build.h>
> +#include <xen/event.h>
> 
> #include <xen/irq.h>
> #include <xen/grant_table.h>
> @@ -2619,6 +2620,8 @@ static int __init prepare_dtb_domU(struct domain *d, struct kernel_info *kinfo)
>     int ret;
> 
>     kinfo->phandle_gic = GUEST_PHANDLE_GIC;
> +    kinfo->gnttab_start = GUEST_GNTTAB_BASE;
> +    kinfo->gnttab_size = GUEST_GNTTAB_SIZE;
> 
>     addrcells = GUEST_ROOT_ADDRESS_CELLS;
>     sizecells = GUEST_ROOT_SIZE_CELLS;
> @@ -2693,6 +2696,13 @@ static int __init prepare_dtb_domU(struct domain *d, struct kernel_info *kinfo)
>             goto err;
>     }
> 
> +    if ( kinfo->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;
> @@ -2959,6 +2969,27 @@ static int __init construct_domain(struct domain *d, struct kernel_info *kinfo)
>     return 0;
> }
> 
> +static int __init alloc_xenstore_evtchn(struct domain *d)
> +{
> +    struct evtchn *chn;
> +    int port;
> +
> +    if ( (port = get_free_port(d)) < 0 )
> +    {
> +        printk("Failed allocating event channel for domain\n");
> +        return port;
> +    }
> +    chn = evtchn_from_port(d, port);
> +
> +    chn->state = ECS_UNBOUND;
> +    chn->u.unbound.remote_domid = hardware_domain->domain_id;
> +    evtchn_port_init(d, chn);
> +
> +    d->arch.hvm.params[HVM_PARAM_STORE_EVTCHN] = chn->port;
> +
> +    return 0;
> +}
> +
> static int __init construct_domU(struct domain *d,
>                                  const struct dt_device_node *node)
> {
> @@ -3014,7 +3045,19 @@ static int __init construct_domU(struct domain *d,
>         return rc;
> 
>     if ( kinfo.vpl011 )
> +    {
>         rc = domain_vpl011_init(d, NULL);
> +        if ( rc < 0 )
> +            return rc;
> +    }
> +
> +    if ( kinfo.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
> 

Re: [XEN PATCH v2 4/5] xenstored: send an evtchn notification on introduce_domain

[Bertrand Marquis]

Hi Stefano,

> On 13 Jan 2022, at 00:58, Stefano Stabellini <sstabellini@kernel.org> 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>

Cheers
Bertrand

> ---
> 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 d03c7d93a9..7487129e47 100644
> --- a/tools/xenstore/xenstored_domain.c
> +++ b/tools/xenstore/xenstored_domain.c
> @@ -461,6 +461,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
> 
> 

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

[Bertrand Marquis]

Hi Stefano,

> On 13 Jan 2022, at 00:58, Stefano Stabellini <sstabellini@kernel.org> wrote:
> 
> From: Luca Miccio <lucmiccio@gmail.com>
> 
> Add an example application that can be run in dom0 to complete the
> dom0less domains initialization so that they can get access to xenstore
> and use PV drivers.
> 
> Signed-off-by: Luca Miccio <lucmiccio@gmail.com>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
> CC: Wei Liu <wl@xen.org>
> CC: Anthony PERARD <anthony.perard@citrix.com>
> CC: Juergen Gross <jgross@suse.com>
> ---
> 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 | 266 ++++++++++++++++++++++++++++++++++
> 2 files changed, 279 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..0434ca064f
> --- /dev/null
> +++ b/tools/helpers/init-dom0less.c
> @@ -0,0 +1,266 @@
> +#include <stdbool.h>
> +#include <syslog.h>
> +#include <stdio.h>
> +#include <err.h>
> +#include <stdlib.h>
> +#include <xenstore.h>
> +#include <xenctrl.h>
> +#include <xenguest.h>
> +#include <libxl.h>
> +#include <xenevtchn.h>
> +
> +#include "init-dom-json.h"
> +
> +#define NR_MAGIC_PAGES 4
> +#define CONSOLE_PFN_OFFSET 0
> +#define XENSTORE_PFN_OFFSET 1
> +#define STR_MAX_LENGTH 64
> +
> +static int alloc_magic_pages(libxl_dominfo *info, struct xc_dom_image *dom)
> +{
> +    int rc, i;
> +    const xen_pfn_t base = GUEST_MAGIC_BASE >> XC_PAGE_SHIFT;
> +    xen_pfn_t p2m[NR_MAGIC_PAGES];
> +
> +    rc = xc_domain_setmaxmem(dom->xch, dom->guest_domid,
> +                             info->max_memkb + NR_MAGIC_PAGES * 4);
> +    if (rc < 0)
> +        return rc;
> +
> +    for (i = 0; i < NR_MAGIC_PAGES; i++)
> +        p2m[i] = base + i;
> +
> +    rc = xc_domain_populate_physmap_exact(dom->xch, dom->guest_domid,
> +                                          NR_MAGIC_PAGES, 0, 0, p2m);
> +    if (rc < 0)
> +        return rc;
> +
> +    dom->xenstore_pfn = base + XENSTORE_PFN_OFFSET;
> +
> +    xc_clear_domain_page(dom->xch, dom->guest_domid, dom->xenstore_pfn);
> +
> +    xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_STORE_PFN,
> +                     dom->xenstore_pfn);
> +    return 0;
> +}
> +
> +static void do_xs_write(struct xs_handle *xsh, xs_transaction_t t,
> +                        char *path, char *val)
> +{
> +    if (!xs_write(xsh, t, path, val, strlen(val)))
> +        fprintf(stderr, "writing %s to xenstore failed.\n", path);

Error here is ignored, do we really want that ?

> +}
> +
> +static void do_xs_write_dom(struct xs_handle *xsh, xs_transaction_t t,
> +                            domid_t domid, char *path, char *val)
> +{
> +    char full_path[STR_MAX_LENGTH];
> +
> +    snprintf(full_path, STR_MAX_LENGTH,
> +             "/local/domain/%d/%s", domid, path);
> +    do_xs_write(xsh, t, full_path, val);
> +}
> +
> +static void do_xs_write_libxl(struct xs_handle *xsh, xs_transaction_t t,
> +                              domid_t domid, char *path, char *val)
> +{
> +    char full_path[STR_MAX_LENGTH];
> +
> +    snprintf(full_path, STR_MAX_LENGTH,
> +             "/libxl/%d/%s", domid, path);
> +    do_xs_write(xsh, t, full_path, val);
> +}
> +
> +static void do_xs_write_vm(struct xs_handle *xsh, xs_transaction_t t,
> +                           libxl_uuid uuid, char *path, char *val)
> +{
> +    char full_path[STR_MAX_LENGTH];
> +
> +    snprintf(full_path, STR_MAX_LENGTH,
> +             "/vm/" LIBXL_UUID_FMT "/%s", LIBXL_UUID_BYTES(uuid), path);
> +    do_xs_write(xsh, t, full_path, val);
> +}
> +
> +static int restore_xenstore(struct xs_handle *xsh,
> +                            libxl_dominfo *info, libxl_uuid uuid,
> +                            evtchn_port_t xenstore_port)
> +{
> +    domid_t domid;
> +    int i;
> +    char uuid_str[STR_MAX_LENGTH];
> +    char dom_name_str[STR_MAX_LENGTH];
> +    char vm_val_str[STR_MAX_LENGTH];
> +    char id_str[STR_MAX_LENGTH];
> +    char max_memkb_str[STR_MAX_LENGTH];
> +    char cpu_str[STR_MAX_LENGTH];
> +    char xenstore_port_str[STR_MAX_LENGTH];
> +    char ring_ref_str[STR_MAX_LENGTH];
> +    xs_transaction_t t;
> +
> +    domid = info->domid;
> +    snprintf(id_str, STR_MAX_LENGTH, "%d", domid);
> +    snprintf(dom_name_str, STR_MAX_LENGTH, "dom0less-%d", domid);
> +    snprintf(uuid_str, STR_MAX_LENGTH, LIBXL_UUID_FMT, LIBXL_UUID_BYTES(uuid));
> +    snprintf(vm_val_str, STR_MAX_LENGTH,
> +             "vm/" LIBXL_UUID_FMT, LIBXL_UUID_BYTES(uuid));
> +    snprintf(max_memkb_str, STR_MAX_LENGTH, "%lu", info->max_memkb);
> +    snprintf(ring_ref_str, STR_MAX_LENGTH, "%lld",
> +             (GUEST_MAGIC_BASE >> XC_PAGE_SHIFT) + XENSTORE_PFN_OFFSET);
> +    snprintf(xenstore_port_str, STR_MAX_LENGTH, "%d", xenstore_port);
> +
> +retry_transaction:
> +    t = xs_transaction_start(xsh);
> +    if (t == XBT_NULL)
> +        return errno;
> +

All the following is very static and for a dummy reviewer (like me) kind of magic.
Could you add a comment on top of this explaining how this list of entries was
created so that someone updating/reviewing this code can make sure it is right ?

> +    /* /vm */
> +    do_xs_write_vm(xsh, t, uuid, "name", dom_name_str);
> +    do_xs_write_vm(xsh, t, uuid, "uuid", uuid_str);
> +    do_xs_write_vm(xsh, t, uuid, "start_time", "0");
> +
> +    /* /domain */
> +    do_xs_write_dom(xsh, t, domid, "vm", vm_val_str);
> +    do_xs_write_dom(xsh, t, domid, "name", dom_name_str);
> +    do_xs_write_dom(xsh, t, domid, "cpu", "");
> +    for (i = 0; i < info->vcpu_max_id; i++) {
> +        snprintf(cpu_str, STR_MAX_LENGTH, "cpu/%d/availability/", i);
> +        do_xs_write_dom(xsh, t, domid, cpu_str,
> +                        (info->cpupool & (1 << i)) ? "online" : "offline");
> +    }
> +    do_xs_write_dom(xsh, t, domid, "cpu/0", "");
> +    do_xs_write_dom(xsh, t, domid, "cpu/availability", "online");
> +
> +    do_xs_write_dom(xsh, t, domid, "memory", "");
> +    do_xs_write_dom(xsh, t, domid, "memory/static-max", max_memkb_str);
> +    do_xs_write_dom(xsh, t, domid, "memory/videoram", "-1");
> +
> +    do_xs_write_dom(xsh, t, domid, "device", "");
> +    do_xs_write_dom(xsh, t, domid, "device/suspend", "");
> +    do_xs_write_dom(xsh, t, domid, "device/suspend/event-channel", "");
> +
> +    do_xs_write_dom(xsh, t, domid, "control", "");
> +    do_xs_write_dom(xsh, t, domid, "control/shutdown", "");
> +    do_xs_write_dom(xsh, t, domid, "control/feature-poweroff", "1");
> +    do_xs_write_dom(xsh, t, domid, "control/feature-reboot", "1");
> +    do_xs_write_dom(xsh, t, domid, "control/feature-suspend", "");
> +    do_xs_write_dom(xsh, t, domid, "control/sysrq", "");
> +    do_xs_write_dom(xsh, t, domid, "control/platform-feature-multiprocessor-suspend", "1");
> +    do_xs_write_dom(xsh, t, domid, "control", "platform-feature-xs_reset_watches");
> +
> +    do_xs_write_dom(xsh, t, domid, "domid", id_str);
> +    do_xs_write_dom(xsh, t, domid, "data", "");
> +    do_xs_write_dom(xsh, t, domid, "drivers", "");
> +    do_xs_write_dom(xsh, t, domid, "feature", "");
> +    do_xs_write_dom(xsh, t, domid, "attr", "");
> +
> +    do_xs_write_dom(xsh, t, domid, "store/port", xenstore_port_str);
> +    do_xs_write_dom(xsh, t, domid, "store/ring-ref", ring_ref_str);
> +
> +    do_xs_write_libxl(xsh, t, domid, "type", "pvh");
> +    do_xs_write_libxl(xsh, t, domid, "dm-version", "qemu_xen");
> +
> +    if (!xs_transaction_end(xsh, t, false))
> +        if (errno == EAGAIN)
> +            goto retry_transaction;
> +
> +    return 0;
> +}
> +
> +static int init_domain(struct xs_handle *xsh, libxl_dominfo *info)
> +{
> +    struct xc_dom_image dom;
> +    libxl_uuid uuid;
> +    uint64_t v;
> +    int rc;
> +
> +    printf("#### Init dom0less domain: %d ####\n", info->domid);
> +    dom.guest_domid = info->domid;
> +    dom.xenstore_domid = 0;
> +    dom.xch = xc_interface_open(0, 0, 0);
> +
> +    rc = xc_hvm_param_get(dom.xch, info->domid, HVM_PARAM_STORE_EVTCHN, &v);
> +    if (rc != 0) {
> +        printf("Failed to get HVM_PARAM_STORE_EVTCHN\n");
> +        return 1;
> +    }
> +    dom.xenstore_evtchn = v;
> +
> +    /* Console won't be initialized but set its data for completeness */
> +    dom.console_domid = 0;
> +
> +    /* Alloc magic pages */
> +    printf("Allocating magic pages\n");
> +    if (alloc_magic_pages(info, &dom) != 0) {
> +        printf("Error on alloc magic pages\n");
> +        return 1;
> +    }
> +
> +    printf("Setup Grant Tables\n");
> +    xc_dom_gnttab_init(&dom);
> +
> +    printf("Setup UUID\n");
> +    libxl_uuid_generate(&uuid);
> +    xc_domain_sethandle(dom.xch, info->domid, libxl_uuid_bytearray(&uuid));
> +
> +    printf("Creating JSON\n");
> +    rc = gen_stub_json_config(info->domid, &uuid);
> +    if (rc)
> +        err(1, "gen_stub_json_config");
> +
> +    printf("Restoring Xenstore values\n");
> +    restore_xenstore(xsh, info, uuid, dom.xenstore_evtchn);
> +
> +    printf("Introducing domain\n");
> +    xs_introduce_domain(xsh, info->domid,
> +            (GUEST_MAGIC_BASE >> XC_PAGE_SHIFT) + XENSTORE_PFN_OFFSET,
> +            dom.xenstore_evtchn);

This is really verbose and you have no option to make this a bit more quiet.
Do we really need all those prints ? Could end up being a lot if you have 5 or 6 domains.

> +    return 0;
> +}
> +
> +/* Check if domain has been configured in XS */
> +static bool domain_exists(struct xs_handle *xsh, int domid)
> +{
> +    return xs_is_domain_introduced(xsh, domid);
> +}
> +
> +int main(int argc, char **argv)
> +{
> +    libxl_dominfo *info;
> +    libxl_ctx *ctx;
> +    int nb_vm, i;
> +    struct xs_handle *xsh;
> +
> +    xsh = xs_daemon_open();
> +    if (xsh == NULL) {
> +        fprintf(stderr, "Could not contact XenStore");
> +        exit(1);
> +    }
> +
> +    if (libxl_ctx_alloc(&ctx, LIBXL_VERSION, 0, NULL)) {
> +        fprintf(stderr, "cannot init xl context\n");

You might want to close xsh to be clean here

> +        exit(1);
> +    }
> +
> +    info = libxl_list_domain(ctx, &nb_vm);
> +    if (!info) {
> +        fprintf(stderr, "libxl_list_vm failed.\n");

You might want to close xsh to be clean here

> +        exit(EXIT_FAILURE);

Why using EXIT_FAILURE here but 1 on the exit before ?

> +    }
> +
> +    for (i = 0; i < nb_vm; i++) {
> +        domid_t domid = info[i].domid;
> +
> +        /* Don't need to check for Dom0 */
> +        if (!domid)
> +            continue;
> +
> +        printf("Checking domid: %u\n", domid);
> +        if (!domain_exists(xsh, domid))
> +            init_domain(xsh, &info[i]);
> +        else
> +            printf("Domain %d has already been initialized\n", domid);
> +    }
> +    libxl_dominfo_list_free(info, nb_vm);
> +    xs_close(xsh);
> +    return 0;
> +}
> -- 
> 2.25.1
> 

Re: [XEN PATCH v2 1/5] xen: introduce xen,enhanced dom0less property

[Stefano Stabellini]

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

On Thu, 13 Jan 2022, Luca Fancellu wrote:
> > On 13 Jan 2022, at 00:58, Stefano Stabellini <sstabellini@kernel.org> wrote:
> > 
> > From: Stefano Stabellini <stefano.stabellini@xilinx.com>
> > 
> > Introduce a new "xen,enhanced" dom0less property to enable/disable PV
> > driver interfaces for dom0less guests. Currently only "enabled" and
> > "disabled" are supported property values (and empty). Leave the option
> > open to implement further possible values in the future (e.g.
> > "xenstore" to enable only xenstore.)
> > 
> > This patch only parses the property. Next patches will make use of it.
> > 
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
> > CC: Julien Grall <julien@xen.org>
> > CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
> > CC: Bertrand Marquis <bertrand.marquis@arm.com>
> 
> Hi Stefano,
> 
> Subject to Bertrand’s comment on commit message:
> 
> Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>
> 
> Just a small curiosity, why we use the prefix “xen,” for the property? I thought since the node uses
> a “xen,domain" compatible we could use just “enhanced” just like the other properties “vpl011”, “nr_spis”, ...

Julien was the one to suggest the naming but I actually like it.

Yes, if we wanted we could use "enhanced" but I think it is a better
idea to call this property "xen,enhanced" because that way it is easily
distinguishable from properties that could be hypervisor-neutral.

Imagine one day we have a device tree spec for a generic hypervisor
domain. It could have things like memory and even vpl011. However, for
sure it is not going to have "Xen PV drivers enabled". From a device
tree perspective it is better to align properties as much as possible
between similar nodes of different vendors, so ideally "memory" is going
ot be called "memory" for all hypervisor vendors. So it makes sense for
this property to have the "xen," prefix as it is different from the ones
that are more neutral.


> > ---
> > 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           |  8 ++++++++
> > xen/arch/arm/include/asm/kernel.h     |  3 +++
> > 3 files changed, 29 insertions(+)
> > 
> > diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt
> > index 71895663a4..38c29fb3d8 100644
> > --- a/docs/misc/arm/device-tree/booting.txt
> > +++ b/docs/misc/arm/device-tree/booting.txt
> > @@ -169,6 +169,24 @@ with the following properties:
> >     Please note that the SPI used for the virtual pl011 could clash with the
> >     physical SPI of a physical device assigned to the guest.
> > 
> > +- xen,enhanced
> > +
> > +    A string property. Possible property values are:
> > +
> > +    - "enabled" (or missing property value)
> > +    Xen PV interfaces, including grant-table and xenstore, will be
> > +    enabled for the VM.
> > +
> > +    - "disabled"
> > +    Xen PV interfaces are disabled.
> > +
> > +    If the xen,enhanced property is present with no value, it defaults
> > +    to "enabled". If the xen,enhanced property is not present, PV
> > +    interfaces are disabled.
> > +
> > +    In the future other possible property values might be added to
> > +    enable only selected interfaces.
> > +
> > - nr_spis
> > 
> >     Optional. A 32-bit integer specifying the number of SPIs (Shared
> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > index 6931c022a2..9144d6c0b6 100644
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -2963,6 +2963,7 @@ static int __init construct_domU(struct domain *d,
> >                                  const struct dt_device_node *node)
> > {
> >     struct kernel_info kinfo = {};
> > +    const char *dom0less_enhanced;
> >     int rc;
> >     u64 mem;
> > 
> > @@ -2978,6 +2979,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;
> > 
> > @@ -3095,6 +3102,7 @@ static int __init construct_dom0(struct domain *d)
> > 
> >     kinfo.unassigned_mem = dom0_mem;
> >     kinfo.d = d;
> > +    kinfo.dom0less_enhanced = true;
> > 
> >     rc = kernel_probe(&kinfo, NULL);
> >     if ( rc < 0 )
> > 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: [XEN PATCH v2 2/5] xen: export get_free_port

[Stefano Stabellini]

On Thu, 13 Jan 2022, Jan Beulich wrote:
> On 13.01.2022 01:58, Stefano Stabellini wrote:
> > --- a/xen/common/event_channel.c
> > +++ b/xen/common/event_channel.c
> > @@ -232,7 +232,7 @@ int evtchn_allocate_port(struct domain *d, evtchn_port_t port)
> >      return 0;
> >  }
> >  
> > -static int get_free_port(struct domain *d)
> > +int get_free_port(struct domain *d)
> 
> The name of the function isn't really suitable for being non-static.
> Can't we fold its functionality back into evtchn_allocate_port() (or
> the other way around, depending on the perspective you want to take)
> in case the caller passes in port 0? (Btw., it is imo wrong for the
> loop over ports to start at 0, when it is part of the ABI that port
> 0 is always invalid. evtchn_init() also better wouldn't depend on it
> being the only party to successfully invoke the function getting back
> port 0.)

I agree that "get_free_port" is not a great name for a non-static
function. Also, it should be noted that for the sake of this patch
series I could just call evtchn_allocate_port(d, 1) given that it is the
first evtchn to be allocated. So maybe, that's the best way forward?


To address your specific suggestion, in my opinion it would be best to
have two different functions to allocate a new port:
- one with a specific evtchn_port_t port parameter
- one without it (meaning: "I don't care about the number")

Folding the functionality of "give me any number" when 0 is passed to
evtchn_allocate_port() doesn't seem to be an improvement to the API to
me.

That said, I am still OK with making the suggested change if that's what
you prefer.

Another alternative is simply to rename "get_free_port" to
"evtchn_allocate" (or something else to distinguish it from
evtchn_allocate_port).

Re: [XEN PATCH v2 1/5] xen: introduce xen,enhanced dom0less property

[Stefano Stabellini]

On Thu, 13 Jan 2022, Bertrand Marquis wrote:
> Hi Stefano,
> 
> > On 13 Jan 2022, at 00:58, Stefano Stabellini <sstabellini@kernel.org> wrote:
> > 
> > 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.)
> 
> You should also say here that you set this option to true in the code for dom0
> and that the option is only for DomUs.

Good point!
 

> > 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>
> 
> With the previous added in commit message:
> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>

Thank you!


> Cheers
> Bertrand
> 
> > ---
> > 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           |  8 ++++++++
> > xen/arch/arm/include/asm/kernel.h     |  3 +++
> > 3 files changed, 29 insertions(+)
> > 
> > diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt
> > index 71895663a4..38c29fb3d8 100644
> > --- a/docs/misc/arm/device-tree/booting.txt
> > +++ b/docs/misc/arm/device-tree/booting.txt
> > @@ -169,6 +169,24 @@ with the following properties:
> >     Please note that the SPI used for the virtual pl011 could clash with the
> >     physical SPI of a physical device assigned to the guest.
> > 
> > +- xen,enhanced
> > +
> > +    A string property. Possible property values are:
> > +
> > +    - "enabled" (or missing property value)
> > +    Xen PV interfaces, including grant-table and xenstore, will be
> > +    enabled for the VM.
> > +
> > +    - "disabled"
> > +    Xen PV interfaces are disabled.
> > +
> > +    If the xen,enhanced property is present with no value, it defaults
> > +    to "enabled". If the xen,enhanced property is not present, PV
> > +    interfaces are disabled.
> > +
> > +    In the future other possible property values might be added to
> > +    enable only selected interfaces.
> > +
> > - nr_spis
> > 
> >     Optional. A 32-bit integer specifying the number of SPIs (Shared
> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > index 6931c022a2..9144d6c0b6 100644
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -2963,6 +2963,7 @@ static int __init construct_domU(struct domain *d,
> >                                  const struct dt_device_node *node)
> > {
> >     struct kernel_info kinfo = {};
> > +    const char *dom0less_enhanced;
> >     int rc;
> >     u64 mem;
> > 
> > @@ -2978,6 +2979,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;
> > 
> > @@ -3095,6 +3102,7 @@ static int __init construct_dom0(struct domain *d)
> > 
> >     kinfo.unassigned_mem = dom0_mem;
> >     kinfo.d = d;
> > +    kinfo.dom0less_enhanced = true;
> > 
> >     rc = kernel_probe(&kinfo, NULL);
> >     if ( rc < 0 )
> > 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: [XEN PATCH v2 2/5] xen: export get_free_port

[Jan Beulich]

On 14.01.2022 02:20, Stefano Stabellini wrote:
> On Thu, 13 Jan 2022, Jan Beulich wrote:
>> On 13.01.2022 01:58, Stefano Stabellini wrote:
>>> --- a/xen/common/event_channel.c
>>> +++ b/xen/common/event_channel.c
>>> @@ -232,7 +232,7 @@ int evtchn_allocate_port(struct domain *d, evtchn_port_t port)
>>>      return 0;
>>>  }
>>>  
>>> -static int get_free_port(struct domain *d)
>>> +int get_free_port(struct domain *d)
>>
>> The name of the function isn't really suitable for being non-static.
>> Can't we fold its functionality back into evtchn_allocate_port() (or
>> the other way around, depending on the perspective you want to take)
>> in case the caller passes in port 0? (Btw., it is imo wrong for the
>> loop over ports to start at 0, when it is part of the ABI that port
>> 0 is always invalid. evtchn_init() also better wouldn't depend on it
>> being the only party to successfully invoke the function getting back
>> port 0.)
> 
> I agree that "get_free_port" is not a great name for a non-static
> function. Also, it should be noted that for the sake of this patch
> series I could just call evtchn_allocate_port(d, 1) given that it is the
> first evtchn to be allocated. So maybe, that's the best way forward?
> 
> 
> To address your specific suggestion, in my opinion it would be best to
> have two different functions to allocate a new port:
> - one with a specific evtchn_port_t port parameter
> - one without it (meaning: "I don't care about the number")
> 
> Folding the functionality of "give me any number" when 0 is passed to
> evtchn_allocate_port() doesn't seem to be an improvement to the API to
> me.

I view it the other way around - that way the function would actually
start matching its name. So far it's more like marking a given port
number as in use, rather than allocating.

> That said, I am still OK with making the suggested change if that's what
> you prefer.

Given experience, hoping for others to voice an opinion isn't likely
to become reality.

Jan

Re: [XEN PATCH v2 2/5] xen: export get_free_port

[Julien Grall]

Hi Stefano,

On 13/01/2022 04:58, Stefano Stabellini wrote:
> From: Stefano Stabellini <stefano.stabellini@xilinx.com>
> 
> get_free_port will soon be used to allocate the xenstore event channel
> for dom0less domains.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
> CC: Julien Grall <julien@xen.org>
> CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
> CC: Bertrand Marquis <bertrand.marquis@arm.com>
> CC: Jan Beulich <jbeulich@suse.com>
> CC: Andrew Cooper <andrew.cooper3@citrix.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 da88ad141a..5b0bcaaad4 100644
> --- a/xen/common/event_channel.c
> +++ b/xen/common/event_channel.c
> @@ -232,7 +232,7 @@ int evtchn_allocate_port(struct domain *d, evtchn_port_t port)
>       return 0;
>   }
>   
> -static int get_free_port(struct domain *d)
> +int get_free_port(struct domain *d)

I dislike the idea to expose get_free_port() (or whichever name we 
decide) because this can be easily misused.

In fact looking at your next patch (#3), you are misusing it as it is 
meant to be called with d->event_lock. I know this doesn't much matter
in your situation because this is done at boot with no other domains 
running (or potentially any event channel allocation). However, I still 
think we should get the API right.

I am also not entirely happy of open-coding the allocation in 
domain_build.c. Instead, I would prefer if we provide a new helper to 
allocate an unbound event channel. This would be similar to your v1 (I 
still need to review the patch though).

Cheers,

-- 
Julien Grall

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

[Julien Grall]

Hi,

On 13/01/2022 14:15, Bertrand Marquis wrote:
> Hi Stefano,
> 
> + Penny in CC for the question.
> 
>> On 13 Jan 2022, at 00:58, Stefano Stabellini <sstabellini@kernel.org> wrote:
>>
>> From: Luca Miccio <lucmiccio@gmail.com>
>>
>> If "xen,enhanced" is enabled, then add to dom0less domains:
>>
>> - the hypervisor node in device tree
>> - the xenstore event channel
>>
>> The xenstore event channel is also used for the first notification to
>> let the guest know that xenstore has become available.
>>
>> Signed-off-by: Luca Miccio <lucmiccio@gmail.com>
>> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
>> CC: Julien Grall <julien@xen.org>
>> CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
>> CC: Bertrand Marquis <bertrand.marquis@arm.com>
> 
> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
> 
> Just one question: GUEST_GNTTAB_BASE is fixed but could it be a problem for a direct map guest in the future ?
It will be an issue. I think we can re-use the same method as we do in 
dom0 (see find_gnttab_region()).

Cheers,

-- 
Julien Grall

Re: [XEN PATCH v2 2/5] xen: export get_free_port

[Stefano Stabellini]

On Sun, 23 Jan 2022, Julien Grall wrote:
> > diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
> > index da88ad141a..5b0bcaaad4 100644
> > --- a/xen/common/event_channel.c
> > +++ b/xen/common/event_channel.c
> > @@ -232,7 +232,7 @@ int evtchn_allocate_port(struct domain *d, evtchn_port_t
> > port)
> >       return 0;
> >   }
> >   -static int get_free_port(struct domain *d)
> > +int get_free_port(struct domain *d)
> 
> I dislike the idea to expose get_free_port() (or whichever name we decide)
> because this can be easily misused.
> 
> In fact looking at your next patch (#3), you are misusing it as it is meant to
> be called with d->event_lock. I know this doesn't much matter
> in your situation because this is done at boot with no other domains running
> (or potentially any event channel allocation). However, I still think we
> should get the API right.
> 
> I am also not entirely happy of open-coding the allocation in domain_build.c.
> Instead, I would prefer if we provide a new helper to allocate an unbound
> event channel. This would be similar to your v1 (I still need to review the
> patch though).

I am happy to go back to v1 and address feedback on that patch. However,
I am having difficulties with the implementation. Jan pointed out:


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

This makes it difficult to reuse _evtchn_alloc_unbound for the
implementation of evtchn_alloc_unbound. In fact, I couldn't find a way
to do it.

Instead, I just create a new public function called
"evtchn_alloc_unbound" and renamed the existing funtion to
"_evtchn_alloc_unbound" (this to addresses Jan's feedback that the
static function should be the one starting with "_"). So the function
names are inverted compared to v1.

Please let me know if you have any better suggestions.


diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
index da88ad141a..c6b7dd7fbd 100644
--- a/xen/common/event_channel.c
+++ b/xen/common/event_channel.c
@@ -18,6 +18,7 @@
 
 #include <xen/init.h>
 #include <xen/lib.h>
+#include <xen/err.h>
 #include <xen/errno.h>
 #include <xen/sched.h>
 #include <xen/irq.h>
@@ -284,7 +285,27 @@ void evtchn_free(struct domain *d, struct evtchn *chn)
     xsm_evtchn_close_post(chn);
 }
 
-static int evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc)
+struct evtchn *evtchn_alloc_unbound(struct domain *d, domid_t remote_dom)
+{
+    struct evtchn *chn;
+    int port;
+
+    if ( (port = get_free_port(d)) < 0 )
+        return ERR_PTR(port);
+    chn = evtchn_from_port(d, port);
+
+    evtchn_write_lock(chn);
+
+    chn->state = ECS_UNBOUND;
+    chn->u.unbound.remote_domid = remote_dom;
+    evtchn_port_init(d, chn);
+
+    evtchn_write_unlock(chn);
+
+    return chn;
+}
+
+static int _evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc)
 {
     struct evtchn *chn;
     struct domain *d;
@@ -1195,7 +1216,7 @@ long do_event_channel_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
         struct evtchn_alloc_unbound alloc_unbound;
         if ( copy_from_guest(&alloc_unbound, arg, 1) != 0 )
             return -EFAULT;
-        rc = evtchn_alloc_unbound(&alloc_unbound);
+        rc = _evtchn_alloc_unbound(&alloc_unbound);
         if ( !rc && __copy_to_guest(arg, &alloc_unbound, 1) )
             rc = -EFAULT; /* Cleaning up here would be a mess! */
         break;
diff --git a/xen/include/xen/event.h b/xen/include/xen/event.h
index 21c95e14fd..85dcf1d0c4 100644
--- a/xen/include/xen/event.h
+++ b/xen/include/xen/event.h
@@ -68,6 +68,9 @@ int evtchn_close(struct domain *d1, int port1, bool guest);
 /* Free an event channel. */
 void evtchn_free(struct domain *d, struct evtchn *chn);
 
+/* Create a new event channel port */
+struct evtchn *evtchn_alloc_unbound(struct domain *d, domid_t remote_dom);
+
 /* Allocate a specific event channel port. */
 int evtchn_allocate_port(struct domain *d, unsigned int port);
 

Re: [XEN PATCH v2 2/5] xen: export get_free_port

[Jan Beulich]

On 25.01.2022 02:10, Stefano Stabellini wrote:
> On Sun, 23 Jan 2022, Julien Grall wrote:
>>> diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
>>> index da88ad141a..5b0bcaaad4 100644
>>> --- a/xen/common/event_channel.c
>>> +++ b/xen/common/event_channel.c
>>> @@ -232,7 +232,7 @@ int evtchn_allocate_port(struct domain *d, evtchn_port_t
>>> port)
>>>       return 0;
>>>   }
>>>   -static int get_free_port(struct domain *d)
>>> +int get_free_port(struct domain *d)
>>
>> I dislike the idea to expose get_free_port() (or whichever name we decide)
>> because this can be easily misused.
>>
>> In fact looking at your next patch (#3), you are misusing it as it is meant to
>> be called with d->event_lock. I know this doesn't much matter
>> in your situation because this is done at boot with no other domains running
>> (or potentially any event channel allocation). However, I still think we
>> should get the API right.
>>
>> I am also not entirely happy of open-coding the allocation in domain_build.c.
>> Instead, I would prefer if we provide a new helper to allocate an unbound
>> event channel. This would be similar to your v1 (I still need to review the
>> patch though).
> 
> I am happy to go back to v1 and address feedback on that patch. However,
> I am having difficulties with the implementation. Jan pointed out:
> 
> 
>>> -
>>> -    chn->state = ECS_UNBOUND;
>>
>> This cannot be pulled ahead of the XSM check (or in general anything
>> potentially resulting in an error), as check_free_port() relies on
>> ->state remaining ECS_FREE until it is known that the calling function
>> can't fail anymore.
> 
> This makes it difficult to reuse _evtchn_alloc_unbound for the
> implementation of evtchn_alloc_unbound. In fact, I couldn't find a way
> to do it.
> 
> Instead, I just create a new public function called
> "evtchn_alloc_unbound" and renamed the existing funtion to
> "_evtchn_alloc_unbound" (this to addresses Jan's feedback that the
> static function should be the one starting with "_"). So the function
> names are inverted compared to v1.
> 
> Please let me know if you have any better suggestions.
> 
> 
> diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
> index da88ad141a..c6b7dd7fbd 100644
> --- a/xen/common/event_channel.c
> +++ b/xen/common/event_channel.c
> @@ -18,6 +18,7 @@
>  
>  #include <xen/init.h>
>  #include <xen/lib.h>
> +#include <xen/err.h>
>  #include <xen/errno.h>
>  #include <xen/sched.h>
>  #include <xen/irq.h>
> @@ -284,7 +285,27 @@ void evtchn_free(struct domain *d, struct evtchn *chn)
>      xsm_evtchn_close_post(chn);
>  }
>  
> -static int evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc)
> +struct evtchn *evtchn_alloc_unbound(struct domain *d, domid_t remote_dom)
> +{
> +    struct evtchn *chn;
> +    int port;
> +
> +    if ( (port = get_free_port(d)) < 0 )
> +        return ERR_PTR(port);
> +    chn = evtchn_from_port(d, port);
> +
> +    evtchn_write_lock(chn);
> +
> +    chn->state = ECS_UNBOUND;
> +    chn->u.unbound.remote_domid = remote_dom;
> +    evtchn_port_init(d, chn);
> +
> +    evtchn_write_unlock(chn);
> +
> +    return chn;
> +}
> +
> +static int _evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc)
>  {
>      struct evtchn *chn;
>      struct domain *d;

Instead of introducing a clone of this function (with, btw, still
insufficient locking), did you consider simply using the existing
evtchn_alloc_unbound() as-is, i.e. with the caller passing
evtchn_alloc_unbound_t *?

Jan

Re: [XEN PATCH v2 2/5] xen: export get_free_port

[Julien Grall]

Hi Jan,

On 25/01/2022 08:22, Jan Beulich wrote:
> On 25.01.2022 02:10, Stefano Stabellini wrote:
>> On Sun, 23 Jan 2022, Julien Grall wrote:
>>>> diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
>>>> index da88ad141a..5b0bcaaad4 100644
>>>> --- a/xen/common/event_channel.c
>>>> +++ b/xen/common/event_channel.c
>>>> @@ -232,7 +232,7 @@ int evtchn_allocate_port(struct domain *d, evtchn_port_t
>>>> port)
>>>>        return 0;
>>>>    }
>>>>    -static int get_free_port(struct domain *d)
>>>> +int get_free_port(struct domain *d)
>>>
>>> I dislike the idea to expose get_free_port() (or whichever name we decide)
>>> because this can be easily misused.
>>>
>>> In fact looking at your next patch (#3), you are misusing it as it is meant to
>>> be called with d->event_lock. I know this doesn't much matter
>>> in your situation because this is done at boot with no other domains running
>>> (or potentially any event channel allocation). However, I still think we
>>> should get the API right.
>>>
>>> I am also not entirely happy of open-coding the allocation in domain_build.c.
>>> Instead, I would prefer if we provide a new helper to allocate an unbound
>>> event channel. This would be similar to your v1 (I still need to review the
>>> patch though).
>>
>> I am happy to go back to v1 and address feedback on that patch. However,
>> I am having difficulties with the implementation. Jan pointed out:
>>
>>
>>>> -
>>>> -    chn->state = ECS_UNBOUND;
>>>
>>> This cannot be pulled ahead of the XSM check (or in general anything
>>> potentially resulting in an error), as check_free_port() relies on
>>> ->state remaining ECS_FREE until it is known that the calling function
>>> can't fail anymore.
>>
>> This makes it difficult to reuse _evtchn_alloc_unbound for the
>> implementation of evtchn_alloc_unbound. In fact, I couldn't find a way
>> to do it.
>>
>> Instead, I just create a new public function called
>> "evtchn_alloc_unbound" and renamed the existing funtion to
>> "_evtchn_alloc_unbound" (this to addresses Jan's feedback that the
>> static function should be the one starting with "_"). So the function
>> names are inverted compared to v1.
>>
>> Please let me know if you have any better suggestions.
>>
>>
>> diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
>> index da88ad141a..c6b7dd7fbd 100644
>> --- a/xen/common/event_channel.c
>> +++ b/xen/common/event_channel.c
>> @@ -18,6 +18,7 @@
>>   
>>   #include <xen/init.h>
>>   #include <xen/lib.h>
>> +#include <xen/err.h>
>>   #include <xen/errno.h>
>>   #include <xen/sched.h>
>>   #include <xen/irq.h>
>> @@ -284,7 +285,27 @@ void evtchn_free(struct domain *d, struct evtchn *chn)
>>       xsm_evtchn_close_post(chn);
>>   }
>>   
>> -static int evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc)
>> +struct evtchn *evtchn_alloc_unbound(struct domain *d, domid_t remote_dom)
>> +{
>> +    struct evtchn *chn;
>> +    int port;
>> +
>> +    if ( (port = get_free_port(d)) < 0 )
>> +        return ERR_PTR(port);
>> +    chn = evtchn_from_port(d, port);
>> +
>> +    evtchn_write_lock(chn);
>> +
>> +    chn->state = ECS_UNBOUND;
>> +    chn->u.unbound.remote_domid = remote_dom;
>> +    evtchn_port_init(d, chn);
>> +
>> +    evtchn_write_unlock(chn);
>> +
>> +    return chn;
>> +}
>> +
>> +static int _evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc)
>>   {
>>       struct evtchn *chn;
>>       struct domain *d;
> 
> Instead of introducing a clone of this function (with, btw, still
> insufficient locking), did you consider simply using the existing
> evtchn_alloc_unbound() as-is, i.e. with the caller passing
> evtchn_alloc_unbound_t *?

This is feasible with some tweaking. Which reminds me that I have a 
similar patch to what you describe:

https://xenbits.xen.org/gitweb/?p=people/julieng/xen-unstable.git;a=commit;h=560d656a9a792450530eeefd0d06cfd54dcd7685

This is doing more than what we need here as it takes care about 
restoring a port (for Live-Update).

Note that They are forward port from 4.11 to unstable and untested on 
the latter.

Cheers,

-- 
Julien Grall

Re: [XEN PATCH v2 2/5] xen: export get_free_port

[Stefano Stabellini]

On Tue, 25 Jan 2022, Jan Beulich wrote:
> On 25.01.2022 02:10, Stefano Stabellini wrote:
> > On Sun, 23 Jan 2022, Julien Grall wrote:
> >>> diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
> >>> index da88ad141a..5b0bcaaad4 100644
> >>> --- a/xen/common/event_channel.c
> >>> +++ b/xen/common/event_channel.c
> >>> @@ -232,7 +232,7 @@ int evtchn_allocate_port(struct domain *d, evtchn_port_t
> >>> port)
> >>>       return 0;
> >>>   }
> >>>   -static int get_free_port(struct domain *d)
> >>> +int get_free_port(struct domain *d)
> >>
> >> I dislike the idea to expose get_free_port() (or whichever name we decide)
> >> because this can be easily misused.
> >>
> >> In fact looking at your next patch (#3), you are misusing it as it is meant to
> >> be called with d->event_lock. I know this doesn't much matter
> >> in your situation because this is done at boot with no other domains running
> >> (or potentially any event channel allocation). However, I still think we
> >> should get the API right.
> >>
> >> I am also not entirely happy of open-coding the allocation in domain_build.c.
> >> Instead, I would prefer if we provide a new helper to allocate an unbound
> >> event channel. This would be similar to your v1 (I still need to review the
> >> patch though).
> > 
> > I am happy to go back to v1 and address feedback on that patch. However,
> > I am having difficulties with the implementation. Jan pointed out:
> > 
> > 
> >>> -
> >>> -    chn->state = ECS_UNBOUND;
> >>
> >> This cannot be pulled ahead of the XSM check (or in general anything
> >> potentially resulting in an error), as check_free_port() relies on
> >> ->state remaining ECS_FREE until it is known that the calling function
> >> can't fail anymore.
> > 
> > This makes it difficult to reuse _evtchn_alloc_unbound for the
> > implementation of evtchn_alloc_unbound. In fact, I couldn't find a way
> > to do it.
> > 
> > Instead, I just create a new public function called
> > "evtchn_alloc_unbound" and renamed the existing funtion to
> > "_evtchn_alloc_unbound" (this to addresses Jan's feedback that the
> > static function should be the one starting with "_"). So the function
> > names are inverted compared to v1.
> > 
> > Please let me know if you have any better suggestions.
> > 
> > 
> > diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
> > index da88ad141a..c6b7dd7fbd 100644
> > --- a/xen/common/event_channel.c
> > +++ b/xen/common/event_channel.c
> > @@ -18,6 +18,7 @@
> >  
> >  #include <xen/init.h>
> >  #include <xen/lib.h>
> > +#include <xen/err.h>
> >  #include <xen/errno.h>
> >  #include <xen/sched.h>
> >  #include <xen/irq.h>
> > @@ -284,7 +285,27 @@ void evtchn_free(struct domain *d, struct evtchn *chn)
> >      xsm_evtchn_close_post(chn);
> >  }
> >  
> > -static int evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc)
> > +struct evtchn *evtchn_alloc_unbound(struct domain *d, domid_t remote_dom)
> > +{
> > +    struct evtchn *chn;
> > +    int port;
> > +
> > +    if ( (port = get_free_port(d)) < 0 )
> > +        return ERR_PTR(port);
> > +    chn = evtchn_from_port(d, port);
> > +
> > +    evtchn_write_lock(chn);
> > +
> > +    chn->state = ECS_UNBOUND;
> > +    chn->u.unbound.remote_domid = remote_dom;
> > +    evtchn_port_init(d, chn);
> > +
> > +    evtchn_write_unlock(chn);
> > +
> > +    return chn;
> > +}
> > +
> > +static int _evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc)
> >  {
> >      struct evtchn *chn;
> >      struct domain *d;
> 
> Instead of introducing a clone of this function (with, btw, still
> insufficient locking), did you consider simply using the existing
> evtchn_alloc_unbound() as-is, i.e. with the caller passing
> evtchn_alloc_unbound_t *?

Yes, we tried that first. Unfortunately the (dummy) XSM check cannot
work. This is how we would want to call the function:


    alloc.dom = d->domain_id;
    alloc.remote_dom = hardware_domain->domain_id;
    rc = evtchn_alloc_unbound(&alloc);


This is the implementation of the XSM check:

static XSM_INLINE int xsm_evtchn_unbound(
    XSM_DEFAULT_ARG struct domain *d, struct evtchn *chn, domid_t id2)
{
    XSM_ASSERT_ACTION(XSM_TARGET);
    return xsm_default_action(action, current->domain, d);
}


Note the usage of current->domain. If you have any suggestions on how to
fix it please let me know.

Re: [XEN PATCH v2 2/5] xen: export get_free_port

[Julien Grall]

Hi Stefano,

On 25/01/2022 22:49, Stefano Stabellini wrote:
> On Tue, 25 Jan 2022, Jan Beulich wrote:
>> On 25.01.2022 02:10, Stefano Stabellini wrote:
>>> On Sun, 23 Jan 2022, Julien Grall wrote:
>>>>> diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
>>>>> index da88ad141a..5b0bcaaad4 100644
>>>>> --- a/xen/common/event_channel.c
>>>>> +++ b/xen/common/event_channel.c
>>>>> @@ -232,7 +232,7 @@ int evtchn_allocate_port(struct domain *d, evtchn_port_t
>>>>> port)
>>>>>        return 0;
>>>>>    }
>>>>>    -static int get_free_port(struct domain *d)
>>>>> +int get_free_port(struct domain *d)
>>>>
>>>> I dislike the idea to expose get_free_port() (or whichever name we decide)
>>>> because this can be easily misused.
>>>>
>>>> In fact looking at your next patch (#3), you are misusing it as it is meant to
>>>> be called with d->event_lock. I know this doesn't much matter
>>>> in your situation because this is done at boot with no other domains running
>>>> (or potentially any event channel allocation). However, I still think we
>>>> should get the API right.
>>>>
>>>> I am also not entirely happy of open-coding the allocation in domain_build.c.
>>>> Instead, I would prefer if we provide a new helper to allocate an unbound
>>>> event channel. This would be similar to your v1 (I still need to review the
>>>> patch though).
>>>
>>> I am happy to go back to v1 and address feedback on that patch. However,
>>> I am having difficulties with the implementation. Jan pointed out:
>>>
>>>
>>>>> -
>>>>> -    chn->state = ECS_UNBOUND;
>>>>
>>>> This cannot be pulled ahead of the XSM check (or in general anything
>>>> potentially resulting in an error), as check_free_port() relies on
>>>> ->state remaining ECS_FREE until it is known that the calling function
>>>> can't fail anymore.
>>>
>>> This makes it difficult to reuse _evtchn_alloc_unbound for the
>>> implementation of evtchn_alloc_unbound. In fact, I couldn't find a way
>>> to do it.
>>>
>>> Instead, I just create a new public function called
>>> "evtchn_alloc_unbound" and renamed the existing funtion to
>>> "_evtchn_alloc_unbound" (this to addresses Jan's feedback that the
>>> static function should be the one starting with "_"). So the function
>>> names are inverted compared to v1.
>>>
>>> Please let me know if you have any better suggestions.
>>>
>>>
>>> diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
>>> index da88ad141a..c6b7dd7fbd 100644
>>> --- a/xen/common/event_channel.c
>>> +++ b/xen/common/event_channel.c
>>> @@ -18,6 +18,7 @@
>>>   
>>>   #include <xen/init.h>
>>>   #include <xen/lib.h>
>>> +#include <xen/err.h>
>>>   #include <xen/errno.h>
>>>   #include <xen/sched.h>
>>>   #include <xen/irq.h>
>>> @@ -284,7 +285,27 @@ void evtchn_free(struct domain *d, struct evtchn *chn)
>>>       xsm_evtchn_close_post(chn);
>>>   }
>>>   
>>> -static int evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc)
>>> +struct evtchn *evtchn_alloc_unbound(struct domain *d, domid_t remote_dom)
>>> +{
>>> +    struct evtchn *chn;
>>> +    int port;
>>> +
>>> +    if ( (port = get_free_port(d)) < 0 )
>>> +        return ERR_PTR(port);
>>> +    chn = evtchn_from_port(d, port);
>>> +
>>> +    evtchn_write_lock(chn);
>>> +
>>> +    chn->state = ECS_UNBOUND;
>>> +    chn->u.unbound.remote_domid = remote_dom;
>>> +    evtchn_port_init(d, chn);
>>> +
>>> +    evtchn_write_unlock(chn);
>>> +
>>> +    return chn;
>>> +}
>>> +
>>> +static int _evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc)
>>>   {
>>>       struct evtchn *chn;
>>>       struct domain *d;
>>
>> Instead of introducing a clone of this function (with, btw, still
>> insufficient locking), did you consider simply using the existing
>> evtchn_alloc_unbound() as-is, i.e. with the caller passing
>> evtchn_alloc_unbound_t *?
> 
> Yes, we tried that first. Unfortunately the (dummy) XSM check cannot
> work. This is how we would want to call the function:
> 
> 
>      alloc.dom = d->domain_id;
>      alloc.remote_dom = hardware_domain->domain_id;
>      rc = evtchn_alloc_unbound(&alloc);
> 
> 
> This is the implementation of the XSM check:
> 
> static XSM_INLINE int xsm_evtchn_unbound(
>      XSM_DEFAULT_ARG struct domain *d, struct evtchn *chn, domid_t id2)
> {
>      XSM_ASSERT_ACTION(XSM_TARGET);
>      return xsm_default_action(action, current->domain, d);
> }
> 
> 
> Note the usage of current->domain. If you have any suggestions on how to
> fix it please let me know.

If I am not mistaken, current should still point to a domain (in this 
case idle).

So one alternative would be to ignore XSM if current->domain == idle and 
the system is booting (this could be part of xsm_default_action())

Another alternative would be to switch current to another domain. 'dom0' 
wouldn't be a solution because it doesn't exist for "true" dom0less. So 
a possibility would be to use dom_xen or create a fake build domain to 
be used for XSM check during boot.

Cheers,

-- 
Julien Grall

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

[Stefano Stabellini]

On Sun, 23 Jan 2022, Julien Grall wrote:
> On 13/01/2022 14:15, Bertrand Marquis wrote:
> > Hi Stefano,
> > 
> > + Penny in CC for the question.
> > 
> > > On 13 Jan 2022, at 00:58, Stefano Stabellini <sstabellini@kernel.org>
> > > wrote:
> > > 
> > > From: Luca Miccio <lucmiccio@gmail.com>
> > > 
> > > If "xen,enhanced" is enabled, then add to dom0less domains:
> > > 
> > > - the hypervisor node in device tree
> > > - the xenstore event channel
> > > 
> > > The xenstore event channel is also used for the first notification to
> > > let the guest know that xenstore has become available.
> > > 
> > > Signed-off-by: Luca Miccio <lucmiccio@gmail.com>
> > > Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
> > > CC: Julien Grall <julien@xen.org>
> > > CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
> > > CC: Bertrand Marquis <bertrand.marquis@arm.com>
> > 
> > Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
> > 
> > Just one question: GUEST_GNTTAB_BASE is fixed but could it be a problem for
> > a direct map guest in the future ?
> It will be an issue. I think we can re-use the same method as we do in dom0
> (see find_gnttab_region()).

Good idea. I prototyped it and it works fine.  I am not going to add the
patch to this series because it needs Penny's but I can easily provide a
patch to her for it.

Re: [XEN PATCH v2 2/5] xen: export get_free_port

[Stefano Stabellini]

On Tue, 25 Jan 2022, Julien Grall wrote:
> On 25/01/2022 22:49, Stefano Stabellini wrote:
> > On Tue, 25 Jan 2022, Jan Beulich wrote:
> > > On 25.01.2022 02:10, Stefano Stabellini wrote:
> > > > On Sun, 23 Jan 2022, Julien Grall wrote:
> > > > > > diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
> > > > > > index da88ad141a..5b0bcaaad4 100644
> > > > > > --- a/xen/common/event_channel.c
> > > > > > +++ b/xen/common/event_channel.c
> > > > > > @@ -232,7 +232,7 @@ int evtchn_allocate_port(struct domain *d,
> > > > > > evtchn_port_t
> > > > > > port)
> > > > > >        return 0;
> > > > > >    }
> > > > > >    -static int get_free_port(struct domain *d)
> > > > > > +int get_free_port(struct domain *d)
> > > > > 
> > > > > I dislike the idea to expose get_free_port() (or whichever name we
> > > > > decide)
> > > > > because this can be easily misused.
> > > > > 
> > > > > In fact looking at your next patch (#3), you are misusing it as it is
> > > > > meant to
> > > > > be called with d->event_lock. I know this doesn't much matter
> > > > > in your situation because this is done at boot with no other domains
> > > > > running
> > > > > (or potentially any event channel allocation). However, I still think
> > > > > we
> > > > > should get the API right.
> > > > > 
> > > > > I am also not entirely happy of open-coding the allocation in
> > > > > domain_build.c.
> > > > > Instead, I would prefer if we provide a new helper to allocate an
> > > > > unbound
> > > > > event channel. This would be similar to your v1 (I still need to
> > > > > review the
> > > > > patch though).
> > > > 
> > > > I am happy to go back to v1 and address feedback on that patch. However,
> > > > I am having difficulties with the implementation. Jan pointed out:
> > > > 
> > > > 
> > > > > > -
> > > > > > -    chn->state = ECS_UNBOUND;
> > > > > 
> > > > > This cannot be pulled ahead of the XSM check (or in general anything
> > > > > potentially resulting in an error), as check_free_port() relies on
> > > > > ->state remaining ECS_FREE until it is known that the calling function
> > > > > can't fail anymore.
> > > > 
> > > > This makes it difficult to reuse _evtchn_alloc_unbound for the
> > > > implementation of evtchn_alloc_unbound. In fact, I couldn't find a way
> > > > to do it.
> > > > 
> > > > Instead, I just create a new public function called
> > > > "evtchn_alloc_unbound" and renamed the existing funtion to
> > > > "_evtchn_alloc_unbound" (this to addresses Jan's feedback that the
> > > > static function should be the one starting with "_"). So the function
> > > > names are inverted compared to v1.
> > > > 
> > > > Please let me know if you have any better suggestions.
> > > > 
> > > > 
> > > > diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
> > > > index da88ad141a..c6b7dd7fbd 100644
> > > > --- a/xen/common/event_channel.c
> > > > +++ b/xen/common/event_channel.c
> > > > @@ -18,6 +18,7 @@
> > > >     #include <xen/init.h>
> > > >   #include <xen/lib.h>
> > > > +#include <xen/err.h>
> > > >   #include <xen/errno.h>
> > > >   #include <xen/sched.h>
> > > >   #include <xen/irq.h>
> > > > @@ -284,7 +285,27 @@ void evtchn_free(struct domain *d, struct evtchn
> > > > *chn)
> > > >       xsm_evtchn_close_post(chn);
> > > >   }
> > > >   -static int evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc)
> > > > +struct evtchn *evtchn_alloc_unbound(struct domain *d, domid_t
> > > > remote_dom)
> > > > +{
> > > > +    struct evtchn *chn;
> > > > +    int port;
> > > > +
> > > > +    if ( (port = get_free_port(d)) < 0 )
> > > > +        return ERR_PTR(port);
> > > > +    chn = evtchn_from_port(d, port);
> > > > +
> > > > +    evtchn_write_lock(chn);
> > > > +
> > > > +    chn->state = ECS_UNBOUND;
> > > > +    chn->u.unbound.remote_domid = remote_dom;
> > > > +    evtchn_port_init(d, chn);
> > > > +
> > > > +    evtchn_write_unlock(chn);
> > > > +
> > > > +    return chn;
> > > > +}
> > > > +
> > > > +static int _evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc)
> > > >   {
> > > >       struct evtchn *chn;
> > > >       struct domain *d;
> > > 
> > > Instead of introducing a clone of this function (with, btw, still
> > > insufficient locking), did you consider simply using the existing
> > > evtchn_alloc_unbound() as-is, i.e. with the caller passing
> > > evtchn_alloc_unbound_t *?
> > 
> > Yes, we tried that first. Unfortunately the (dummy) XSM check cannot
> > work. This is how we would want to call the function:
> > 
> > 
> >      alloc.dom = d->domain_id;
> >      alloc.remote_dom = hardware_domain->domain_id;
> >      rc = evtchn_alloc_unbound(&alloc);
> > 
> > 
> > This is the implementation of the XSM check:
> > 
> > static XSM_INLINE int xsm_evtchn_unbound(
> >      XSM_DEFAULT_ARG struct domain *d, struct evtchn *chn, domid_t id2)
> > {
> >      XSM_ASSERT_ACTION(XSM_TARGET);
> >      return xsm_default_action(action, current->domain, d);
> > }
> > 
> > 
> > Note the usage of current->domain. If you have any suggestions on how to
> > fix it please let me know.
> 
> If I am not mistaken, current should still point to a domain (in this case
> idle).
> 
> So one alternative would be to ignore XSM if current->domain == idle and the
> system is booting (this could be part of xsm_default_action())
> 
> Another alternative would be to switch current to another domain. 'dom0'
> wouldn't be a solution because it doesn't exist for "true" dom0less. So a
> possibility would be to use dom_xen or create a fake build domain to be used
> for XSM check during boot.


Great suggestions! I went with the first suggestion above and confirmed
it solved the problem! With the appended patch I can just call the
current implementation of evtchn_alloc_unbound (made non-static) from
domain_build.c without any issues.

Are you guys OK with something like this?

diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h
index b024119896..99c63ea8c5 100644
--- a/xen/include/xsm/dummy.h
+++ b/xen/include/xsm/dummy.h
@@ -92,7 +92,9 @@ static always_inline int xsm_default_action(
             return 0;
         /* fall through */
     case XSM_PRIV:
-        if ( is_control_domain(src) )
+        if ( is_control_domain(src) ||
+             src->domain_id == DOMID_IDLE ||
+             src->domain_id == DOMID_XEN )
             return 0;
         return -EPERM;
     default:

Re: [XEN PATCH v2 2/5] xen: export get_free_port

[Jan Beulich]

On 25.01.2022 23:49, Stefano Stabellini wrote:
> On Tue, 25 Jan 2022, Jan Beulich wrote:
>> On 25.01.2022 02:10, Stefano Stabellini wrote:
>>> @@ -284,7 +285,27 @@ void evtchn_free(struct domain *d, struct evtchn *chn)
>>>      xsm_evtchn_close_post(chn);
>>>  }
>>>  
>>> -static int evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc)
>>> +struct evtchn *evtchn_alloc_unbound(struct domain *d, domid_t remote_dom)
>>> +{
>>> +    struct evtchn *chn;
>>> +    int port;
>>> +
>>> +    if ( (port = get_free_port(d)) < 0 )
>>> +        return ERR_PTR(port);
>>> +    chn = evtchn_from_port(d, port);
>>> +
>>> +    evtchn_write_lock(chn);
>>> +
>>> +    chn->state = ECS_UNBOUND;
>>> +    chn->u.unbound.remote_domid = remote_dom;
>>> +    evtchn_port_init(d, chn);
>>> +
>>> +    evtchn_write_unlock(chn);
>>> +
>>> +    return chn;
>>> +}
>>> +
>>> +static int _evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc)
>>>  {
>>>      struct evtchn *chn;
>>>      struct domain *d;
>>
>> Instead of introducing a clone of this function (with, btw, still
>> insufficient locking), did you consider simply using the existing
>> evtchn_alloc_unbound() as-is, i.e. with the caller passing
>> evtchn_alloc_unbound_t *?
> 
> Yes, we tried that first. Unfortunately the (dummy) XSM check cannot
> work. This is how we would want to call the function:
> 
> 
>     alloc.dom = d->domain_id;
>     alloc.remote_dom = hardware_domain->domain_id;
>     rc = evtchn_alloc_unbound(&alloc);
> 
> 
> This is the implementation of the XSM check:
> 
> static XSM_INLINE int xsm_evtchn_unbound(
>     XSM_DEFAULT_ARG struct domain *d, struct evtchn *chn, domid_t id2)
> {
>     XSM_ASSERT_ACTION(XSM_TARGET);
>     return xsm_default_action(action, current->domain, d);
> }
> 
> 
> Note the usage of current->domain. If you have any suggestions on how to
> fix it please let me know.

As an alternative to Julien's suggestion the function could also simply
be given a new boolean parameter indicating whether to bypass the XSM
check. That would be more explicit than deriving from system state.

Jan

Re: [XEN PATCH v2 2/5] xen: export get_free_port

[Jan Beulich]

On 26.01.2022 02:03, Stefano Stabellini wrote:
> Are you guys OK with something like this?

With proper proof that this isn't going to regress anything else, maybe.
But ...

> --- a/xen/include/xsm/dummy.h
> +++ b/xen/include/xsm/dummy.h
> @@ -92,7 +92,9 @@ static always_inline int xsm_default_action(
>              return 0;
>          /* fall through */
>      case XSM_PRIV:
> -        if ( is_control_domain(src) )
> +        if ( is_control_domain(src) ||
> +             src->domain_id == DOMID_IDLE ||
> +             src->domain_id == DOMID_XEN )
>              return 0;

... my first question would be under what circumstances you might observe
DOMID_XEN here and hence why this check is there.

Jan

Re: [XEN PATCH v2 2/5] xen: export get_free_port

[Julien Grall]

Hi,

On 26/01/2022 07:30, Jan Beulich wrote:
> On 26.01.2022 02:03, Stefano Stabellini wrote:
>> Are you guys OK with something like this?
> 
> With proper proof that this isn't going to regress anything else, maybe.

That's why I sugested to also gate with system_state < SYS_STATE_boot so 
we reduce the potential regression (the use of hypercall should be 
limited at boot).

FWIW, there was a thread [1] to discuss the same issue as Stefano is 
facing (but in the context of Live-Update).

> But ...
> 
>> --- a/xen/include/xsm/dummy.h
>> +++ b/xen/include/xsm/dummy.h
>> @@ -92,7 +92,9 @@ static always_inline int xsm_default_action(
>>               return 0;
>>           /* fall through */
>>       case XSM_PRIV:
>> -        if ( is_control_domain(src) )
>> +        if ( is_control_domain(src) ||
>> +             src->domain_id == DOMID_IDLE ||
>> +             src->domain_id == DOMID_XEN )
>>               return 0;
> 
> ... my first question would be under what circumstances you might observe
> DOMID_XEN here and hence why this check is there.
> 
> Jan >

[1] 
https://lore.kernel.org/xen-devel/bfd645cf42ef7786183be15c222ad04beed362c0.camel@xen.org/

-- 
Julien Grall

Re: [XEN PATCH v2 2/5] xen: export get_free_port

[Stefano Stabellini]

On Wed, 26 Jan 2022, Julien Grall wrote:
> On 26/01/2022 07:30, Jan Beulich wrote:
> > On 26.01.2022 02:03, Stefano Stabellini wrote:
> > > Are you guys OK with something like this?
> > 
> > With proper proof that this isn't going to regress anything else, maybe.
> 
> That's why I sugested to also gate with system_state < SYS_STATE_boot so we
> reduce the potential regression (the use of hypercall should be limited at
> boot).
> 
> FWIW, there was a thread [1] to discuss the same issue as Stefano is facing
> (but in the context of Live-Update).
> 
> > But ...
> > 
> > > --- a/xen/include/xsm/dummy.h
> > > +++ b/xen/include/xsm/dummy.h
> > > @@ -92,7 +92,9 @@ static always_inline int xsm_default_action(
> > >               return 0;
> > >           /* fall through */
> > >       case XSM_PRIV:
> > > -        if ( is_control_domain(src) )
> > > +        if ( is_control_domain(src) ||
> > > +             src->domain_id == DOMID_IDLE ||
> > > +             src->domain_id == DOMID_XEN )
> > >               return 0;
> > 
> > ... my first question would be under what circumstances you might observe
> > DOMID_XEN here and hence why this check is there.

For simplicity I'll answer all the points here.

I added both DOMID_IDLE and DOMID_XEN because I thought it "made sense",
but there is no need for DOMID_XEN. We only need DOMID_IDLE. Also adding
a system_state <= SYS_STATE_boot is fine (but it needs to be <= instead
of <). The patch appended below works without issues.

Alternatively, I am also quite happy with Jan's suggestion of passing an
extra parameter to evtchn_alloc_unbound, it could be:

int evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc, bool disable_xsm);

So that XSM is enabled by default.

Adding the bool parameter to evtchn_alloc_unbound has the advantage of
having only a very minor impact. But either option is totally fine by
me, just let me know your preference.



diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h
index b024119896..01996bd9d8 100644
--- a/xen/include/xsm/dummy.h
+++ b/xen/include/xsm/dummy.h
@@ -94,6 +94,8 @@ static always_inline int xsm_default_action(
     case XSM_PRIV:
         if ( is_control_domain(src) )
             return 0;
+        if ( system_state <= SYS_STATE_boot && src->domain_id == DOMID_IDLE )
+            return 0;
         return -EPERM;
     default:
         LINKER_BUG_ON(1);

Re: [XEN PATCH v2 2/5] xen: export get_free_port

[Julien Grall]

Hi Stefano,

On 27/01/2022 01:50, Stefano Stabellini wrote:
> On Wed, 26 Jan 2022, Julien Grall wrote:
>> On 26/01/2022 07:30, Jan Beulich wrote:
>>> On 26.01.2022 02:03, Stefano Stabellini wrote:
>>>> Are you guys OK with something like this?
>>>
>>> With proper proof that this isn't going to regress anything else, maybe.
>>
>> That's why I sugested to also gate with system_state < SYS_STATE_boot so we
>> reduce the potential regression (the use of hypercall should be limited at
>> boot).
>>
>> FWIW, there was a thread [1] to discuss the same issue as Stefano is facing
>> (but in the context of Live-Update).
>>
>>> But ...
>>>
>>>> --- a/xen/include/xsm/dummy.h
>>>> +++ b/xen/include/xsm/dummy.h
>>>> @@ -92,7 +92,9 @@ static always_inline int xsm_default_action(
>>>>                return 0;
>>>>            /* fall through */
>>>>        case XSM_PRIV:
>>>> -        if ( is_control_domain(src) )
>>>> +        if ( is_control_domain(src) ||
>>>> +             src->domain_id == DOMID_IDLE ||
>>>> +             src->domain_id == DOMID_XEN )
>>>>                return 0;
>>>
>>> ... my first question would be under what circumstances you might observe
>>> DOMID_XEN here and hence why this check is there.
> 
> For simplicity I'll answer all the points here.
> 
> I added both DOMID_IDLE and DOMID_XEN because I thought it "made sense",
> but there is no need for DOMID_XEN. We only need DOMID_IDLE. Also adding
> a system_state <= SYS_STATE_boot is fine (but it needs to be <= instead
> of <). The patch appended below works without issues.
> 
> Alternatively, I am also quite happy with Jan's suggestion of passing an
> extra parameter to evtchn_alloc_unbound, it could be:
> 
> int evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc, bool disable_xsm);
> 
> So that XSM is enabled by default.
> 
> Adding the bool parameter to evtchn_alloc_unbound has the advantage of
> having only a very minor impact.

We will likely need similar approach for other hypercalls in the future 
if we need to call them from Xen context (e.g. when restoring domain 
state during Live-Update).

So my preference would be to directly go with modifying the 
xsm_default_action().

> But either option is totally fine by
> me, just let me know your preference.
> 
> 
> 
> diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h
> index b024119896..01996bd9d8 100644
> --- a/xen/include/xsm/dummy.h
> +++ b/xen/include/xsm/dummy.h
> @@ -94,6 +94,8 @@ static always_inline int xsm_default_action(
>       case XSM_PRIV:
>           if ( is_control_domain(src) )
>               return 0;
> +        if ( system_state <= SYS_STATE_boot && src->domain_id == DOMID_IDLE )

I would surround this with unlikely and possibly prevent speculation (Jan?).

Cheers,

-- 
Julien Grall

Re: [XEN PATCH v2 2/5] xen: export get_free_port

[Jan Beulich]

On 27.01.2022 10:51, Julien Grall wrote:
> On 27/01/2022 01:50, Stefano Stabellini wrote:
>> On Wed, 26 Jan 2022, Julien Grall wrote:
>>> On 26/01/2022 07:30, Jan Beulich wrote:
>>>> On 26.01.2022 02:03, Stefano Stabellini wrote:
>>>>> Are you guys OK with something like this?
>>>>
>>>> With proper proof that this isn't going to regress anything else, maybe.
>>>
>>> That's why I sugested to also gate with system_state < SYS_STATE_boot so we
>>> reduce the potential regression (the use of hypercall should be limited at
>>> boot).
>>>
>>> FWIW, there was a thread [1] to discuss the same issue as Stefano is facing
>>> (but in the context of Live-Update).
>>>
>>>> But ...
>>>>
>>>>> --- a/xen/include/xsm/dummy.h
>>>>> +++ b/xen/include/xsm/dummy.h
>>>>> @@ -92,7 +92,9 @@ static always_inline int xsm_default_action(
>>>>>                return 0;
>>>>>            /* fall through */
>>>>>        case XSM_PRIV:
>>>>> -        if ( is_control_domain(src) )
>>>>> +        if ( is_control_domain(src) ||
>>>>> +             src->domain_id == DOMID_IDLE ||
>>>>> +             src->domain_id == DOMID_XEN )
>>>>>                return 0;
>>>>
>>>> ... my first question would be under what circumstances you might observe
>>>> DOMID_XEN here and hence why this check is there.
>>
>> For simplicity I'll answer all the points here.
>>
>> I added both DOMID_IDLE and DOMID_XEN because I thought it "made sense",
>> but there is no need for DOMID_XEN. We only need DOMID_IDLE. Also adding
>> a system_state <= SYS_STATE_boot is fine (but it needs to be <= instead
>> of <). The patch appended below works without issues.
>>
>> Alternatively, I am also quite happy with Jan's suggestion of passing an
>> extra parameter to evtchn_alloc_unbound, it could be:
>>
>> int evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc, bool disable_xsm);
>>
>> So that XSM is enabled by default.
>>
>> Adding the bool parameter to evtchn_alloc_unbound has the advantage of
>> having only a very minor impact.
> 
> We will likely need similar approach for other hypercalls in the future 
> if we need to call them from Xen context (e.g. when restoring domain 
> state during Live-Update).
> 
> So my preference would be to directly go with modifying the 
> xsm_default_action().

How would this help when a real XSM policy is in use? Already in SILO
mode I think you wouldn't get past the check, as the idle domain
doesn't satisfy silo_mode_dom_check()'s use of is_control_domain().
Actually I'm not even sure you'd get that far - I was under the
impression that the domain at other side of the channel may not even
be around at that time, in which case silo_evtchn_unbound() would
return -ESRCH.

Additionally relaxing things at a central place like this one comes
with the risk of relaxing too much. As said in reply to Stefano - if
this is to be done, proof will need to be provided that this doesn't
and won't permit operations which aren't supposed to be permitted. I
for one would much prefer relaxation on a case-by-case basis. That
said I'm afraid it hasn't become clear to me why the XSM check needs
bypassing here in the first place, and why this is acceptable from a
security standpoint.

>> --- a/xen/include/xsm/dummy.h
>> +++ b/xen/include/xsm/dummy.h
>> @@ -94,6 +94,8 @@ static always_inline int xsm_default_action(
>>       case XSM_PRIV:
>>           if ( is_control_domain(src) )
>>               return 0;
>> +        if ( system_state <= SYS_STATE_boot && src->domain_id == DOMID_IDLE )
> 
> I would surround this with unlikely and possibly prevent speculation (Jan?).

Unlikely - perhaps yes. Preventing speculation in principle also
yes, but not at the expense of adding a 2nd LFENCE (besides the one
in is_control_domain()). Yet open-coding is_control_domain() wouldn't
be very nice either. But as per above I hope anyway we're not going
to need to find a good solution here.

Jan

Re: [XEN PATCH v2 2/5] xen: export get_free_port

[Julien Grall]

Hi,

On 27/01/2022 12:07, Jan Beulich wrote:
> On 27.01.2022 10:51, Julien Grall wrote:
>> On 27/01/2022 01:50, Stefano Stabellini wrote:
>>> On Wed, 26 Jan 2022, Julien Grall wrote:
>>>> On 26/01/2022 07:30, Jan Beulich wrote:
>>>>> On 26.01.2022 02:03, Stefano Stabellini wrote:
>>>>>> Are you guys OK with something like this?
>>>>>
>>>>> With proper proof that this isn't going to regress anything else, maybe.
>>>>
>>>> That's why I sugested to also gate with system_state < SYS_STATE_boot so we
>>>> reduce the potential regression (the use of hypercall should be limited at
>>>> boot).
>>>>
>>>> FWIW, there was a thread [1] to discuss the same issue as Stefano is facing
>>>> (but in the context of Live-Update).
>>>>
>>>>> But ...
>>>>>
>>>>>> --- a/xen/include/xsm/dummy.h
>>>>>> +++ b/xen/include/xsm/dummy.h
>>>>>> @@ -92,7 +92,9 @@ static always_inline int xsm_default_action(
>>>>>>                 return 0;
>>>>>>             /* fall through */
>>>>>>         case XSM_PRIV:
>>>>>> -        if ( is_control_domain(src) )
>>>>>> +        if ( is_control_domain(src) ||
>>>>>> +             src->domain_id == DOMID_IDLE ||
>>>>>> +             src->domain_id == DOMID_XEN )
>>>>>>                 return 0;
>>>>>
>>>>> ... my first question would be under what circumstances you might observe
>>>>> DOMID_XEN here and hence why this check is there.
>>>
>>> For simplicity I'll answer all the points here.
>>>
>>> I added both DOMID_IDLE and DOMID_XEN because I thought it "made sense",
>>> but there is no need for DOMID_XEN. We only need DOMID_IDLE. Also adding
>>> a system_state <= SYS_STATE_boot is fine (but it needs to be <= instead
>>> of <). The patch appended below works without issues.
>>>
>>> Alternatively, I am also quite happy with Jan's suggestion of passing an
>>> extra parameter to evtchn_alloc_unbound, it could be:
>>>
>>> int evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc, bool disable_xsm);
>>>
>>> So that XSM is enabled by default.
>>>
>>> Adding the bool parameter to evtchn_alloc_unbound has the advantage of
>>> having only a very minor impact.
>>
>> We will likely need similar approach for other hypercalls in the future
>> if we need to call them from Xen context (e.g. when restoring domain
>> state during Live-Update).
>>
>> So my preference would be to directly go with modifying the
>> xsm_default_action().
> 
> How would this help when a real XSM policy is in use? Already in SILO
> mode I think you wouldn't get past the check, as the idle domain
> doesn't satisfy silo_mode_dom_check()'s use of is_control_domain().
> Actually I'm not even sure you'd get that far - I was under the
> impression that the domain at other side of the channel may not even
> be around at that time, in which case silo_evtchn_unbound() would
> return -ESRCH.

This would not help for real XSM policy. We would either need to bypass 
XSM completely or decide what to do depending on the mode (e.g. SILO, 
FLASK...).

> 
> Additionally relaxing things at a central place like this one comes
> with the risk of relaxing too much. As said in reply to Stefano - if
> this is to be done, proof will need to be provided that this doesn't
> and won't permit operations which aren't supposed to be permitted. I
> for one would much prefer relaxation on a case-by-case basis.

The problem is you will end up to modify a lot of code. This will be 
quite tedious when the policy is likely going to be the same (e.g. if we 
are booting, then allow to use the hypercall).

So I think it makes more sense to do the modification at central place. 
That said, this is not strictly necessary for what Stefano needs. So I 
am OK if we go with local hack and deferred the debate to when a wider 
solution is needed.

Cheers,

-- 
Julien Grall

Re: [XEN PATCH v2 2/5] xen: export get_free_port

[Stefano Stabellini]

On Thu, 27 Jan 2022, Julien Grall wrote:
> On 27/01/2022 12:07, Jan Beulich wrote:
> > On 27.01.2022 10:51, Julien Grall wrote:
> > > On 27/01/2022 01:50, Stefano Stabellini wrote:
> > > > On Wed, 26 Jan 2022, Julien Grall wrote:
> > > > > On 26/01/2022 07:30, Jan Beulich wrote:
> > > > > > On 26.01.2022 02:03, Stefano Stabellini wrote:
> > > > > > > Are you guys OK with something like this?
> > > > > > 
> > > > > > With proper proof that this isn't going to regress anything else,
> > > > > > maybe.
> > > > > 
> > > > > That's why I sugested to also gate with system_state < SYS_STATE_boot
> > > > > so we
> > > > > reduce the potential regression (the use of hypercall should be
> > > > > limited at
> > > > > boot).
> > > > > 
> > > > > FWIW, there was a thread [1] to discuss the same issue as Stefano is
> > > > > facing
> > > > > (but in the context of Live-Update).
> > > > > 
> > > > > > But ...
> > > > > > 
> > > > > > > --- a/xen/include/xsm/dummy.h
> > > > > > > +++ b/xen/include/xsm/dummy.h
> > > > > > > @@ -92,7 +92,9 @@ static always_inline int xsm_default_action(
> > > > > > >                 return 0;
> > > > > > >             /* fall through */
> > > > > > >         case XSM_PRIV:
> > > > > > > -        if ( is_control_domain(src) )
> > > > > > > +        if ( is_control_domain(src) ||
> > > > > > > +             src->domain_id == DOMID_IDLE ||
> > > > > > > +             src->domain_id == DOMID_XEN )
> > > > > > >                 return 0;
> > > > > > 
> > > > > > ... my first question would be under what circumstances you might
> > > > > > observe
> > > > > > DOMID_XEN here and hence why this check is there.
> > > > 
> > > > For simplicity I'll answer all the points here.
> > > > 
> > > > I added both DOMID_IDLE and DOMID_XEN because I thought it "made sense",
> > > > but there is no need for DOMID_XEN. We only need DOMID_IDLE. Also adding
> > > > a system_state <= SYS_STATE_boot is fine (but it needs to be <= instead
> > > > of <). The patch appended below works without issues.
> > > > 
> > > > Alternatively, I am also quite happy with Jan's suggestion of passing an
> > > > extra parameter to evtchn_alloc_unbound, it could be:
> > > > 
> > > > int evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc, bool
> > > > disable_xsm);
> > > > 
> > > > So that XSM is enabled by default.
> > > > 
> > > > Adding the bool parameter to evtchn_alloc_unbound has the advantage of
> > > > having only a very minor impact.
> > > 
> > > We will likely need similar approach for other hypercalls in the future
> > > if we need to call them from Xen context (e.g. when restoring domain
> > > state during Live-Update).
> > > 
> > > So my preference would be to directly go with modifying the
> > > xsm_default_action().
> > 
> > How would this help when a real XSM policy is in use? Already in SILO
> > mode I think you wouldn't get past the check, as the idle domain
> > doesn't satisfy silo_mode_dom_check()'s use of is_control_domain().
> > Actually I'm not even sure you'd get that far - I was under the
> > impression that the domain at other side of the channel may not even
> > be around at that time, in which case silo_evtchn_unbound() would
> > return -ESRCH.
> 
> This would not help for real XSM policy. We would either need to bypass XSM
> completely or decide what to do depending on the mode (e.g. SILO, FLASK...).
> 
> > 
> > Additionally relaxing things at a central place like this one comes
> > with the risk of relaxing too much. As said in reply to Stefano - if
> > this is to be done, proof will need to be provided that this doesn't
> > and won't permit operations which aren't supposed to be permitted. I
> > for one would much prefer relaxation on a case-by-case basis.
> 
> The problem is you will end up to modify a lot of code. This will be quite
> tedious when the policy is likely going to be the same (e.g. if we are
> booting, then allow to use the hypercall).
> 
> So I think it makes more sense to do the modification at central place. That
> said, this is not strictly necessary for what Stefano needs. So I am OK if we
> go with local hack and deferred the debate to when a wider solution is needed.

OK, thank you. I'll go with the following then.


diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
index da88ad141a..dde85444fe 100644
--- a/xen/common/event_channel.c
+++ b/xen/common/event_channel.c
@@ -284,7 +284,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, bool disable_xsm)
 {
     struct evtchn *chn;
     struct domain *d;
@@ -301,9 +301,12 @@ static int evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc)
         ERROR_EXIT_DOM(port, d);
     chn = evtchn_from_port(d, port);
 
-    rc = xsm_evtchn_unbound(XSM_TARGET, d, chn, alloc->remote_dom);
-    if ( rc )
-        goto out;
+    if ( !disable_xsm )
+    {
+        rc = xsm_evtchn_unbound(XSM_TARGET, d, chn, alloc->remote_dom);
+        if ( rc )
+            goto out;
+    }
 
     evtchn_write_lock(chn);
 
@@ -1195,7 +1198,7 @@ long do_event_channel_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
         struct evtchn_alloc_unbound alloc_unbound;
         if ( copy_from_guest(&alloc_unbound, arg, 1) != 0 )
             return -EFAULT;
-        rc = evtchn_alloc_unbound(&alloc_unbound);
+        rc = evtchn_alloc_unbound(&alloc_unbound, false);
         if ( !rc && __copy_to_guest(arg, &alloc_unbound, 1) )
             rc = -EFAULT; /* Cleaning up here would be a mess! */
         break;
diff --git a/xen/include/xen/event.h b/xen/include/xen/event.h
index 21c95e14fd..5ea3ac345b 100644
--- a/xen/include/xen/event.h
+++ b/xen/include/xen/event.h
@@ -68,6 +68,9 @@ int evtchn_close(struct domain *d1, int port1, bool guest);
 /* Free an event channel. */
 void evtchn_free(struct domain *d, struct evtchn *chn);
 
+/* Create a new event channel port */
+int evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc, bool disable_xsm);
+
 /* Allocate a specific event channel port. */
 int evtchn_allocate_port(struct domain *d, unsigned int port);
 

Re: [XEN PATCH v2 2/5] xen: export get_free_port

[Jan Beulich]

On 28.01.2022 02:38, Stefano Stabellini wrote:
> --- a/xen/common/event_channel.c
> +++ b/xen/common/event_channel.c
> @@ -284,7 +284,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, bool disable_xsm)

Nit: I don't think "disable" expresses the behavior. May I suggest "skip" or
"bypass" or some such? Or invert the boolean and name it "check_xsm" or even
simply "xsm"?

Jan