From: Luca Miccio <lucmiccio@gmail.com>

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

Signed-off-by: Luca Miccio <lucmiccio@gmail.com>
Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
CC: Wei Liu <wl@xen.org>
CC: Anthony PERARD <anthony.perard@citrix.com>
CC: Juergen Gross <jgross@suse.com>
---
Changes in v4:
- only alloc xs page (no other magic pages)
- add xenstore permissions
- check all return values
- rename restore_xenstore to create_xenstore
- set target_memkb
- set start_time properly
- close xs transaction on error
- call xc_dom_gnttab_seed instead of xc_dom_gnttab_init
- xs_open instead of xs_daemon_open

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

Changes in v2:
- do not set HVM_PARAM_STORE_EVTCHN twice
- rename restore_xenstore to create_xenstore
- increase maxmem
---
 tools/helpers/Makefile        |  13 ++
 tools/helpers/init-dom0less.c | 323 ++++++++++++++++++++++++++++++++++
 2 files changed, 336 insertions(+)
 create mode 100644 tools/helpers/init-dom0less.c

diff --git a/tools/helpers/Makefile b/tools/helpers/Makefile
index 7f6c422440..8e42997052 100644
--- a/tools/helpers/Makefile
+++ b/tools/helpers/Makefile
@@ -10,6 +10,9 @@ ifeq ($(CONFIG_Linux),y)
 ifeq ($(CONFIG_X86),y)
 PROGS += init-xenstore-domain
 endif
+ifeq ($(CONFIG_ARM),y)
+PROGS += init-dom0less
+endif
 endif
 
 XEN_INIT_DOM0_OBJS = xen-init-dom0.o init-dom-json.o
@@ -26,6 +29,13 @@ $(INIT_XENSTORE_DOMAIN_OBJS): CFLAGS += $(CFLAGS_libxenstore)
 $(INIT_XENSTORE_DOMAIN_OBJS): CFLAGS += $(CFLAGS_libxenlight)
 $(INIT_XENSTORE_DOMAIN_OBJS): CFLAGS += -include $(XEN_ROOT)/tools/config.h
 
+INIT_DOM0LESS_OBJS = init-dom0less.o init-dom-json.o
+$(INIT_DOM0LESS_OBJS): CFLAGS += $(CFLAGS_libxentoollog)
+$(INIT_DOM0LESS_OBJS): CFLAGS += $(CFLAGS_libxenstore)
+$(INIT_DOM0LESS_OBJS): CFLAGS += $(CFLAGS_libxenlight)
+$(INIT_DOM0LESS_OBJS): CFLAGS += $(CFLAGS_libxenctrl)
+$(INIT_DOM0LESS_OBJS): CFLAGS += $(CFLAGS_libxenevtchn)
+
 .PHONY: all
 all: $(PROGS)
 
@@ -35,6 +45,9 @@ xen-init-dom0: $(XEN_INIT_DOM0_OBJS)
 init-xenstore-domain: $(INIT_XENSTORE_DOMAIN_OBJS)
 	$(CC) $(LDFLAGS) -o $@ $(INIT_XENSTORE_DOMAIN_OBJS) $(LDLIBS_libxentoollog) $(LDLIBS_libxenstore) $(LDLIBS_libxenctrl) $(LDLIBS_libxenguest) $(LDLIBS_libxenlight) $(APPEND_LDFLAGS)
 
+init-dom0less: $(INIT_DOM0LESS_OBJS)
+	$(CC) $(LDFLAGS) -o $@ $(INIT_DOM0LESS_OBJS) $(LDLIBS_libxenctrl) $(LDLIBS_libxenevtchn) $(LDLIBS_libxentoollog) $(LDLIBS_libxenstore) $(LDLIBS_libxenlight) $(LDLIBS_libxenguest)  $(APPEND_LDFLAGS)
+
 .PHONY: install
 install: all
 	$(INSTALL_DIR) $(DESTDIR)$(LIBEXEC_BIN)
diff --git a/tools/helpers/init-dom0less.c b/tools/helpers/init-dom0less.c
new file mode 100644
index 0000000000..dc9ccee868
--- /dev/null
+++ b/tools/helpers/init-dom0less.c
@@ -0,0 +1,323 @@
+#include <stdbool.h>
+#include <syslog.h>
+#include <stdio.h>
+#include <err.h>
+#include <stdlib.h>
+#include <sys/time.h>
+#include <xenstore.h>
+#include <xenctrl.h>
+#include <xenguest.h>
+#include <libxl.h>
+#include <xenevtchn.h>
+
+#include "init-dom-json.h"
+
+#define XENSTORE_PFN_OFFSET 1
+#define STR_MAX_LENGTH 64
+
+static int alloc_xs_page(struct xc_interface_core *xch,
+                         libxl_dominfo *info,
+                         uint64_t *xenstore_pfn)
+{
+    int rc;
+    const xen_pfn_t base = GUEST_MAGIC_BASE >> XC_PAGE_SHIFT;
+    xen_pfn_t p2m = (GUEST_MAGIC_BASE >> XC_PAGE_SHIFT) + XENSTORE_PFN_OFFSET;
+
+    rc = xc_domain_setmaxmem(xch, info->domid,
+                             info->max_memkb + (XC_PAGE_SIZE/1024));
+    if (rc < 0)
+        return rc;
+
+    rc = xc_domain_populate_physmap_exact(xch, info->domid, 1, 0, 0, &p2m);
+    if (rc < 0)
+        return rc;
+
+    *xenstore_pfn = base + XENSTORE_PFN_OFFSET;
+    rc = xc_clear_domain_page(xch, info->domid, *xenstore_pfn);
+    if (rc < 0)
+        return rc;
+
+    return 0;
+}
+
+static bool do_xs_write_dom(struct xs_handle *xsh, xs_transaction_t t,
+                            domid_t domid, char *path, char *val)
+{
+    char full_path[STR_MAX_LENGTH];
+    struct xs_permissions perms[2];
+
+    perms[0].id = domid;
+    perms[0].perms = XS_PERM_NONE;
+    perms[1].id = 0;
+    perms[1].perms = XS_PERM_READ;
+
+    if (snprintf(full_path, STR_MAX_LENGTH,
+                 "/local/domain/%u/%s", domid, path) < 0)
+        return false;
+    if (!xs_write(xsh, t, full_path, val, strlen(val)))
+        return false;
+    return xs_set_permissions(xsh, t, full_path, perms, 2);
+}
+
+static bool do_xs_write_libxl(struct xs_handle *xsh, xs_transaction_t t,
+                              domid_t domid, char *path, char *val)
+{
+    char full_path[STR_MAX_LENGTH];
+
+    if (snprintf(full_path, STR_MAX_LENGTH,
+                 "/libxl/%u/%s", domid, path) < 0)
+        return false;
+    return xs_write(xsh, t, full_path, val, strlen(val));
+}
+
+static bool do_xs_write_vm(struct xs_handle *xsh, xs_transaction_t t,
+                           libxl_uuid uuid, char *path, char *val)
+{
+    char full_path[STR_MAX_LENGTH];
+
+    if (snprintf(full_path, STR_MAX_LENGTH,
+                 "/vm/" LIBXL_UUID_FMT "/%s", LIBXL_UUID_BYTES(uuid), path) < 0)
+        return false;
+    return xs_write(xsh, t, full_path, val, strlen(val));
+}
+
+/*
+ * The xenstore nodes are the xenstore nodes libxl writes at domain
+ * creation.
+ *
+ * The list was retrieved by running xenstore-ls on a corresponding
+ * domain started by xl/libxl.
+ */
+static int create_xenstore(struct xs_handle *xsh,
+                           libxl_dominfo *info, libxl_uuid uuid,
+                           evtchn_port_t xenstore_port)
+{
+    domid_t domid;
+    unsigned int i;
+    char uuid_str[STR_MAX_LENGTH];
+    char dom_name_str[STR_MAX_LENGTH];
+    char vm_val_str[STR_MAX_LENGTH];
+    char id_str[STR_MAX_LENGTH];
+    char max_memkb_str[STR_MAX_LENGTH];
+    char target_memkb_str[STR_MAX_LENGTH];
+    char cpu_str[STR_MAX_LENGTH];
+    char xenstore_port_str[STR_MAX_LENGTH];
+    char ring_ref_str[STR_MAX_LENGTH];
+    xs_transaction_t t;
+    struct timeval start_time;
+    char start_time_str[STR_MAX_LENGTH];
+    int rc;
+
+    if (gettimeofday(&start_time, NULL) < 0)
+        return -errno;
+    rc = snprintf(start_time_str, STR_MAX_LENGTH, "%jd.%02d",
+            (intmax_t)start_time.tv_sec, (int)start_time.tv_usec / 10000);
+    if (rc < 0)
+        return rc;
+
+    domid = info->domid;
+    rc = snprintf(id_str, STR_MAX_LENGTH, "%u", domid);
+    if (rc < 0)
+        return rc;
+    rc = snprintf(dom_name_str, STR_MAX_LENGTH, "dom0less-%u", domid);
+    if (rc < 0)
+        return rc;
+    rc = snprintf(uuid_str, STR_MAX_LENGTH, LIBXL_UUID_FMT, LIBXL_UUID_BYTES(uuid));
+    if (rc < 0)
+        return rc;
+    rc = snprintf(vm_val_str, STR_MAX_LENGTH,
+                  "vm/" LIBXL_UUID_FMT, LIBXL_UUID_BYTES(uuid));
+    if (rc < 0)
+        return rc;
+    rc = snprintf(max_memkb_str, STR_MAX_LENGTH, "%lu", info->max_memkb);
+    if (rc < 0)
+        return rc;
+    rc = snprintf(target_memkb_str, STR_MAX_LENGTH, "%lu", info->current_memkb);
+    if (rc < 0)
+        return rc;
+    rc = snprintf(ring_ref_str, STR_MAX_LENGTH, "%lld",
+                  (GUEST_MAGIC_BASE >> XC_PAGE_SHIFT) + XENSTORE_PFN_OFFSET);
+    if (rc < 0)
+        return rc;
+    rc = snprintf(xenstore_port_str, STR_MAX_LENGTH, "%u", xenstore_port);
+    if (rc < 0)
+        return rc;
+
+retry_transaction:
+    t = xs_transaction_start(xsh);
+    if (t == XBT_NULL)
+        return -errno;
+
+    rc = -EIO;
+    /* /vm */
+    if (!do_xs_write_vm(xsh, t, uuid, "name", dom_name_str)) goto err;
+    if (!do_xs_write_vm(xsh, t, uuid, "uuid", uuid_str)) goto err;
+    if (!do_xs_write_vm(xsh, t, uuid, "start_time", start_time_str)) goto err;
+
+    /* /domain */
+    if (!do_xs_write_dom(xsh, t, domid, "vm", vm_val_str)) goto err;
+    if (!do_xs_write_dom(xsh, t, domid, "name", dom_name_str)) goto err;
+    if (!do_xs_write_dom(xsh, t, domid, "cpu", "")) goto err;
+    for (i = 0; i < info->vcpu_max_id; i++) {
+        rc = snprintf(cpu_str, STR_MAX_LENGTH, "cpu/%u/availability/", i);
+        if (rc < 0)
+            goto err;
+        rc = -EIO;
+        if (!do_xs_write_dom(xsh, t, domid, cpu_str,
+                             (info->cpupool & (1 << i)) ? "online" : "offline"))
+            goto err;
+    }
+
+    if (!do_xs_write_dom(xsh, t, domid, "memory", "")) goto err;
+    if (!do_xs_write_dom(xsh, t, domid, "memory/static-max", max_memkb_str)) goto err;
+    if (!do_xs_write_dom(xsh, t, domid, "memory/target", target_memkb_str)) goto err;
+    if (!do_xs_write_dom(xsh, t, domid, "memory/videoram", "-1")) goto err;
+
+    if (!do_xs_write_dom(xsh, t, domid, "device", "")) goto err;
+    if (!do_xs_write_dom(xsh, t, domid, "device/suspend", "")) goto err;
+    if (!do_xs_write_dom(xsh, t, domid, "device/suspend/event-channel", "")) goto err;
+
+    if (!do_xs_write_dom(xsh, t, domid, "control", "")) goto err;
+    if (!do_xs_write_dom(xsh, t, domid, "control/shutdown", "")) goto err;
+    if (!do_xs_write_dom(xsh, t, domid, "control/feature-poweroff", "1")) goto err;
+    if (!do_xs_write_dom(xsh, t, domid, "control/feature-reboot", "1")) goto err;
+    if (!do_xs_write_dom(xsh, t, domid, "control/feature-suspend", "")) goto err;
+    if (!do_xs_write_dom(xsh, t, domid, "control/sysrq", "")) goto err;
+    if (!do_xs_write_dom(xsh, t, domid, "control/platform-feature-multiprocessor-suspend", "1")) goto err;
+    if (!do_xs_write_dom(xsh, t, domid, "control/platform-feature-xs_reset_watches", "1")) goto err;
+
+    if (!do_xs_write_dom(xsh, t, domid, "domid", id_str)) goto err;
+    if (!do_xs_write_dom(xsh, t, domid, "data", "")) goto err;
+    if (!do_xs_write_dom(xsh, t, domid, "drivers", "")) goto err;
+    if (!do_xs_write_dom(xsh, t, domid, "feature", "")) goto err;
+    if (!do_xs_write_dom(xsh, t, domid, "attr", "")) goto err;
+
+    if (!do_xs_write_dom(xsh, t, domid, "store/port", xenstore_port_str)) goto err;
+    if (!do_xs_write_dom(xsh, t, domid, "store/ring-ref", ring_ref_str)) goto err;
+
+    if (!do_xs_write_libxl(xsh, t, domid, "type", "pvh")) goto err;
+    if (!do_xs_write_libxl(xsh, t, domid, "dm-version", "qemu_xen")) goto err;
+
+    if (!xs_transaction_end(xsh, t, false)) {
+        if (errno == EAGAIN)
+            goto retry_transaction;
+        else
+            return -errno;
+    }
+
+    return 0;
+
+err:
+    xs_transaction_end(xsh, t, true);
+    return rc;
+}
+
+static int init_domain(struct xs_handle *xsh, libxl_dominfo *info)
+{
+    struct xc_interface_core *xch;
+    libxl_uuid uuid;
+    uint64_t xenstore_evtchn, xenstore_pfn;
+    int rc;
+
+    printf("Init dom0less domain: %u\n", info->domid);
+    xch = xc_interface_open(0, 0, 0);
+
+    rc = xc_hvm_param_get(xch, info->domid, HVM_PARAM_STORE_EVTCHN,
+                          &xenstore_evtchn);
+    if (rc != 0) {
+        printf("Failed to get HVM_PARAM_STORE_EVTCHN\n");
+        return 1;
+    }
+
+    /* Alloc xenstore page */
+    if (alloc_xs_page(xch, info, &xenstore_pfn) != 0) {
+        printf("Error on alloc magic pages\n");
+        return 1;
+    }
+
+    rc = xc_dom_gnttab_seed(xch, info->domid, true,
+                            (xen_pfn_t)-1, xenstore_pfn, 0, 0);
+    if (rc)
+        err(1, "xc_dom_gnttab_seed");
+
+    libxl_uuid_generate(&uuid);
+    xc_domain_sethandle(xch, info->domid, libxl_uuid_bytearray(&uuid));
+
+    rc = gen_stub_json_config(info->domid, &uuid);
+    if (rc)
+        err(1, "gen_stub_json_config");
+
+    /* Now everything is ready: set HVM_PARAM_STORE_PFN */
+    rc = xc_hvm_param_set(xch, info->domid, HVM_PARAM_STORE_PFN,
+                          xenstore_pfn);
+    if (rc < 0)
+        return rc;
+
+    rc = create_xenstore(xsh, info, uuid, xenstore_evtchn);
+    if (rc)
+        err(1, "writing to xenstore");
+
+    rc = xs_introduce_domain(xsh, info->domid,
+            (GUEST_MAGIC_BASE >> XC_PAGE_SHIFT) + XENSTORE_PFN_OFFSET,
+            xenstore_evtchn);
+    if (!rc)
+        err(1, "xs_introduce_domain");
+    return 0;
+}
+
+/* Check if domain has been configured in XS */
+static bool domain_exists(struct xs_handle *xsh, int domid)
+{
+    return xs_is_domain_introduced(xsh, domid);
+}
+
+int main(int argc, char **argv)
+{
+    libxl_dominfo *info = NULL;
+    libxl_ctx *ctx;
+    int nb_vm = 0, rc = 0, i;
+    struct xs_handle *xsh = NULL;
+
+    /* TODO reuse libxl xsh connection */
+    xsh = xs_open(0);
+    if (xsh == NULL) {
+        fprintf(stderr, "Could not contact XenStore");
+        rc = -errno;
+        goto out;
+    }
+
+    rc = libxl_ctx_alloc(&ctx, LIBXL_VERSION, 0, NULL);
+    if (rc) {
+        fprintf(stderr, "cannot init xl context\n");
+        goto out;
+    }
+
+    info = libxl_list_domain(ctx, &nb_vm);
+    if (!info) {
+        fprintf(stderr, "libxl_list_vm failed.\n");
+        rc = -1;
+        goto out;
+    }
+
+    for (i = 0; i < nb_vm; i++) {
+        domid_t domid = info[i].domid;
+
+        /* Don't need to check for Dom0 */
+        if (!domid)
+            continue;
+
+        printf("Checking domid: %u\n", domid);
+        if (!domain_exists(xsh, domid)) {
+            rc = init_domain(xsh, &info[i]);
+            if (rc < 0) {
+                fprintf(stderr, "init_domain failed.\n");
+                goto out;
+            }
+        } else {
+            printf("Domain %u has already been initialized\n", domid);
+        }
+    }
+out:
+    libxl_dominfo_list_free(info, nb_vm);
+    return rc;
+}
-- 
2.25.1

Hi,

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

On 01/04/2022 01:38, Stefano Stabellini wrote:
> +static int init_domain(struct xs_handle *xsh, libxl_dominfo *info)
> +{
> +    struct xc_interface_core *xch;
> +    libxl_uuid uuid;
> +    uint64_t xenstore_evtchn, xenstore_pfn;
> +    int rc;
> +
> +    printf("Init dom0less domain: %u\n", info->domid);
> +    xch = xc_interface_open(0, 0, 0);
> +
> +    rc = xc_hvm_param_get(xch, info->domid, HVM_PARAM_STORE_EVTCHN,
> +                          &xenstore_evtchn);
> +    if (rc != 0) {
> +        printf("Failed to get HVM_PARAM_STORE_EVTCHN\n");
> +        return 1;
> +    }
> +
> +    /* Alloc xenstore page */
> +    if (alloc_xs_page(xch, info, &xenstore_pfn) != 0) {
> +        printf("Error on alloc magic pages\n");
> +        return 1;
> +    }
> +
> +    rc = xc_dom_gnttab_seed(xch, info->domid, true,
> +                            (xen_pfn_t)-1, xenstore_pfn, 0, 0);
> +    if (rc)
> +        err(1, "xc_dom_gnttab_seed");
> +
> +    libxl_uuid_generate(&uuid);
> +    xc_domain_sethandle(xch, info->domid, libxl_uuid_bytearray(&uuid));
> +
> +    rc = gen_stub_json_config(info->domid, &uuid);
> +    if (rc)
> +        err(1, "gen_stub_json_config");
> +
> +    /* Now everything is ready: set HVM_PARAM_STORE_PFN */
> +    rc = xc_hvm_param_set(xch, info->domid, HVM_PARAM_STORE_PFN,
> +                          xenstore_pfn);

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

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

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

Juergen, any thoughts?

Cheers,

-- 
Julien Grall

On 01.04.22 12:21, Julien Grall wrote:
> Hi,
> 
> I have posted some comments in v3 after you sent this version. Please have a look.
> 
> On 01/04/2022 01:38, Stefano Stabellini wrote:
>> +static int init_domain(struct xs_handle *xsh, libxl_dominfo *info)
>> +{
>> +    struct xc_interface_core *xch;
>> +    libxl_uuid uuid;
>> +    uint64_t xenstore_evtchn, xenstore_pfn;
>> +    int rc;
>> +
>> +    printf("Init dom0less domain: %u\n", info->domid);
>> +    xch = xc_interface_open(0, 0, 0);
>> +
>> +    rc = xc_hvm_param_get(xch, info->domid, HVM_PARAM_STORE_EVTCHN,
>> +                          &xenstore_evtchn);
>> +    if (rc != 0) {
>> +        printf("Failed to get HVM_PARAM_STORE_EVTCHN\n");
>> +        return 1;
>> +    }
>> +
>> +    /* Alloc xenstore page */
>> +    if (alloc_xs_page(xch, info, &xenstore_pfn) != 0) {
>> +        printf("Error on alloc magic pages\n");
>> +        return 1;
>> +    }
>> +
>> +    rc = xc_dom_gnttab_seed(xch, info->domid, true,
>> +                            (xen_pfn_t)-1, xenstore_pfn, 0, 0);
>> +    if (rc)
>> +        err(1, "xc_dom_gnttab_seed");
>> +
>> +    libxl_uuid_generate(&uuid);
>> +    xc_domain_sethandle(xch, info->domid, libxl_uuid_bytearray(&uuid));
>> +
>> +    rc = gen_stub_json_config(info->domid, &uuid);
>> +    if (rc)
>> +        err(1, "gen_stub_json_config");
>> +
>> +    /* Now everything is ready: set HVM_PARAM_STORE_PFN */
>> +    rc = xc_hvm_param_set(xch, info->domid, HVM_PARAM_STORE_PFN,
>> +                          xenstore_pfn);
> 
> On patch #1, you told me you didn't want to allocate the page in Xen because it 
> wouldn't be initialized by Xenstored. But this is what we are doing here.

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

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

I think it should work like that:

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

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


Juergen

Hi Juergen,

On 01/04/2022 11:46, Juergen Gross wrote:
> On 01.04.22 12:21, Julien Grall wrote:
>> Hi,
>>
>> I have posted some comments in v3 after you sent this version. Please 
>> have a look.
>>
>> On 01/04/2022 01:38, Stefano Stabellini wrote:
>>> +static int init_domain(struct xs_handle *xsh, libxl_dominfo *info)
>>> +{
>>> +    struct xc_interface_core *xch;
>>> +    libxl_uuid uuid;
>>> +    uint64_t xenstore_evtchn, xenstore_pfn;
>>> +    int rc;
>>> +
>>> +    printf("Init dom0less domain: %u\n", info->domid);
>>> +    xch = xc_interface_open(0, 0, 0);
>>> +
>>> +    rc = xc_hvm_param_get(xch, info->domid, HVM_PARAM_STORE_EVTCHN,
>>> +                          &xenstore_evtchn);
>>> +    if (rc != 0) {
>>> +        printf("Failed to get HVM_PARAM_STORE_EVTCHN\n");
>>> +        return 1;
>>> +    }
>>> +
>>> +    /* Alloc xenstore page */
>>> +    if (alloc_xs_page(xch, info, &xenstore_pfn) != 0) {
>>> +        printf("Error on alloc magic pages\n");
>>> +        return 1;
>>> +    }
>>> +
>>> +    rc = xc_dom_gnttab_seed(xch, info->domid, true,
>>> +                            (xen_pfn_t)-1, xenstore_pfn, 0, 0);
>>> +    if (rc)
>>> +        err(1, "xc_dom_gnttab_seed");
>>> +
>>> +    libxl_uuid_generate(&uuid);
>>> +    xc_domain_sethandle(xch, info->domid, libxl_uuid_bytearray(&uuid));
>>> +
>>> +    rc = gen_stub_json_config(info->domid, &uuid);
>>> +    if (rc)
>>> +        err(1, "gen_stub_json_config");
>>> +
>>> +    /* Now everything is ready: set HVM_PARAM_STORE_PFN */
>>> +    rc = xc_hvm_param_set(xch, info->domid, HVM_PARAM_STORE_PFN,
>>> +                          xenstore_pfn);
>>
>> On patch #1, you told me you didn't want to allocate the page in Xen 
>> because it wouldn't be initialized by Xenstored. But this is what we 
>> are doing here.
> 
> Xenstore (at least the C variant) is only using the fixed grant ref
> GNTTAB_RESERVED_XENSTORE, so it doesn't need the page to be advertised
> to the guest. And the mapping is done only when the domain is being
> introduced to Xenstore.

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

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

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

Cheers,

-- 
Julien Grall

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

Looking for it every second or so would be enough.

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

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

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


Juergen

Hi Juergen,

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

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

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

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

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

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

Cheers,

-- 
Julien Grall

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

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


Juergen

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


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

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

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

Is that right?

Hi Stefano,

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

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

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

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

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

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

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

Cheers,

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

-- 
Julien Grall

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

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

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