[PATCH v3 0/7] xen/evtchn: implement static event channel signaling

[PATCH v3 0/7] xen/evtchn: implement static event channel signaling

[Rahul Singh]

The purpose of this patch series is to add the static event channel signaling
support to Xen on Arm based on design doc [1].

[1] https://lists.xenproject.org/archives/html/xen-devel/2022-05/msg01160.html

Julien Grall (1):
  xen/evtchn: Make sure all buckets below d->valid_evtchns are allocated

Rahul Singh (6):
  xen/evtchn: Add an helper to reserve/allocate a port
  xen/evtchn: restrict the maximum number of evtchn supported for domUs
  xen/evtchn: modify evtchn_alloc_unbound to allocate specified port
  xen/evtchn: modify evtchn_bind_interdomain to support static evtchn
  xen/arm: introduce xen-evtchn dom0less property
  xen/arm: introduce new xen,enhanced property value

 docs/misc/arm/device-tree/booting.txt |  68 +++++++++++-
 xen/arch/arm/domain_build.c           | 148 +++++++++++++++++++++++++-
 xen/arch/arm/include/asm/kernel.h     |   3 +
 xen/arch/arm/include/asm/setup.h      |   1 +
 xen/arch/arm/setup.c                  |   2 +
 xen/common/event_channel.c            | 121 +++++++++++++--------
 xen/include/xen/device_tree.h         |  13 +++
 xen/include/xen/event.h               |   8 +-
 8 files changed, 313 insertions(+), 51 deletions(-)

-- 
2.25.1

[PATCH v3 1/7] xen/evtchn: Make sure all buckets below d->valid_evtchns are allocated

[Rahul Singh]

From: Julien Grall <jgrall@amazon.com>

Since commit 01280dc19cf3 "evtchn: simplify port_is_valid()", the event
channels code assumes that all the buckets below d->valid_evtchns are
always allocated.

This assumption hold in most of the situation because a guest is not
allowed to chose the port. Instead, it will be the first free from port
0.

When static event channel support will be added for dom0less domains
user can request to allocate the evtchn port numbers that are scattered
in nature.

The existing implementation of evtchn_allocate_port() is not able to
deal with such situation and will end up to override bucket or/and leave
some bucket unallocated. The latter will result to a droplet crash if
the event channel belongs to an unallocated bucket.

This can be solved by making sure that all the buckets below
d->valid_evtchns are allocated. There should be no impact for most of
the situation but LM/LU as only one bucket would be allocated. For
LM/LU, we may end up to allocate multiple buckets if ports in use are
sparse.

A potential alternative is to check that the bucket is valid in
is_port_valid(). This should still possible to do it without taking
per-domain lock but will result a couple more of memory access.

Signed-off-by: Julien Grall <jgrall@amazon.com>
Signed-off-by: Rahul Singh <rahul.singh@arm.com>
---
Changes in v3:
 - fix comments in commit msg.
 - modify code related to d->valid_evtchns and {read,write}_atomic()
Changes in v2:
 - new patch in this version to avoid the security issue
---
 xen/common/event_channel.c | 55 ++++++++++++++++++++++++--------------
 1 file changed, 35 insertions(+), 20 deletions(-)

diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
index c2c6f8c151..80b06d9743 100644
--- a/xen/common/event_channel.c
+++ b/xen/common/event_channel.c
@@ -193,6 +193,15 @@ static struct evtchn *alloc_evtchn_bucket(struct domain *d, unsigned int port)
     return NULL;
 }
 
+/*
+ * Allocate a given port and ensure all the buckets up to that ports
+ * have been allocated.
+ *
+ * The last part is important because the rest of the event channel code
+ * relies on all the buckets up to d->valid_evtchns to be valid. However,
+ * event channels may be sparsed when restoring a domain during Guest
+ * Transparent Migration and Live Update.
+ */
 int evtchn_allocate_port(struct domain *d, evtchn_port_t port)
 {
     if ( port > d->max_evtchn_port || port >= max_evtchns(d) )
@@ -207,30 +216,36 @@ int evtchn_allocate_port(struct domain *d, evtchn_port_t port)
     }
     else
     {
-        struct evtchn *chn;
-        struct evtchn **grp;
+        unsigned int alloc_port = read_atomic(&d->valid_evtchns);
 
-        if ( !group_from_port(d, port) )
+        do
         {
-            grp = xzalloc_array(struct evtchn *, BUCKETS_PER_GROUP);
-            if ( !grp )
-                return -ENOMEM;
-            group_from_port(d, port) = grp;
-        }
+            struct evtchn *chn;
+            struct evtchn **grp;
 
-        chn = alloc_evtchn_bucket(d, port);
-        if ( !chn )
-            return -ENOMEM;
-        bucket_from_port(d, port) = chn;
+            if ( !group_from_port(d, alloc_port) )
+            {
+                grp = xzalloc_array(struct evtchn *, BUCKETS_PER_GROUP);
+                if ( !grp )
+                    return -ENOMEM;
+                group_from_port(d, alloc_port) = grp;
+            }
 
-        /*
-         * d->valid_evtchns is used to check whether the bucket can be
-         * accessed without the per-domain lock. Therefore,
-         * d->valid_evtchns should be seen *after* the new bucket has
-         * been setup.
-         */
-        smp_wmb();
-        write_atomic(&d->valid_evtchns, d->valid_evtchns + EVTCHNS_PER_BUCKET);
+            chn = alloc_evtchn_bucket(d, alloc_port);
+            if ( !chn )
+                return -ENOMEM;
+            bucket_from_port(d, alloc_port) = chn;
+
+            /*
+             * d->valid_evtchns is used to check whether the bucket can be
+             * accessed without the per-domain lock. Therefore,
+             * d->valid_evtchns should be seen *after* the new bucket has
+             * been setup.
+             */
+            smp_wmb();
+            alloc_port += EVTCHNS_PER_BUCKET;
+            write_atomic(&d->valid_evtchns, alloc_port);
+        } while ( port >= alloc_port );
     }
 
     write_atomic(&d->active_evtchns, d->active_evtchns + 1);
-- 
2.25.1

[PATCH v3 2/7] xen/evtchn: Add an helper to reserve/allocate a port

[Rahul Singh]

In a follow-up patch we will want to either reserve or allocate a port
for various event channel helpers.

A new wrapper is introduced to either reserve a given port or allocate
a fresh one if zero.

Take the opportunity to replace the open-coded version in
evtchn_bind_virq().

Signed-off-by: Stanislav Kinsburskii <staskins@amazon.com>
Signed-off-by: Julien Grall <jgrall@amazon.com>
Signed-off-by: Rahul Singh <rahul.singh@arm.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
---
Changes in v3:
 - minor comments in commit msg
Changes in v2:
 - new patch in this version
---
 xen/common/event_channel.c | 29 ++++++++++++++++-------------
 1 file changed, 16 insertions(+), 13 deletions(-)

diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
index 80b06d9743..ef4da0781d 100644
--- a/xen/common/event_channel.c
+++ b/xen/common/event_channel.c
@@ -305,6 +305,18 @@ void evtchn_free(struct domain *d, struct evtchn *chn)
     xsm_evtchn_close_post(chn);
 }
 
+static int evtchn_get_port(struct domain *d, evtchn_port_t port)
+{
+    int rc;
+
+    if ( port != 0 )
+        rc = evtchn_allocate_port(d, port);
+    else
+        rc = get_free_port(d);
+
+    return rc ?: port;
+}
+
 int evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc)
 {
     struct evtchn *chn;
@@ -462,19 +474,10 @@ int evtchn_bind_virq(evtchn_bind_virq_t *bind, evtchn_port_t port)
     if ( read_atomic(&v->virq_to_evtchn[virq]) )
         ERROR_EXIT(-EEXIST);
 
-    if ( port != 0 )
-    {
-        if ( (rc = evtchn_allocate_port(d, port)) != 0 )
-            ERROR_EXIT(rc);
-    }
-    else
-    {
-        int alloc_port = get_free_port(d);
-
-        if ( alloc_port < 0 )
-            ERROR_EXIT(alloc_port);
-        port = alloc_port;
-    }
+    port = rc = evtchn_get_port(d, port);
+    if ( rc < 0 )
+        ERROR_EXIT(rc);
+    rc = 0;
 
     chn = evtchn_from_port(d, port);
 
-- 
2.25.1

[PATCH v3 3/7] xen/evtchn: restrict the maximum number of evtchn supported for domUs

[Rahul Singh]

Restrict the maximum number of evtchn supported for domUs to avoid
allocating a large amount of memory in Xen.

Set the default value of max_evtchn_port to 1023. The value of 1023
should be sufficient for domUs guests because on ARM we don't bind
physical interrupts to event channels. The only use of the evtchn port
is inter-domain communications.

Signed-off-by: Rahul Singh <rahul.singh@arm.com>
---
Changes in v3:
 - added in commit msg why we set the max_evtchn_port value to 1023.
 - added the comment in code also why we set the max_evtchn_port to 1023
 - remove the define and set the value to 1023 in code directly.
Changes in v2:
 - new patch in the version
---
 xen/arch/arm/domain_build.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 3fd1186b53..fde133cd94 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -3277,7 +3277,13 @@ void __init create_domUs(void)
         struct xen_domctl_createdomain d_cfg = {
             .arch.gic_version = XEN_DOMCTL_CONFIG_GIC_NATIVE,
             .flags = XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap,
-            .max_evtchn_port = -1,
+            /*
+             * The default of 1023 should be sufficient for domUs guests
+             * because on ARM we don't bind physical interrupts to event
+             * channels. The only use of the evtchn port is inter-domain
+             * communications.
+             */
+            .max_evtchn_port = 1023,
             .max_grant_frames = -1,
             .max_maptrack_frames = -1,
             .grant_opts = XEN_DOMCTL_GRANT_version(opt_gnttab_max_version),
-- 
2.25.1

[PATCH v3 4/7] xen/evtchn: modify evtchn_alloc_unbound to allocate specified port

[Rahul Singh]

Currently evtchn_alloc_unbound() always allocates the next available
port. Static event channel support for dom0less domains requires
allocating a specified port.

Modify the evtchn_alloc_unbound() to accept the port number as an
argument and allocate the specified port if available. If the port
number argument is zero, the next available port will be allocated.

Signed-off-by: Rahul Singh <rahul.singh@arm.com>
Reviewed-by: Julien Grall <jgrall@amazon.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
---
Changes in v3:
 - fix minor comments in commit msg
Changes in v2:
 - fix minor comments
---
 xen/arch/arm/domain_build.c |  2 +-
 xen/common/event_channel.c  | 17 ++++++++++++-----
 xen/include/xen/event.h     |  3 ++-
 3 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index fde133cd94..707e247f6a 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -3171,7 +3171,7 @@ static int __init alloc_xenstore_evtchn(struct domain *d)
 
     alloc.dom = d->domain_id;
     alloc.remote_dom = hardware_domain->domain_id;
-    rc = evtchn_alloc_unbound(&alloc);
+    rc = evtchn_alloc_unbound(&alloc, 0);
     if ( rc )
     {
         printk("Failed allocating event channel for domain\n");
diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
index ef4da0781d..b464f09d2a 100644
--- a/xen/common/event_channel.c
+++ b/xen/common/event_channel.c
@@ -317,11 +317,15 @@ static int evtchn_get_port(struct domain *d, evtchn_port_t port)
     return rc ?: port;
 }
 
-int evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc)
+/*
+ * If port is zero get the next free port and allocate. If port is non-zero
+ * allocate the specified port.
+ */
+int evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc, evtchn_port_t port)
 {
     struct evtchn *chn;
     struct domain *d;
-    int            port, rc;
+    int            rc;
     domid_t        dom = alloc->dom;
 
     d = rcu_lock_domain_by_any_id(dom);
@@ -330,8 +334,11 @@ int evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc)
 
     write_lock(&d->event_lock);
 
-    if ( (port = get_free_port(d)) < 0 )
-        ERROR_EXIT_DOM(port, d);
+    port = rc = evtchn_get_port(d, port);
+    if ( rc < 0 )
+        ERROR_EXIT(rc);
+    rc = 0;
+
     chn = evtchn_from_port(d, port);
 
     rc = xsm_evtchn_unbound(XSM_TARGET, d, chn, alloc->remote_dom);
@@ -1222,7 +1229,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, 0);
         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 f3021fe304..f31963703f 100644
--- a/xen/include/xen/event.h
+++ b/xen/include/xen/event.h
@@ -72,7 +72,8 @@ void evtchn_free(struct domain *d, struct evtchn *chn);
 int evtchn_allocate_port(struct domain *d, unsigned int port);
 
 /* Allocate a new event channel */
-int __must_check evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc);
+int __must_check evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc,
+                                      evtchn_port_t port);
 
 /* Unmask a local event-channel port. */
 int evtchn_unmask(unsigned int port);
-- 
2.25.1

[PATCH v3 5/7] xen/evtchn: modify evtchn_bind_interdomain to support static evtchn

[Rahul Singh]

Static event channel support will be added for dom0less domains. Modify
evtchn_bind_interdomain to support static evtchn.

It is necessary to have access to the evtchn_bind_interdomain function
to do that, so make evtchn_bind_interdomain global and also make it
__must_check.

Currently evtchn_bind_interdomain() always allocates the next available
local port. Static event channel support for dom0less domains requires
allocating a specified port. Modify the evtchn_bind_interdomain to
accept the port number as an argument and allocate the specified port
if available. If the port number argument is zero, the next available
port will be allocated.

Currently evtchn_bind_interdomain() finds the local domain from
"current->domain" pointer. evtchn_bind_interdomain() will be called from
the XEN to create static event channel during domain creation.
"current" pointer is not valid at that time, therefore modify the
evtchn_bind_interdomain() to pass domain as an argument.

Signed-off-by: Rahul Singh <rahul.singh@arm.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Julien Grall <jgrall@amazon.com>
---
Changes in v3:
 - fix minor comments in commit msg
Changes in v2:
 - Merged patches related to evtchn_bind_interdomain in one patch
---
 xen/common/event_channel.c | 20 ++++++++++++++------
 xen/include/xen/event.h    |  5 +++++
 2 files changed, 19 insertions(+), 6 deletions(-)

diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
index b464f09d2a..19832002aa 100644
--- a/xen/common/event_channel.c
+++ b/xen/common/event_channel.c
@@ -381,11 +381,16 @@ static void double_evtchn_unlock(struct evtchn *lchn, struct evtchn *rchn)
     evtchn_write_unlock(rchn);
 }
 
-static int evtchn_bind_interdomain(evtchn_bind_interdomain_t *bind)
+/*
+ * If lport is zero get the next free port and allocate. If port is non-zero
+ * allocate the specified lport.
+ */
+int evtchn_bind_interdomain(evtchn_bind_interdomain_t *bind, struct domain *ld,
+                            evtchn_port_t lport)
 {
     struct evtchn *lchn, *rchn;
-    struct domain *ld = current->domain, *rd;
-    int            lport, rc;
+    struct domain *rd;
+    int            rc;
     evtchn_port_t  rport = bind->remote_port;
     domid_t        rdom = bind->remote_dom;
 
@@ -405,8 +410,11 @@ static int evtchn_bind_interdomain(evtchn_bind_interdomain_t *bind)
         write_lock(&ld->event_lock);
     }
 
-    if ( (lport = get_free_port(ld)) < 0 )
-        ERROR_EXIT(lport);
+    lport = rc = evtchn_get_port(ld, lport);
+    if ( rc < 0 )
+        ERROR_EXIT(rc);
+    rc = 0;
+
     lchn = evtchn_from_port(ld, lport);
 
     rchn = _evtchn_from_port(rd, rport);
@@ -1239,7 +1247,7 @@ long do_event_channel_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
         struct evtchn_bind_interdomain bind_interdomain;
         if ( copy_from_guest(&bind_interdomain, arg, 1) != 0 )
             return -EFAULT;
-        rc = evtchn_bind_interdomain(&bind_interdomain);
+        rc = evtchn_bind_interdomain(&bind_interdomain, current->domain, 0);
         if ( !rc && __copy_to_guest(arg, &bind_interdomain, 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 f31963703f..8eae9984a9 100644
--- a/xen/include/xen/event.h
+++ b/xen/include/xen/event.h
@@ -75,6 +75,11 @@ int evtchn_allocate_port(struct domain *d, unsigned int port);
 int __must_check evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc,
                                       evtchn_port_t port);
 
+/* Bind an event channel port to interdomain */
+int __must_check evtchn_bind_interdomain(evtchn_bind_interdomain_t *bind,
+                                         struct domain *ld,
+                                         evtchn_port_t port);
+
 /* Unmask a local event-channel port. */
 int evtchn_unmask(unsigned int port);
 
-- 
2.25.1

[PATCH v3 6/7] xen/arm: introduce xen-evtchn dom0less property

[Rahul Singh]

Introduce a new sub-node under /chosen node to establish static event
channel communication between domains on dom0less systems.

An event channel will be created beforehand to allow the domains to
send notifications to each other.

Signed-off-by: Rahul Singh <rahul.singh@arm.com>
---
Changes in v3:
 - use device-tree used_by to find the domain id of the evtchn node.
 - add new static_evtchn_create variable in struct dt_device_node to
   hold the information if evtchn is already created.
 - fix minor comments
Changes in v2:
 - no change
---
 docs/misc/arm/device-tree/booting.txt |  64 ++++++++++++-
 xen/arch/arm/domain_build.c           | 128 ++++++++++++++++++++++++++
 xen/arch/arm/include/asm/setup.h      |   1 +
 xen/arch/arm/setup.c                  |   2 +
 xen/include/xen/device_tree.h         |  13 +++
 5 files changed, 207 insertions(+), 1 deletion(-)

diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt
index 98253414b8..edef98e6d1 100644
--- a/docs/misc/arm/device-tree/booting.txt
+++ b/docs/misc/arm/device-tree/booting.txt
@@ -212,7 +212,7 @@ with the following properties:
     enable only selected interfaces.
 
 Under the "xen,domain" compatible node, one or more sub-nodes are present
-for the DomU kernel and ramdisk.
+for the DomU kernel, ramdisk and static event channel.
 
 The kernel sub-node has the following properties:
 
@@ -254,11 +254,44 @@ The ramdisk sub-node has the following properties:
     property because it will be created by the UEFI stub on boot.
     This option is needed only when UEFI boot is used.
 
+The static event channel sub-node has the following properties:
+
+- compatible
+
+    "xen,evtchn"
+
+- xen,evtchn
+
+    The property is tuples of two numbers
+    (local-evtchn link-to-foreign-evtchn) where:
+
+    local-evtchn is an integer value that will be used to allocate local port
+    for a domain to send and receive event notifications to/from the remote
+    domain. Maximum supported value is 2^17 for FIFO ABI and 4096 for 2L ABI.
+    It is recommended to use low event channel IDs.
+
+    link-to-foreign-evtchn is a single phandle to a remote evtchn to which
+    local-evtchn will be connected.
 
 Example
 =======
 
 chosen {
+
+    module@0 {
+        compatible = "multiboot,kernel", "multiboot,module";
+        xen,uefi-binary = "...";
+        bootargs = "...";
+
+    };
+
+    /* one sub-node per local event channel */
+    ec1: evtchn@1 {
+         compatible = "xen,evtchn-v1";
+         /* local-evtchn link-to-foreign-evtchn */
+         xen,evtchn = <0xa &ec2>;
+    };
+
     domU1 {
         compatible = "xen,domain";
         #address-cells = <0x2>;
@@ -277,6 +310,23 @@ chosen {
             compatible = "multiboot,ramdisk", "multiboot,module";
             reg = <0x0 0x4b000000 0xffffff>;
         };
+
+        /* one sub-node per local event channel */
+        ec2: evtchn@2 {
+            compatible = "xen,evtchn-v1";
+            /* local-evtchn link-to-foreign-evtchn */
+            xen,evtchn = <0xa &ec1>;
+        };
+
+        ec3: evtchn@3 {
+            compatible = "xen,evtchn-v1";
+            xen,evtchn = <0xb &ec5>;
+        };
+
+        ec4: evtchn@4 {
+            compatible = "xen,evtchn-v1";
+            xen,evtchn = <0xc &ec6>;
+        };
     };
 
     domU2 {
@@ -296,6 +346,18 @@ chosen {
             compatible = "multiboot,ramdisk", "multiboot,module";
             reg = <0x0 0x4d000000 0xffffff>;
         };
+
+        /* one sub-node per local event channel */
+        ec5: evtchn@5 {
+            compatible = "xen,evtchn-v1";
+            /* local-evtchn link-to-foreign-evtchn */
+            xen,evtchn = <0xb &ec3>;
+        };
+
+        ec6: evtchn@6 {
+            compatible = "xen,evtchn-v1";
+            xen,evtchn = <0xd &ec4>;
+        };
     };
 };
 
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 707e247f6a..4b24261825 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -33,6 +33,8 @@
 #include <xen/grant_table.h>
 #include <xen/serial.h>
 
+#define STATIC_EVTCHN_NODE_SIZE_CELLS 2
+
 static unsigned int __initdata opt_dom0_max_vcpus;
 integer_param("dom0_max_vcpus", opt_dom0_max_vcpus);
 
@@ -3052,6 +3054,131 @@ void __init evtchn_allocate(struct domain *d)
     d->arch.hvm.params[HVM_PARAM_CALLBACK_IRQ] = val;
 }
 
+static int __init get_evtchn_dt_property(const struct dt_device_node *np,
+                                         uint32_t *port, uint32_t *phandle)
+{
+    const __be32 *prop = NULL;
+    uint32_t len;
+
+    prop = dt_get_property(np, "xen,evtchn", &len);
+    if ( !prop )
+    {
+        printk(XENLOG_ERR "xen,evtchn property should not be empty.\n");
+        return -EINVAL;
+    }
+
+    if ( !len || len < dt_cells_to_size(STATIC_EVTCHN_NODE_SIZE_CELLS) )
+    {
+        printk(XENLOG_ERR "xen,evtchn property value is not valid.\n");
+        return -EINVAL;
+    }
+
+    *port = dt_next_cell(1, &prop);
+    *phandle = dt_next_cell(1, &prop);
+
+    return 0;
+}
+
+static int __init alloc_domain_evtchn(struct dt_device_node *node)
+{
+    int rc;
+    uint32_t domU1_port, domU2_port, remote_phandle;
+    struct dt_device_node *remote_node;
+    struct evtchn_alloc_unbound alloc_unbound;
+    struct evtchn_bind_interdomain bind_interdomain;
+    struct domain *d1 = NULL, *d2 = NULL;
+
+    if ( dt_device_static_evtchn_created(node) )
+        return 0;
+
+    rc = get_evtchn_dt_property(node, &domU1_port, &remote_phandle);
+    if ( rc )
+        return rc;
+
+    remote_node = dt_find_node_by_phandle(remote_phandle);
+    if ( !remote_node )
+    {
+        printk(XENLOG_ERR
+                "evtchn: could not find remote evtchn phandle\n");
+        return -EINVAL;
+    }
+
+    rc = get_evtchn_dt_property(remote_node, &domU2_port, &remote_phandle);
+    if ( rc )
+        return rc;
+
+    if ( node->phandle != remote_phandle )
+    {
+        printk(XENLOG_ERR "xen,evtchn property is not setup correctly.\n");
+        return -EINVAL;
+    }
+
+    d1 = get_domain_by_id(dt_get_parent(node)->used_by);
+    d2 = get_domain_by_id(dt_get_parent(remote_node)->used_by);
+
+    if ( !d1 || !d2 )
+    {
+        printk(XENLOG_ERR "evtchn: could not find domains\n" );
+        return -EINVAL;
+    }
+
+    alloc_unbound.dom = d1->domain_id;
+    alloc_unbound.remote_dom = d2->domain_id;
+
+    rc = evtchn_alloc_unbound(&alloc_unbound, domU1_port);
+    if ( rc < 0 )
+    {
+        printk(XENLOG_ERR
+                "evtchn_alloc_unbound() failure (Error %d) \n", rc);
+        return rc;
+    }
+
+    bind_interdomain.remote_dom  = d1->domain_id;
+    bind_interdomain.remote_port = domU1_port;
+
+    rc = evtchn_bind_interdomain(&bind_interdomain, d2, domU2_port);
+    if ( rc < 0 )
+    {
+        printk(XENLOG_ERR
+                "evtchn_bind_interdomain() failure (Error %d) \n", rc);
+        return rc;
+    }
+
+    dt_device_set_static_evtchn_created(node);
+    dt_device_set_static_evtchn_created(remote_node);
+
+    return 0;
+}
+
+void __init process_static_evtchn_node(struct dt_device_node *node)
+{
+    if ( dt_device_is_compatible(node, "xen,evtchn-v1") )
+    {
+        if ( alloc_domain_evtchn(node) != 0 )
+            panic("Could not set up domains evtchn\n");
+    }
+}
+
+void __init alloc_static_evtchn(void)
+{
+    struct dt_device_node *node, *evtchn_node;
+    struct dt_device_node *chosen = dt_find_node_by_path("/chosen");
+
+    BUG_ON(chosen == NULL);
+
+    if ( hardware_domain )
+        dt_device_set_used_by(chosen, hardware_domain->domain_id);
+
+    dt_for_each_child_node(chosen, node)
+    {
+        if ( hardware_domain )
+            process_static_evtchn_node(node);
+
+        dt_for_each_child_node(node, evtchn_node)
+            process_static_evtchn_node(evtchn_node);
+    }
+}
+
 static void __init find_gnttab_region(struct domain *d,
                                       struct kernel_info *kinfo)
 {
@@ -3364,6 +3491,7 @@ void __init create_domUs(void)
             panic("Error creating domain %s\n", dt_node_name(node));
 
         d->is_console = true;
+        dt_device_set_used_by(node, d->domain_id);
 
         if ( construct_domU(d, node) != 0 )
             panic("Could not set up domain %s\n", dt_node_name(node));
diff --git a/xen/arch/arm/include/asm/setup.h b/xen/arch/arm/include/asm/setup.h
index 5815ccf8c5..5ee28b270f 100644
--- a/xen/arch/arm/include/asm/setup.h
+++ b/xen/arch/arm/include/asm/setup.h
@@ -106,6 +106,7 @@ int acpi_make_efi_nodes(void *fdt, struct membank tbl_add[]);
 
 void create_domUs(void);
 void create_dom0(void);
+void alloc_static_evtchn(void);
 
 void discard_initial_modules(void);
 void fw_unreserved_regions(paddr_t s, paddr_t e,
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 6e0398f3f6..cf15d359d2 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -1077,6 +1077,8 @@ void __init start_xen(unsigned long boot_phys_offset,
     if ( acpi_disabled )
         create_domUs();
 
+    alloc_static_evtchn();
+
     /*
      * This needs to be called **before** heap_init_late() so modules
      * will be scrubbed (unless suppressed).
diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
index 430a1ef445..5579c875d2 100644
--- a/xen/include/xen/device_tree.h
+++ b/xen/include/xen/device_tree.h
@@ -82,6 +82,7 @@ struct dt_device_node {
     dt_phandle phandle;
     char *full_name;
     domid_t used_by; /* By default it's used by dom0 */
+    bool_t static_evtchn_created;
 
     struct dt_property *properties;
     struct dt_device_node *parent;
@@ -317,6 +318,18 @@ static inline bool_t dt_property_name_is_equal(const struct dt_property *pp,
     return !dt_prop_cmp(pp->name, name);
 }
 
+static inline void
+dt_device_set_static_evtchn_created(struct dt_device_node *device)
+{
+    device->static_evtchn_created = true;
+}
+
+static inline bool_t
+dt_device_static_evtchn_created(const struct dt_device_node *device)
+{
+    return device->static_evtchn_created;
+}
+
 /**
  * dt_find_compatible_node - Find a node based on type and one of the
  *                           tokens in its "compatible" property
-- 
2.25.1

[PATCH v3 7/7] xen/arm: introduce new xen,enhanced property value

[Rahul Singh]

Introduce a new "xen,enhanced" dom0less property value "no-xenstore" to
disable xenstore interface for dom0less guests.

Signed-off-by: Rahul Singh <rahul.singh@arm.com>
---
Changes in v3:
 - new patch in this version
---
 docs/misc/arm/device-tree/booting.txt |  4 ++++
 xen/arch/arm/domain_build.c           | 10 +++++++---
 xen/arch/arm/include/asm/kernel.h     |  3 +++
 3 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt
index edef98e6d1..87f57f8889 100644
--- a/docs/misc/arm/device-tree/booting.txt
+++ b/docs/misc/arm/device-tree/booting.txt
@@ -204,6 +204,10 @@ with the following properties:
     - "disabled"
     Xen PV interfaces are disabled.
 
+    - no-xenstore
+    Xen PV interfaces, including grant-table will be enabled for the VM but
+    xenstore will be disabled for the VM.
+
     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.
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 4b24261825..8dd9984225 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -3336,10 +3336,14 @@ static int __init construct_domU(struct domain *d,
          (rc == 0 && !strcmp(dom0less_enhanced, "enabled")) )
     {
         if ( hardware_domain )
-            kinfo.dom0less_enhanced = true;
+            kinfo.dom0less_xenstore = true;
         else
-            panic("Tried to use xen,enhanced without dom0\n");
+            panic("Tried to use xen,enhanced without dom0 without no-xenstore\n");
     }
+    else if ( rc == 0 && !strcmp(dom0less_enhanced, "no-xenstore") )
+        kinfo.dom0less_xenstore = false;
+
+    kinfo.dom0less_enhanced = true;
 
     if ( vcpu_create(d, 0) == NULL )
         return -ENOMEM;
@@ -3379,7 +3383,7 @@ static int __init construct_domU(struct domain *d,
     if ( rc < 0 )
         return rc;
 
-    if ( kinfo.dom0less_enhanced )
+    if ( kinfo.dom0less_xenstore )
     {
         ASSERT(hardware_domain);
         rc = alloc_xenstore_evtchn(d);
diff --git a/xen/arch/arm/include/asm/kernel.h b/xen/arch/arm/include/asm/kernel.h
index c4dc039b54..3d7fa94910 100644
--- a/xen/arch/arm/include/asm/kernel.h
+++ b/xen/arch/arm/include/asm/kernel.h
@@ -39,6 +39,9 @@ struct kernel_info {
     /* Enable PV drivers */
     bool dom0less_enhanced;
 
+    /* Enable Xenstore */
+    bool dom0less_xenstore;
+
     /* GIC phandle */
     uint32_t phandle_gic;
 
-- 
2.25.1

Re: [PATCH v3 2/7] xen/evtchn: Add an helper to reserve/allocate a port

[Julien Grall]

Hi Rahul,

On 01/09/2022 10:13, Rahul Singh wrote:
> In a follow-up patch we will want to either reserve or allocate a port
> for various event channel helpers.
> 
> A new wrapper is introduced to either reserve a given port or allocate
> a fresh one if zero.
> 
> Take the opportunity to replace the open-coded version in
> evtchn_bind_virq().
> 
> Signed-off-by: Stanislav Kinsburskii <staskins@amazon.com>

Usually, the "From:" match the first signed-off-by. This is not the case 
here. Can you clarify whether you effectively rewrite the patch or 
simply took it?

If the former, then I would suggest to write: "Based on ..."
If the latter, then please update the "From:".

Cheers,

-- 
Julien Grall

Re: [PATCH v3 2/7] xen/evtchn: Add an helper to reserve/allocate a port

[Rahul Singh]

Hi Julien,

> On 1 Sep 2022, at 10:27 am, Julien Grall <julien@xen.org> wrote:
> 
> Hi Rahul,
> 
> On 01/09/2022 10:13, Rahul Singh wrote:
>> In a follow-up patch we will want to either reserve or allocate a port
>> for various event channel helpers.
>> A new wrapper is introduced to either reserve a given port or allocate
>> a fresh one if zero.
>> Take the opportunity to replace the open-coded version in
>> evtchn_bind_virq().
>> Signed-off-by: Stanislav Kinsburskii <staskins@amazon.com>
> 
> Usually, the "From:" match the first signed-off-by. This is not the case here. Can you clarify whether you effectively rewrite the patch or simply took it?

I just took the patch with small changes.
> 
> If the former, then I would suggest to write: "Based on ..."
> If the latter, then please update the "From:".

Ok. I will update the “From:”

Regards,
Rahul

Re: [PATCH v3 1/7] xen/evtchn: Make sure all buckets below d->valid_evtchns are allocated

[Michal Orzel]

Hi Rahul,

On 01/09/2022 11:13, Rahul Singh wrote:
> 
> From: Julien Grall <jgrall@amazon.com>
> 
> Since commit 01280dc19cf3 "evtchn: simplify port_is_valid()", the event
> channels code assumes that all the buckets below d->valid_evtchns are
> always allocated.
> 
> This assumption hold in most of the situation because a guest is not
> allowed to chose the port. Instead, it will be the first free from port
> 0.
> 
> When static event channel support will be added for dom0less domains
> user can request to allocate the evtchn port numbers that are scattered
> in nature.
> 
> The existing implementation of evtchn_allocate_port() is not able to
> deal with such situation and will end up to override bucket or/and leave
> some bucket unallocated. The latter will result to a droplet crash if
> the event channel belongs to an unallocated bucket.
> 
> This can be solved by making sure that all the buckets below
> d->valid_evtchns are allocated. There should be no impact for most of
> the situation but LM/LU as only one bucket would be allocated. For
> LM/LU, we may end up to allocate multiple buckets if ports in use are
> sparse.
> 
> A potential alternative is to check that the bucket is valid in
> is_port_valid(). This should still possible to do it without taking
> per-domain lock but will result a couple more of memory access.
> 
> Signed-off-by: Julien Grall <jgrall@amazon.com>
> Signed-off-by: Rahul Singh <rahul.singh@arm.com>
> ---
> Changes in v3:
>  - fix comments in commit msg.
>  - modify code related to d->valid_evtchns and {read,write}_atomic()
> Changes in v2:
>  - new patch in this version to avoid the security issue
> ---
>  xen/common/event_channel.c | 55 ++++++++++++++++++++++++--------------
>  1 file changed, 35 insertions(+), 20 deletions(-)
> 
> diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
> index c2c6f8c151..80b06d9743 100644
> --- a/xen/common/event_channel.c
> +++ b/xen/common/event_channel.c
> @@ -193,6 +193,15 @@ static struct evtchn *alloc_evtchn_bucket(struct domain *d, unsigned int port)
>      return NULL;
>  }
> 
> +/*
> + * Allocate a given port and ensure all the buckets up to that ports
> + * have been allocated.
> + *
> + * The last part is important because the rest of the event channel code
> + * relies on all the buckets up to d->valid_evtchns to be valid. However,
> + * event channels may be sparsed when restoring a domain during Guest
> + * Transparent Migration and Live Update.
You got rid of mentioning these non-existing features from the commit msg,
but you still mention them here.

Apart from that, all the Jan comments were addressed, so:
Reviewed-by: Michal Orzel <michal.orzel@amd.com>

~Michal

Re: [PATCH v3 1/7] xen/evtchn: Make sure all buckets below d->valid_evtchns are allocated

[Rahul Singh]

Hi Michal,

> On 1 Sep 2022, at 1:47 pm, Michal Orzel <michal.orzel@amd.com> wrote:
> 
> Hi Rahul,
> 
> On 01/09/2022 11:13, Rahul Singh wrote:
>> 
>> From: Julien Grall <jgrall@amazon.com>
>> 
>> Since commit 01280dc19cf3 "evtchn: simplify port_is_valid()", the event
>> channels code assumes that all the buckets below d->valid_evtchns are
>> always allocated.
>> 
>> This assumption hold in most of the situation because a guest is not
>> allowed to chose the port. Instead, it will be the first free from port
>> 0.
>> 
>> When static event channel support will be added for dom0less domains
>> user can request to allocate the evtchn port numbers that are scattered
>> in nature.
>> 
>> The existing implementation of evtchn_allocate_port() is not able to
>> deal with such situation and will end up to override bucket or/and leave
>> some bucket unallocated. The latter will result to a droplet crash if
>> the event channel belongs to an unallocated bucket.
>> 
>> This can be solved by making sure that all the buckets below
>> d->valid_evtchns are allocated. There should be no impact for most of
>> the situation but LM/LU as only one bucket would be allocated. For
>> LM/LU, we may end up to allocate multiple buckets if ports in use are
>> sparse.
>> 
>> A potential alternative is to check that the bucket is valid in
>> is_port_valid(). This should still possible to do it without taking
>> per-domain lock but will result a couple more of memory access.
>> 
>> Signed-off-by: Julien Grall <jgrall@amazon.com>
>> Signed-off-by: Rahul Singh <rahul.singh@arm.com>
>> ---
>> Changes in v3:
>> - fix comments in commit msg.
>> - modify code related to d->valid_evtchns and {read,write}_atomic()
>> Changes in v2:
>> - new patch in this version to avoid the security issue
>> ---
>> xen/common/event_channel.c | 55 ++++++++++++++++++++++++--------------
>> 1 file changed, 35 insertions(+), 20 deletions(-)
>> 
>> diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
>> index c2c6f8c151..80b06d9743 100644
>> --- a/xen/common/event_channel.c
>> +++ b/xen/common/event_channel.c
>> @@ -193,6 +193,15 @@ static struct evtchn *alloc_evtchn_bucket(struct domain *d, unsigned int port)
>>     return NULL;
>> }
>> 
>> +/*
>> + * Allocate a given port and ensure all the buckets up to that ports
>> + * have been allocated.
>> + *
>> + * The last part is important because the rest of the event channel code
>> + * relies on all the buckets up to d->valid_evtchns to be valid. However,
>> + * event channels may be sparsed when restoring a domain during Guest
>> + * Transparent Migration and Live Update.
> You got rid of mentioning these non-existing features from the commit msg,
> but you still mention them here.

I missed that I will fix that in next version.

Regards,
Rahul

Re: [PATCH v3 3/7] xen/evtchn: restrict the maximum number of evtchn supported for domUs

[Michal Orzel]

Hi Rahul,

On 01/09/2022 11:13, Rahul Singh wrote:
> 
> Restrict the maximum number of evtchn supported for domUs to avoid
> allocating a large amount of memory in Xen.
> 
> Set the default value of max_evtchn_port to 1023. The value of 1023
> should be sufficient for domUs guests because on ARM we don't bind
> physical interrupts to event channels. The only use of the evtchn port
> is inter-domain communications.
Following the previous discussion, I think the only missing piece is
an explanation that 1023 was chose to follow the default behavior of libxl.

Apart from that:
Reviewed-by: Michal Orzel <michal.orzel@amd.com>

~Michal

Re: [PATCH v3 3/7] xen/evtchn: restrict the maximum number of evtchn supported for domUs

[Julien Grall]

Hi,

On 01/09/2022 14:53, Michal Orzel wrote:
> On 01/09/2022 11:13, Rahul Singh wrote:
>>
>> Restrict the maximum number of evtchn supported for domUs to avoid
>> allocating a large amount of memory in Xen.
>>
>> Set the default value of max_evtchn_port to 1023. The value of 1023
>> should be sufficient for domUs guests because on ARM we don't bind
>> physical interrupts to event channels. The only use of the evtchn port
>> is inter-domain communications.
> Following the previous discussion, I think the only missing piece is
> an explanation that 1023 was chose to follow the default behavior of libxl.

+1. The current explanation only justify why we haven't added a 
device-tree property to change the default value.

> 
> Apart from that:
> Reviewed-by: Michal Orzel <michal.orzel@amd.com>

Cheers,

-- 
Julien Grall

Re: [PATCH v3 6/7] xen/arm: introduce xen-evtchn dom0less property

[Julien Grall]

Hi Rahul,

On 01/09/2022 10:13, Rahul Singh wrote:
> Introduce a new sub-node under /chosen node to establish static event
> channel communication between domains on dom0less systems.
> 
> An event channel will be created beforehand to allow the domains to
> send notifications to each other.
> 
> Signed-off-by: Rahul Singh <rahul.singh@arm.com>
> ---
> Changes in v3:
>   - use device-tree used_by to find the domain id of the evtchn node.
>   - add new static_evtchn_create variable in struct dt_device_node to
>     hold the information if evtchn is already created.
>   - fix minor comments
> Changes in v2:
>   - no change
> ---
>   docs/misc/arm/device-tree/booting.txt |  64 ++++++++++++-
>   xen/arch/arm/domain_build.c           | 128 ++++++++++++++++++++++++++
>   xen/arch/arm/include/asm/setup.h      |   1 +
>   xen/arch/arm/setup.c                  |   2 +
>   xen/include/xen/device_tree.h         |  13 +++
>   5 files changed, 207 insertions(+), 1 deletion(-)
> 
> diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt
> index 98253414b8..edef98e6d1 100644
> --- a/docs/misc/arm/device-tree/booting.txt
> +++ b/docs/misc/arm/device-tree/booting.txt
> @@ -212,7 +212,7 @@ with the following properties:
>       enable only selected interfaces.
>   
>   Under the "xen,domain" compatible node, one or more sub-nodes are present
> -for the DomU kernel and ramdisk.
> +for the DomU kernel, ramdisk and static event channel.
>   
>   The kernel sub-node has the following properties:
>   
> @@ -254,11 +254,44 @@ The ramdisk sub-node has the following properties:
>       property because it will be created by the UEFI stub on boot.
>       This option is needed only when UEFI boot is used.
>   
> +The static event channel sub-node has the following properties:
> +
> +- compatible
> +
> +    "xen,evtchn"
> +
> +- xen,evtchn
> +
> +    The property is tuples of two numbers
> +    (local-evtchn link-to-foreign-evtchn) where:
> +
> +    local-evtchn is an integer value that will be used to allocate local port
> +    for a domain to send and receive event notifications to/from the remote
> +    domain. Maximum supported value is 2^17 for FIFO ABI and 4096 for 2L ABI.
> +    It is recommended to use low event channel IDs.
> +
> +    link-to-foreign-evtchn is a single phandle to a remote evtchn to which
> +    local-evtchn will be connected.
>   
>   Example
>   =======
>   
>   chosen {
> +
> +    module@0 {
> +        compatible = "multiboot,kernel", "multiboot,module";
> +        xen,uefi-binary = "...";
> +        bootargs = "...";
> +
> +    };

NIT: Describing this node in the example seems unnecessary.

> +
> +    /* one sub-node per local event channel */
> +    ec1: evtchn@1 {
> +         compatible = "xen,evtchn-v1";
> +         /* local-evtchn link-to-foreign-evtchn */
> +         xen,evtchn = <0xa &ec2>;
> +    };
> +

Here you provide an example for dom0. But the position where you 
describe the binding suggests this binding only exists for domUs.

Either we duplicate the binding or we re-order the documentation to have 
common binding in a single place. My preference would be the latter.

>       domU1 {
>           compatible = "xen,domain";
>           #address-cells = <0x2>;
> @@ -277,6 +310,23 @@ chosen {
>               compatible = "multiboot,ramdisk", "multiboot,module";
>               reg = <0x0 0x4b000000 0xffffff>;
>           };
> +
> +        /* one sub-node per local event channel */
> +        ec2: evtchn@2 {
> +            compatible = "xen,evtchn-v1";
> +            /* local-evtchn link-to-foreign-evtchn */
> +            xen,evtchn = <0xa &ec1>;
> +        };
> +
> +        ec3: evtchn@3 {
> +            compatible = "xen,evtchn-v1";
> +            xen,evtchn = <0xb &ec5>;
> +        };
> +
> +        ec4: evtchn@4 {
> +            compatible = "xen,evtchn-v1";
> +            xen,evtchn = <0xc &ec6>;
> +        };
>       };
>   
>       domU2 {
> @@ -296,6 +346,18 @@ chosen {
>               compatible = "multiboot,ramdisk", "multiboot,module";
>               reg = <0x0 0x4d000000 0xffffff>;
>           };
> +
> +        /* one sub-node per local event channel */
> +        ec5: evtchn@5 {
> +            compatible = "xen,evtchn-v1";
> +            /* local-evtchn link-to-foreign-evtchn */
> +            xen,evtchn = <0xb &ec3>;
> +        };
> +
> +        ec6: evtchn@6 {
> +            compatible = "xen,evtchn-v1";
> +            xen,evtchn = <0xd &ec4>;
> +        };
>       };
>   };
>   
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 707e247f6a..4b24261825 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -33,6 +33,8 @@
>   #include <xen/grant_table.h>
>   #include <xen/serial.h>
>   
> +#define STATIC_EVTCHN_NODE_SIZE_CELLS 2
> +
>   static unsigned int __initdata opt_dom0_max_vcpus;
>   integer_param("dom0_max_vcpus", opt_dom0_max_vcpus);
>   
> @@ -3052,6 +3054,131 @@ void __init evtchn_allocate(struct domain *d)
>       d->arch.hvm.params[HVM_PARAM_CALLBACK_IRQ] = val;
>   }
>   
> +static int __init get_evtchn_dt_property(const struct dt_device_node *np,
> +                                         uint32_t *port, uint32_t *phandle)
> +{
> +    const __be32 *prop = NULL;
> +    uint32_t len;
> +
> +    prop = dt_get_property(np, "xen,evtchn", &len);
> +    if ( !prop )
> +    {
> +        printk(XENLOG_ERR "xen,evtchn property should not be empty.\n");
> +        return -EINVAL;
> +    }
> +
> +    if ( !len || len < dt_cells_to_size(STATIC_EVTCHN_NODE_SIZE_CELLS) )
> +    {
> +        printk(XENLOG_ERR "xen,evtchn property value is not valid.\n");
> +        return -EINVAL;
> +    }
> +
> +    *port = dt_next_cell(1, &prop);
> +    *phandle = dt_next_cell(1, &prop);
> +
> +    return 0;
> +}
> +
> +static int __init alloc_domain_evtchn(struct dt_device_node *node)
> +{
> +    int rc;
> +    uint32_t domU1_port, domU2_port, remote_phandle;
> +    struct dt_device_node *remote_node;
> +    struct evtchn_alloc_unbound alloc_unbound;
> +    struct evtchn_bind_interdomain bind_interdomain;
> +    struct domain *d1 = NULL, *d2 = NULL;
> +
> +    if ( dt_device_static_evtchn_created(node) )

I think this deserve a comment explain why the node would be created. 
I.e it would happen if the other side was created first. I will comment 
about dt_device_static_evtchn_created() futher down.

> +        return 0;
> +
> +    rc = get_evtchn_dt_property(node, &domU1_port, &remote_phandle);
> +    if ( rc )
> +        return rc;
> +
> +    remote_node = dt_find_node_by_phandle(remote_phandle);
> +    if ( !remote_node )
> +    {
> +        printk(XENLOG_ERR
> +                "evtchn: could not find remote evtchn phandle\n");
> +        return -EINVAL;
> +    }
> +
> +    rc = get_evtchn_dt_property(remote_node, &domU2_port, &remote_phandle);
> +    if ( rc )
> +        return rc;
> +
> +    if ( node->phandle != remote_phandle )
> +    {
> +        printk(XENLOG_ERR "xen,evtchn property is not setup correctly.\n");
> +        return -EINVAL;
> +    }
> +
> +    d1 = get_domain_by_id(dt_get_parent(node)->used_by);
> +    d2 = get_domain_by_id(dt_get_parent(remote_node)->used_by);

I think dt_get_parent() could return NULL (i.e. for the root). So I 
think you want to check that at least remote_node() has a parent. For 
convenience, this check could be done in

> +
> +    if ( !d1 || !d2 )
> +    {
> +        printk(XENLOG_ERR "evtchn: could not find domains\n" );
> +        return -EINVAL;
> +    }
> +
> +    alloc_unbound.dom = d1->domain_id;
> +    alloc_unbound.remote_dom = d2->domain_id;
> +
> +    rc = evtchn_alloc_unbound(&alloc_unbound, domU1_port);
> +    if ( rc < 0 )
> +    {
> +        printk(XENLOG_ERR
> +                "evtchn_alloc_unbound() failure (Error %d) \n", rc);
> +        return rc;
> +    }
> +
> +    bind_interdomain.remote_dom  = d1->domain_id;
> +    bind_interdomain.remote_port = domU1_port;
> +
> +    rc = evtchn_bind_interdomain(&bind_interdomain, d2, domU2_port);
> +    if ( rc < 0 )
> +    {
> +        printk(XENLOG_ERR
> +                "evtchn_bind_interdomain() failure (Error %d) \n", rc);
> +        return rc;
> +    }
> +
> +    dt_device_set_static_evtchn_created(node);
> +    dt_device_set_static_evtchn_created(remote_node);
> +
> +    return 0;
> +}
> +
> +void __init process_static_evtchn_node(struct dt_device_node *node)

This is missing a prototype. So I guess this wants to be static?

That said, I think it would make more sense to fold 
process_static_evtchn_node() in alloc_domain_evtchn() or 
alloc_static-evtchn().

> +{
> +    if ( dt_device_is_compatible(node, "xen,evtchn-v1") )
> +    {
> +        if ( alloc_domain_evtchn(node) != 0 )
> +            panic("Could not set up domains evtchn\n");
> +    }
> +}
> +
> +void __init alloc_static_evtchn(void)
> +{
> +    struct dt_device_node *node, *evtchn_node;
> +    struct dt_device_node *chosen = dt_find_node_by_path("/chosen");
> +
> +    BUG_ON(chosen == NULL);
> +
> +    if ( hardware_domain )
> +        dt_device_set_used_by(chosen, hardware_domain->domain_id);
> +
> +    dt_for_each_child_node(chosen, node)
> +    {
> +        if ( hardware_domain )
> +            process_static_evtchn_node(node);
> +
> +        dt_for_each_child_node(node, evtchn_node)
> +            process_static_evtchn_node(evtchn_node);
> +    }
> +}
> +
>   static void __init find_gnttab_region(struct domain *d,
>                                         struct kernel_info *kinfo)
>   {
> @@ -3364,6 +3491,7 @@ void __init create_domUs(void)
>               panic("Error creating domain %s\n", dt_node_name(node));
>   
>           d->is_console = true;
> +        dt_device_set_used_by(node, d->domain_id);
>   
>           if ( construct_domU(d, node) != 0 )
>               panic("Could not set up domain %s\n", dt_node_name(node));
> diff --git a/xen/arch/arm/include/asm/setup.h b/xen/arch/arm/include/asm/setup.h
> index 5815ccf8c5..5ee28b270f 100644
> --- a/xen/arch/arm/include/asm/setup.h
> +++ b/xen/arch/arm/include/asm/setup.h
> @@ -106,6 +106,7 @@ int acpi_make_efi_nodes(void *fdt, struct membank tbl_add[]);
>   
>   void create_domUs(void);
>   void create_dom0(void);
> +void alloc_static_evtchn(void);
>   
>   void discard_initial_modules(void);
>   void fw_unreserved_regions(paddr_t s, paddr_t e,
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index 6e0398f3f6..cf15d359d2 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -1077,6 +1077,8 @@ void __init start_xen(unsigned long boot_phys_offset,
>       if ( acpi_disabled )
>           create_domUs();
>   
> +    alloc_static_evtchn();
> +
>       /*
>        * This needs to be called **before** heap_init_late() so modules
>        * will be scrubbed (unless suppressed).
> diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
> index 430a1ef445..5579c875d2 100644
> --- a/xen/include/xen/device_tree.h
> +++ b/xen/include/xen/device_tree.h
> @@ -82,6 +82,7 @@ struct dt_device_node {
>       dt_phandle phandle;
>       char *full_name;
>       domid_t used_by; /* By default it's used by dom0 */
> +    bool_t static_evtchn_created;

I can see why you want to add the boolean in dt_device_node. However, I 
dislike this approach because this feels an abuse of dt_device_node and 
the field is only used at boot.

So this seems to be a bit of a waste to include it in the structure 
(even if we are re-using padding today).

I don't have a solution that is has trivial as this approach. However, 
at minimum we should document this is a HACK and should be remove if we 
need space in the structure.

>   
>       struct dt_property *properties;
>       struct dt_device_node *parent;
> @@ -317,6 +318,18 @@ static inline bool_t dt_property_name_is_equal(const struct dt_property *pp,
>       return !dt_prop_cmp(pp->name, name);
>   }
>   
> +static inline void
> +dt_device_set_static_evtchn_created(struct dt_device_node *device)
> +{
> +    device->static_evtchn_created = true;
> +}
> +
> +static inline bool_t
> +dt_device_static_evtchn_created(const struct dt_device_node *device)
> +{
> +    return device->static_evtchn_created;
> +}
> +
>   /**
>    * dt_find_compatible_node - Find a node based on type and one of the
>    *                           tokens in its "compatible" property

Cheers,

-- 
Julien Grall

Re: [PATCH v3 7/7] xen/arm: introduce new xen,enhanced property value

[Julien Grall]

Hi Rahul,

On 01/09/2022 10:13, Rahul Singh wrote:
> Introduce a new "xen,enhanced" dom0less property value "no-xenstore" to
> disable xenstore interface for dom0less guests.
> 
> Signed-off-by: Rahul Singh <rahul.singh@arm.com>
> ---
> Changes in v3:
>   - new patch in this version
> ---
>   docs/misc/arm/device-tree/booting.txt |  4 ++++
>   xen/arch/arm/domain_build.c           | 10 +++++++---
>   xen/arch/arm/include/asm/kernel.h     |  3 +++
>   3 files changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt
> index edef98e6d1..87f57f8889 100644
> --- a/docs/misc/arm/device-tree/booting.txt
> +++ b/docs/misc/arm/device-tree/booting.txt
> @@ -204,6 +204,10 @@ with the following properties:
>       - "disabled"
>       Xen PV interfaces are disabled.
>   
> +    - no-xenstore
> +    Xen PV interfaces, including grant-table will be enabled for the VM but
> +    xenstore will be disabled for the VM.

NIT: I would drop one of the "for the VM" as it seems to be redundant.

> +
>       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.
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 4b24261825..8dd9984225 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -3336,10 +3336,14 @@ static int __init construct_domU(struct domain *d,
>            (rc == 0 && !strcmp(dom0less_enhanced, "enabled")) )
>       {
>           if ( hardware_domain )
> -            kinfo.dom0less_enhanced = true;
> +            kinfo.dom0less_xenstore = true;
>           else
> -            panic("Tried to use xen,enhanced without dom0\n");
> +            panic("Tried to use xen,enhanced without dom0 without no-xenstore\n");

This is a bit hard to parse. How about:

"At the moment, Xenstore support requires dom0 to be present"

>       }
> +    else if ( rc == 0 && !strcmp(dom0less_enhanced, "no-xenstore") )
> +        kinfo.dom0less_xenstore = false;
> +
> +    kinfo.dom0less_enhanced = true;

Wouldn't this now set dom0less_enhanced unconditionally?

>   
>       if ( vcpu_create(d, 0) == NULL )
>           return -ENOMEM;
> @@ -3379,7 +3383,7 @@ static int __init construct_domU(struct domain *d,
>       if ( rc < 0 )
>           return rc;
>   
> -    if ( kinfo.dom0less_enhanced )
> +    if ( kinfo.dom0less_xenstore )
>       {
>           ASSERT(hardware_domain);
>           rc = alloc_xenstore_evtchn(d);
> diff --git a/xen/arch/arm/include/asm/kernel.h b/xen/arch/arm/include/asm/kernel.h
> index c4dc039b54..3d7fa94910 100644
> --- a/xen/arch/arm/include/asm/kernel.h
> +++ b/xen/arch/arm/include/asm/kernel.h
> @@ -39,6 +39,9 @@ struct kernel_info {
>       /* Enable PV drivers */
>       bool dom0less_enhanced;
>   
> +    /* Enable Xenstore */
> +    bool dom0less_xenstore;
> +

AFAIU, it is not possible to have *_xenstore = true and *_enhanced = 
false. I think it would be clearer if ``dom0less_enhanced`` is turned to 
an enum with 3 values:
  - None
  - NOXENSTORE/BASIC
  - FULLY_ENHANCED

If we want to be future proof, I would use a field 'flags' where 
non-zero means enhanced. Each bit would indicate which features of Xen 
is exposed.

Cheers,

-- 
Julien Grall

Re: [PATCH v3 6/7] xen/arm: introduce xen-evtchn dom0less property

[Stefano Stabellini]

On Thu, 1 Sep 2022, Julien Grall wrote:
> On 01/09/2022 10:13, Rahul Singh wrote:
> > Introduce a new sub-node under /chosen node to establish static event
> > channel communication between domains on dom0less systems.
> > 
> > An event channel will be created beforehand to allow the domains to
> > send notifications to each other.
> > 
> > Signed-off-by: Rahul Singh <rahul.singh@arm.com>
> > ---
> > Changes in v3:
> >   - use device-tree used_by to find the domain id of the evtchn node.
> >   - add new static_evtchn_create variable in struct dt_device_node to
> >     hold the information if evtchn is already created.
> >   - fix minor comments
> > Changes in v2:
> >   - no change
> > ---
> >   docs/misc/arm/device-tree/booting.txt |  64 ++++++++++++-
> >   xen/arch/arm/domain_build.c           | 128 ++++++++++++++++++++++++++
> >   xen/arch/arm/include/asm/setup.h      |   1 +
> >   xen/arch/arm/setup.c                  |   2 +
> >   xen/include/xen/device_tree.h         |  13 +++
> >   5 files changed, 207 insertions(+), 1 deletion(-)
> > 
> > diff --git a/docs/misc/arm/device-tree/booting.txt
> > b/docs/misc/arm/device-tree/booting.txt
> > index 98253414b8..edef98e6d1 100644
> > --- a/docs/misc/arm/device-tree/booting.txt
> > +++ b/docs/misc/arm/device-tree/booting.txt
> > @@ -212,7 +212,7 @@ with the following properties:
> >       enable only selected interfaces.
> >     Under the "xen,domain" compatible node, one or more sub-nodes are
> > present
> > -for the DomU kernel and ramdisk.
> > +for the DomU kernel, ramdisk and static event channel.
> >     The kernel sub-node has the following properties:
> >   @@ -254,11 +254,44 @@ The ramdisk sub-node has the following properties:
> >       property because it will be created by the UEFI stub on boot.
> >       This option is needed only when UEFI boot is used.
> >   +The static event channel sub-node has the following properties:
> > +
> > +- compatible
> > +
> > +    "xen,evtchn"
> > +
> > +- xen,evtchn
> > +
> > +    The property is tuples of two numbers
> > +    (local-evtchn link-to-foreign-evtchn) where:
> > +
> > +    local-evtchn is an integer value that will be used to allocate local
> > port
> > +    for a domain to send and receive event notifications to/from the remote
> > +    domain. Maximum supported value is 2^17 for FIFO ABI and 4096 for 2L
> > ABI.
> > +    It is recommended to use low event channel IDs.
> > +
> > +    link-to-foreign-evtchn is a single phandle to a remote evtchn to which
> > +    local-evtchn will be connected.
> >     Example
> >   =======
> >     chosen {
> > +
> > +    module@0 {
> > +        compatible = "multiboot,kernel", "multiboot,module";
> > +        xen,uefi-binary = "...";
> > +        bootargs = "...";
> > +
> > +    };
> 
> NIT: Describing this node in the example seems unnecessary.
> 
> > +
> > +    /* one sub-node per local event channel */
> > +    ec1: evtchn@1 {
> > +         compatible = "xen,evtchn-v1";
> > +         /* local-evtchn link-to-foreign-evtchn */
> > +         xen,evtchn = <0xa &ec2>;
> > +    };
> > +
> 
> Here you provide an example for dom0. But the position where you describe the
> binding suggests this binding only exists for domUs.
> 
> Either we duplicate the binding or we re-order the documentation to have
> common binding in a single place. My preference would be the latter.
> 
> >       domU1 {
> >           compatible = "xen,domain";
> >           #address-cells = <0x2>;
> > @@ -277,6 +310,23 @@ chosen {
> >               compatible = "multiboot,ramdisk", "multiboot,module";
> >               reg = <0x0 0x4b000000 0xffffff>;
> >           };
> > +
> > +        /* one sub-node per local event channel */
> > +        ec2: evtchn@2 {
> > +            compatible = "xen,evtchn-v1";
> > +            /* local-evtchn link-to-foreign-evtchn */
> > +            xen,evtchn = <0xa &ec1>;
> > +        };
> > +
> > +        ec3: evtchn@3 {
> > +            compatible = "xen,evtchn-v1";
> > +            xen,evtchn = <0xb &ec5>;
> > +        };
> > +
> > +        ec4: evtchn@4 {
> > +            compatible = "xen,evtchn-v1";
> > +            xen,evtchn = <0xc &ec6>;
> > +        };
> >       };
> >         domU2 {
> > @@ -296,6 +346,18 @@ chosen {
> >               compatible = "multiboot,ramdisk", "multiboot,module";
> >               reg = <0x0 0x4d000000 0xffffff>;
> >           };
> > +
> > +        /* one sub-node per local event channel */
> > +        ec5: evtchn@5 {
> > +            compatible = "xen,evtchn-v1";
> > +            /* local-evtchn link-to-foreign-evtchn */
> > +            xen,evtchn = <0xb &ec3>;
> > +        };
> > +
> > +        ec6: evtchn@6 {
> > +            compatible = "xen,evtchn-v1";
> > +            xen,evtchn = <0xd &ec4>;
> > +        };
> >       };
> >   };
> >   diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > index 707e247f6a..4b24261825 100644
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -33,6 +33,8 @@
> >   #include <xen/grant_table.h>
> >   #include <xen/serial.h>
> >   +#define STATIC_EVTCHN_NODE_SIZE_CELLS 2
> > +
> >   static unsigned int __initdata opt_dom0_max_vcpus;
> >   integer_param("dom0_max_vcpus", opt_dom0_max_vcpus);
> >   @@ -3052,6 +3054,131 @@ void __init evtchn_allocate(struct domain *d)
> >       d->arch.hvm.params[HVM_PARAM_CALLBACK_IRQ] = val;
> >   }
> >   +static int __init get_evtchn_dt_property(const struct dt_device_node *np,
> > +                                         uint32_t *port, uint32_t *phandle)
> > +{
> > +    const __be32 *prop = NULL;
> > +    uint32_t len;
> > +
> > +    prop = dt_get_property(np, "xen,evtchn", &len);
> > +    if ( !prop )
> > +    {
> > +        printk(XENLOG_ERR "xen,evtchn property should not be empty.\n");
> > +        return -EINVAL;
> > +    }
> > +
> > +    if ( !len || len < dt_cells_to_size(STATIC_EVTCHN_NODE_SIZE_CELLS) )
> > +    {
> > +        printk(XENLOG_ERR "xen,evtchn property value is not valid.\n");
> > +        return -EINVAL;
> > +    }
> > +
> > +    *port = dt_next_cell(1, &prop);
> > +    *phandle = dt_next_cell(1, &prop);
> > +
> > +    return 0;
> > +}
> > +
> > +static int __init alloc_domain_evtchn(struct dt_device_node *node)
> > +{
> > +    int rc;
> > +    uint32_t domU1_port, domU2_port, remote_phandle;
> > +    struct dt_device_node *remote_node;
> > +    struct evtchn_alloc_unbound alloc_unbound;
> > +    struct evtchn_bind_interdomain bind_interdomain;
> > +    struct domain *d1 = NULL, *d2 = NULL;
> > +
> > +    if ( dt_device_static_evtchn_created(node) )
> 
> I think this deserve a comment explain why the node would be created. I.e it
> would happen if the other side was created first. I will comment about
> dt_device_static_evtchn_created() futher down.
> 
> > +        return 0;
> > +
> > +    rc = get_evtchn_dt_property(node, &domU1_port, &remote_phandle);
> > +    if ( rc )
> > +        return rc;
> > +
> > +    remote_node = dt_find_node_by_phandle(remote_phandle);
> > +    if ( !remote_node )
> > +    {
> > +        printk(XENLOG_ERR
> > +                "evtchn: could not find remote evtchn phandle\n");
> > +        return -EINVAL;
> > +    }
> > +
> > +    rc = get_evtchn_dt_property(remote_node, &domU2_port, &remote_phandle);
> > +    if ( rc )
> > +        return rc;
> > +
> > +    if ( node->phandle != remote_phandle )
> > +    {
> > +        printk(XENLOG_ERR "xen,evtchn property is not setup correctly.\n");
> > +        return -EINVAL;
> > +    }
> > +
> > +    d1 = get_domain_by_id(dt_get_parent(node)->used_by);
> > +    d2 = get_domain_by_id(dt_get_parent(remote_node)->used_by);
> 
> I think dt_get_parent() could return NULL (i.e. for the root). So I think you
> want to check that at least remote_node() has a parent. For convenience, this
> check could be done in
> 
> > +
> > +    if ( !d1 || !d2 )
> > +    {
> > +        printk(XENLOG_ERR "evtchn: could not find domains\n" );
> > +        return -EINVAL;
> > +    }
> > +
> > +    alloc_unbound.dom = d1->domain_id;
> > +    alloc_unbound.remote_dom = d2->domain_id;
> > +
> > +    rc = evtchn_alloc_unbound(&alloc_unbound, domU1_port);
> > +    if ( rc < 0 )
> > +    {
> > +        printk(XENLOG_ERR
> > +                "evtchn_alloc_unbound() failure (Error %d) \n", rc);
> > +        return rc;
> > +    }
> > +
> > +    bind_interdomain.remote_dom  = d1->domain_id;
> > +    bind_interdomain.remote_port = domU1_port;
> > +
> > +    rc = evtchn_bind_interdomain(&bind_interdomain, d2, domU2_port);
> > +    if ( rc < 0 )
> > +    {
> > +        printk(XENLOG_ERR
> > +                "evtchn_bind_interdomain() failure (Error %d) \n", rc);
> > +        return rc;
> > +    }
> > +
> > +    dt_device_set_static_evtchn_created(node);
> > +    dt_device_set_static_evtchn_created(remote_node);
> > +
> > +    return 0;
> > +}
> > +
> > +void __init process_static_evtchn_node(struct dt_device_node *node)
> 
> This is missing a prototype. So I guess this wants to be static?
> 
> That said, I think it would make more sense to fold
> process_static_evtchn_node() in alloc_domain_evtchn() or
> alloc_static-evtchn().
> 
> > +{
> > +    if ( dt_device_is_compatible(node, "xen,evtchn-v1") )
> > +    {
> > +        if ( alloc_domain_evtchn(node) != 0 )
> > +            panic("Could not set up domains evtchn\n");
> > +    }
> > +}
> > +
> > +void __init alloc_static_evtchn(void)
> > +{
> > +    struct dt_device_node *node, *evtchn_node;
> > +    struct dt_device_node *chosen = dt_find_node_by_path("/chosen");
> > +
> > +    BUG_ON(chosen == NULL);
> > +
> > +    if ( hardware_domain )
> > +        dt_device_set_used_by(chosen, hardware_domain->domain_id);
> > +
> > +    dt_for_each_child_node(chosen, node)
> > +    {
> > +        if ( hardware_domain )
> > +            process_static_evtchn_node(node);
> > +
> > +        dt_for_each_child_node(node, evtchn_node)
> > +            process_static_evtchn_node(evtchn_node);
> > +    }
> > +}
> > +
> >   static void __init find_gnttab_region(struct domain *d,
> >                                         struct kernel_info *kinfo)
> >   {
> > @@ -3364,6 +3491,7 @@ void __init create_domUs(void)
> >               panic("Error creating domain %s\n", dt_node_name(node));
> >             d->is_console = true;
> > +        dt_device_set_used_by(node, d->domain_id);
> >             if ( construct_domU(d, node) != 0 )
> >               panic("Could not set up domain %s\n", dt_node_name(node));
> > diff --git a/xen/arch/arm/include/asm/setup.h
> > b/xen/arch/arm/include/asm/setup.h
> > index 5815ccf8c5..5ee28b270f 100644
> > --- a/xen/arch/arm/include/asm/setup.h
> > +++ b/xen/arch/arm/include/asm/setup.h
> > @@ -106,6 +106,7 @@ int acpi_make_efi_nodes(void *fdt, struct membank
> > tbl_add[]);
> >     void create_domUs(void);
> >   void create_dom0(void);
> > +void alloc_static_evtchn(void);
> >     void discard_initial_modules(void);
> >   void fw_unreserved_regions(paddr_t s, paddr_t e,
> > diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> > index 6e0398f3f6..cf15d359d2 100644
> > --- a/xen/arch/arm/setup.c
> > +++ b/xen/arch/arm/setup.c
> > @@ -1077,6 +1077,8 @@ void __init start_xen(unsigned long boot_phys_offset,
> >       if ( acpi_disabled )
> >           create_domUs();
> >   +    alloc_static_evtchn();
> > +
> >       /*
> >        * This needs to be called **before** heap_init_late() so modules
> >        * will be scrubbed (unless suppressed).
> > diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
> > index 430a1ef445..5579c875d2 100644
> > --- a/xen/include/xen/device_tree.h
> > +++ b/xen/include/xen/device_tree.h
> > @@ -82,6 +82,7 @@ struct dt_device_node {
> >       dt_phandle phandle;
> >       char *full_name;
> >       domid_t used_by; /* By default it's used by dom0 */
> > +    bool_t static_evtchn_created;
> 
> I can see why you want to add the boolean in dt_device_node. However, I
> dislike this approach because this feels an abuse of dt_device_node and the
> field is only used at boot.
> 
> So this seems to be a bit of a waste to include it in the structure (even if
> we are re-using padding today).
> 
> I don't have a solution that is has trivial as this approach. However, at
> minimum we should document this is a HACK and should be remove if we need
> space in the structure.

I would move static_evtchn_created just above (or below) "bool
is_protected". It would still re-use the padding and it would be
closer to another more similar field of the struct.

The only other option that I can think of would be to use port_is_valid,
instead of static_evtchn_created, to check that the port has already
been allocated, but we wouldn't be able to tell if it is a static evtchn
or simply unavailable for other reasons and it would require more device
tree parsing.

Re: [PATCH v3 6/7] xen/arm: introduce xen-evtchn dom0less property

[Julien Grall]

Hi Stefano,

On 02/09/2022 03:20, Stefano Stabellini wrote:
>>> diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
>>> index 430a1ef445..5579c875d2 100644
>>> --- a/xen/include/xen/device_tree.h
>>> +++ b/xen/include/xen/device_tree.h
>>> @@ -82,6 +82,7 @@ struct dt_device_node {
>>>        dt_phandle phandle;
>>>        char *full_name;
>>>        domid_t used_by; /* By default it's used by dom0 */
>>> +    bool_t static_evtchn_created;
>>
>> I can see why you want to add the boolean in dt_device_node. However, I
>> dislike this approach because this feels an abuse of dt_device_node and the
>> field is only used at boot.
>>
>> So this seems to be a bit of a waste to include it in the structure (even if
>> we are re-using padding today).
>>
>> I don't have a solution that is has trivial as this approach. However, at
>> minimum we should document this is a HACK and should be remove if we need
>> space in the structure.
> 
> I would move static_evtchn_created just above (or below) "bool
> is_protected". It would still re-use the padding and it would be
> closer to another more similar field of the struct.
> 
> The only other option that I can think of would be to use port_is_valid,
> instead of static_evtchn_created, to check that the port has already
> been allocated, but we wouldn't be able to tell if it is a static evtchn
> or simply unavailable for other reasons

You don't need to know the event channel was statically allocated. If 
you have access to the event channel, then you can easily find out what 
is the remote port.

> and it would require more device
> tree parsing.

The parsing is indeed the big cons. Hence, why I hadn't suggest it.

Cheers,

-- 
Julien Grall

Re: [PATCH v3 7/7] xen/arm: introduce new xen,enhanced property value

[Bertrand Marquis]

Hi Julien,

> On 1 Sep 2022, at 19:15, Julien Grall <julien@xen.org> wrote:
> 
> Hi Rahul,
> 
> On 01/09/2022 10:13, Rahul Singh wrote:
>> Introduce a new "xen,enhanced" dom0less property value "no-xenstore" to
>> disable xenstore interface for dom0less guests.
>> Signed-off-by: Rahul Singh <rahul.singh@arm.com>
>> ---
>> Changes in v3:
>>  - new patch in this version
>> ---
>>  docs/misc/arm/device-tree/booting.txt |  4 ++++
>>  xen/arch/arm/domain_build.c           | 10 +++++++---
>>  xen/arch/arm/include/asm/kernel.h     |  3 +++
>>  3 files changed, 14 insertions(+), 3 deletions(-)
>> diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt
>> index edef98e6d1..87f57f8889 100644
>> --- a/docs/misc/arm/device-tree/booting.txt
>> +++ b/docs/misc/arm/device-tree/booting.txt
>> @@ -204,6 +204,10 @@ with the following properties:
>>      - "disabled"
>>      Xen PV interfaces are disabled.
>>  +    - no-xenstore
>> +    Xen PV interfaces, including grant-table will be enabled for the VM but
>> +    xenstore will be disabled for the VM.
> 
> NIT: I would drop one of the "for the VM" as it seems to be redundant.
> 
>> +
>>      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.
>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>> index 4b24261825..8dd9984225 100644
>> --- a/xen/arch/arm/domain_build.c
>> +++ b/xen/arch/arm/domain_build.c
>> @@ -3336,10 +3336,14 @@ static int __init construct_domU(struct domain *d,
>>           (rc == 0 && !strcmp(dom0less_enhanced, "enabled")) )
>>      {
>>          if ( hardware_domain )
>> -            kinfo.dom0less_enhanced = true;
>> +            kinfo.dom0less_xenstore = true;
>>          else
>> -            panic("Tried to use xen,enhanced without dom0\n");
>> +            panic("Tried to use xen,enhanced without dom0 without no-xenstore\n");
> 
> This is a bit hard to parse. How about:
> 
> "At the moment, Xenstore support requires dom0 to be present"
> 
>>      }
>> +    else if ( rc == 0 && !strcmp(dom0less_enhanced, "no-xenstore") )
>> +        kinfo.dom0less_xenstore = false;
>> +
>> +    kinfo.dom0less_enhanced = true;
> 
> Wouldn't this now set dom0less_enhanced unconditionally?
> 
>>        if ( vcpu_create(d, 0) == NULL )
>>          return -ENOMEM;
>> @@ -3379,7 +3383,7 @@ static int __init construct_domU(struct domain *d,
>>      if ( rc < 0 )
>>          return rc;
>>  -    if ( kinfo.dom0less_enhanced )
>> +    if ( kinfo.dom0less_xenstore )
>>      {
>>          ASSERT(hardware_domain);
>>          rc = alloc_xenstore_evtchn(d);
>> diff --git a/xen/arch/arm/include/asm/kernel.h b/xen/arch/arm/include/asm/kernel.h
>> index c4dc039b54..3d7fa94910 100644
>> --- a/xen/arch/arm/include/asm/kernel.h
>> +++ b/xen/arch/arm/include/asm/kernel.h
>> @@ -39,6 +39,9 @@ struct kernel_info {
>>      /* Enable PV drivers */
>>      bool dom0less_enhanced;
>>  +    /* Enable Xenstore */
>> +    bool dom0less_xenstore;
>> +
> 
> AFAIU, it is not possible to have *_xenstore = true and *_enhanced = false. I think it would be clearer if ``dom0less_enhanced`` is turned to an enum with 3 values:
> - None
> - NOXENSTORE/BASIC
> - FULLY_ENHANCED
> 
> If we want to be future proof, I would use a field 'flags' where non-zero means enhanced. Each bit would indicate which features of Xen is exposed.

I think that could be a good solution if we do it this way:
- define a dom0less feature field and have defines like the following:
#define DOM0LESS_GNTTAB
#define DOM0LESS_EVENTCHN
#define DOM0LESS_XENSTORE

- define dom0less enhanced as the right combination:
#define DOM0LESS_ENHANCED = (DOM0LESS_GNTTAB| DOM0LESS_EVENTCHN| DOM0LESS_XENSTORE)

This way we have a proper feature bitset and ENHANCED is properly defined as a combination of the 3 features.

What do you guys think ?

Cheers
Bertrand


> 
> Cheers,
> 
> -- 
> Julien Grall

Re: [PATCH v3 7/7] xen/arm: introduce new xen,enhanced property value

[Rahul Singh]

Hi Julien,

> On 1 Sep 2022, at 7:15 pm, Julien Grall <julien@xen.org> wrote:
> 
> Hi Rahul,
> 
> On 01/09/2022 10:13, Rahul Singh wrote:
>> Introduce a new "xen,enhanced" dom0less property value "no-xenstore" to
>> disable xenstore interface for dom0less guests.
>> Signed-off-by: Rahul Singh <rahul.singh@arm.com>
>> ---
>> Changes in v3:
>>  - new patch in this version
>> ---
>>  docs/misc/arm/device-tree/booting.txt |  4 ++++
>>  xen/arch/arm/domain_build.c           | 10 +++++++---
>>  xen/arch/arm/include/asm/kernel.h     |  3 +++
>>  3 files changed, 14 insertions(+), 3 deletions(-)
>> diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt
>> index edef98e6d1..87f57f8889 100644
>> --- a/docs/misc/arm/device-tree/booting.txt
>> +++ b/docs/misc/arm/device-tree/booting.txt
>> @@ -204,6 +204,10 @@ with the following properties:
>>      - "disabled"
>>      Xen PV interfaces are disabled.
>>  +    - no-xenstore
>> +    Xen PV interfaces, including grant-table will be enabled for the VM but
>> +    xenstore will be disabled for the VM.
> 
> NIT: I would drop one of the "for the VM" as it seems to be redundant.

Ack. 
> 
>> +
>>      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.
>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>> index 4b24261825..8dd9984225 100644
>> --- a/xen/arch/arm/domain_build.c
>> +++ b/xen/arch/arm/domain_build.c
>> @@ -3336,10 +3336,14 @@ static int __init construct_domU(struct domain *d,
>>           (rc == 0 && !strcmp(dom0less_enhanced, "enabled")) )
>>      {
>>          if ( hardware_domain )
>> -            kinfo.dom0less_enhanced = true;
>> +            kinfo.dom0less_xenstore = true;
>>          else
>> -            panic("Tried to use xen,enhanced without dom0\n");
>> +            panic("Tried to use xen,enhanced without dom0 without no-xenstore\n");
> 
> This is a bit hard to parse. How about:
> 
> "At the moment, Xenstore support requires dom0 to be present"

Ack. 
> 
>>      }
>> +    else if ( rc == 0 && !strcmp(dom0less_enhanced, "no-xenstore") )
>> +        kinfo.dom0less_xenstore = false;
>> +
>> +    kinfo.dom0less_enhanced = true;
> 
> Wouldn't this now set dom0less_enhanced unconditionally?

Yes , I will fix this in next version.
 

Regards,
Rahul

Re: [PATCH v3 7/7] xen/arm: introduce new xen,enhanced property value

[Julien Grall]

Hi Bertrand,

On 02/09/2022 15:51, Bertrand Marquis wrote:
>> On 1 Sep 2022, at 19:15, Julien Grall <julien@xen.org> wrote:
>> AFAIU, it is not possible to have *_xenstore = true and *_enhanced = false. I think it would be clearer if ``dom0less_enhanced`` is turned to an enum with 3 values:
>> - None
>> - NOXENSTORE/BASIC
>> - FULLY_ENHANCED
>>
>> If we want to be future proof, I would use a field 'flags' where non-zero means enhanced. Each bit would indicate which features of Xen is exposed.
> 
> I think that could be a good solution if we do it this way:
> - define a dom0less feature field and have defines like the following:
> #define DOM0LESS_GNTTAB
> #define DOM0LESS_EVENTCHN
> #define DOM0LESS_XENSTORE >
> - define dom0less enhanced as the right combination:
> #define DOM0LESS_ENHANCED = (DOM0LESS_GNTTAB| DOM0LESS_EVENTCHN| DOM0LESS_XENSTORE)

I would rather introduce DOM0LESS_ENHANCED_BASIC (or similar) instead of 
defining a bit for gnttab and evtchn. This will avoid the question of 
why we are introducing bits for both features but not the hypercall...

As this is an internal interface, it would be easier to modify afterwards.

Cheers,

-- 
Julien Grall

Re: [PATCH v3 6/7] xen/arm: introduce xen-evtchn dom0less property

[Rahul Singh]

Hi Julien,

> On 1 Sep 2022, at 7:07 pm, Julien Grall <julien@xen.org> wrote:
> 
> Hi Rahul,
> 
> On 01/09/2022 10:13, Rahul Singh wrote:
>> Introduce a new sub-node under /chosen node to establish static event
>> channel communication between domains on dom0less systems.
>> An event channel will be created beforehand to allow the domains to
>> send notifications to each other.
>> Signed-off-by: Rahul Singh <rahul.singh@arm.com>
>> ---
>> Changes in v3:
>>  - use device-tree used_by to find the domain id of the evtchn node.
>>  - add new static_evtchn_create variable in struct dt_device_node to
>>    hold the information if evtchn is already created.
>>  - fix minor comments
>> Changes in v2:
>>  - no change
>> ---
>>  docs/misc/arm/device-tree/booting.txt |  64 ++++++++++++-
>>  xen/arch/arm/domain_build.c           | 128 ++++++++++++++++++++++++++
>>  xen/arch/arm/include/asm/setup.h      |   1 +
>>  xen/arch/arm/setup.c                  |   2 +
>>  xen/include/xen/device_tree.h         |  13 +++
>>  5 files changed, 207 insertions(+), 1 deletion(-)
>> diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt
>> index 98253414b8..edef98e6d1 100644
>> --- a/docs/misc/arm/device-tree/booting.txt
>> +++ b/docs/misc/arm/device-tree/booting.txt
>> @@ -212,7 +212,7 @@ with the following properties:
>>      enable only selected interfaces.
>>    Under the "xen,domain" compatible node, one or more sub-nodes are present
>> -for the DomU kernel and ramdisk.
>> +for the DomU kernel, ramdisk and static event channel.
>>    The kernel sub-node has the following properties:
>>  @@ -254,11 +254,44 @@ The ramdisk sub-node has the following properties:
>>      property because it will be created by the UEFI stub on boot.
>>      This option is needed only when UEFI boot is used.
>>  +The static event channel sub-node has the following properties:
>> +
>> +- compatible
>> +
>> +    "xen,evtchn"
>> +
>> +- xen,evtchn
>> +
>> +    The property is tuples of two numbers
>> +    (local-evtchn link-to-foreign-evtchn) where:
>> +
>> +    local-evtchn is an integer value that will be used to allocate local port
>> +    for a domain to send and receive event notifications to/from the remote
>> +    domain. Maximum supported value is 2^17 for FIFO ABI and 4096 for 2L ABI.
>> +    It is recommended to use low event channel IDs.
>> +
>> +    link-to-foreign-evtchn is a single phandle to a remote evtchn to which
>> +    local-evtchn will be connected.
>>    Example
>>  =======
>>    chosen {
>> +
>> +    module@0 {
>> +        compatible = "multiboot,kernel", "multiboot,module";
>> +        xen,uefi-binary = "...";
>> +        bootargs = "...";
>> +
>> +    };
> 
> NIT: Describing this node in the example seems unnecessary.

Ack. I will remove this.
> 
>> +
>> +    /* one sub-node per local event channel */
>> +    ec1: evtchn@1 {
>> +         compatible = "xen,evtchn-v1";
>> +         /* local-evtchn link-to-foreign-evtchn */
>> +         xen,evtchn = <0xa &ec2>;
>> +    };
>> +
> 
> Here you provide an example for dom0. But the position where you describe the binding suggests this binding only exists for domUs.
> 
> Either we duplicate the binding or we re-order the documentation to have common binding in a single place. My preference would be the latter.
> 

Ack. 
>>      domU1 {
>>          compatible = "xen,domain";
>>          #address-cells = <0x2>;
>> @@ -277,6 +310,23 @@ chosen {
>>              compatible = "multiboot,ramdisk", "multiboot,module";
>>              reg = <0x0 0x4b000000 0xffffff>;
>>          };
>> +
>> +        /* one sub-node per local event channel */
>> +        ec2: evtchn@2 {
>> +            compatible = "xen,evtchn-v1";
>> +            /* local-evtchn link-to-foreign-evtchn */
>> +            xen,evtchn = <0xa &ec1>;
>> +        };
>> +
>> +        ec3: evtchn@3 {
>> +            compatible = "xen,evtchn-v1";
>> +            xen,evtchn = <0xb &ec5>;
>> +        };
>> +
>> +        ec4: evtchn@4 {
>> +            compatible = "xen,evtchn-v1";
>> +            xen,evtchn = <0xc &ec6>;
>> +        };
>>      };
>>        domU2 {
>> @@ -296,6 +346,18 @@ chosen {
>>              compatible = "multiboot,ramdisk", "multiboot,module";
>>              reg = <0x0 0x4d000000 0xffffff>;
>>          };
>> +
>> +        /* one sub-node per local event channel */
>> +        ec5: evtchn@5 {
>> +            compatible = "xen,evtchn-v1";
>> +            /* local-evtchn link-to-foreign-evtchn */
>> +            xen,evtchn = <0xb &ec3>;
>> +        };
>> +
>> +        ec6: evtchn@6 {
>> +            compatible = "xen,evtchn-v1";
>> +            xen,evtchn = <0xd &ec4>;
>> +        };
>>      };
>>  };
>>  diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>> index 707e247f6a..4b24261825 100644
>> --- a/xen/arch/arm/domain_build.c
>> +++ b/xen/arch/arm/domain_build.c
>> @@ -33,6 +33,8 @@
>>  #include <xen/grant_table.h>
>>  #include <xen/serial.h>
>>  +#define STATIC_EVTCHN_NODE_SIZE_CELLS 2
>> +
>>  static unsigned int __initdata opt_dom0_max_vcpus;
>>  integer_param("dom0_max_vcpus", opt_dom0_max_vcpus);
>>  @@ -3052,6 +3054,131 @@ void __init evtchn_allocate(struct domain *d)
>>      d->arch.hvm.params[HVM_PARAM_CALLBACK_IRQ] = val;
>>  }
>>  +static int __init get_evtchn_dt_property(const struct dt_device_node *np,
>> +                                         uint32_t *port, uint32_t *phandle)
>> +{
>> +    const __be32 *prop = NULL;
>> +    uint32_t len;
>> +
>> +    prop = dt_get_property(np, "xen,evtchn", &len);
>> +    if ( !prop )
>> +    {
>> +        printk(XENLOG_ERR "xen,evtchn property should not be empty.\n");
>> +        return -EINVAL;
>> +    }
>> +
>> +    if ( !len || len < dt_cells_to_size(STATIC_EVTCHN_NODE_SIZE_CELLS) )
>> +    {
>> +        printk(XENLOG_ERR "xen,evtchn property value is not valid.\n");
>> +        return -EINVAL;
>> +    }
>> +
>> +    *port = dt_next_cell(1, &prop);
>> +    *phandle = dt_next_cell(1, &prop);
>> +
>> +    return 0;
>> +}
>> +
>> +static int __init alloc_domain_evtchn(struct dt_device_node *node)
>> +{
>> +    int rc;
>> +    uint32_t domU1_port, domU2_port, remote_phandle;
>> +    struct dt_device_node *remote_node;
>> +    struct evtchn_alloc_unbound alloc_unbound;
>> +    struct evtchn_bind_interdomain bind_interdomain;
>> +    struct domain *d1 = NULL, *d2 = NULL;
>> +
>> +    if ( dt_device_static_evtchn_created(node) )
> 
> I think this deserve a comment explain why the node would be created. I.e it would happen if the other side was created first. I will comment about dt_device_static_evtchn_created() futher down.

I will modify as below:

 /*                                                                          
  * Event channel is already created while parsing the other side of         
  * evtchn node.                                                             
  */                                                                         
   if ( dt_device_static_evtchn_created(node) )                                
       return 0;
> 
>> +        return 0;
>> +
>> +    rc = get_evtchn_dt_property(node, &domU1_port, &remote_phandle);
>> +    if ( rc )
>> +        return rc;
>> +
>> +    remote_node = dt_find_node_by_phandle(remote_phandle);
>> +    if ( !remote_node )
>> +    {
>> +        printk(XENLOG_ERR
>> +                "evtchn: could not find remote evtchn phandle\n");
>> +        return -EINVAL;
>> +    }
>> +
>> +    rc = get_evtchn_dt_property(remote_node, &domU2_port, &remote_phandle);
>> +    if ( rc )
>> +        return rc;
>> +
>> +    if ( node->phandle != remote_phandle )
>> +    {
>> +        printk(XENLOG_ERR "xen,evtchn property is not setup correctly.\n");
>> +        return -EINVAL;
>> +    }
>> +
>> +    d1 = get_domain_by_id(dt_get_parent(node)->used_by);
>> +    d2 = get_domain_by_id(dt_get_parent(remote_node)->used_by);
> 
> I think dt_get_parent() could return NULL (i.e. for the root). So I think you want to check that at least remote_node() has a parent. For convenience, this check could be done in

Ack. 
> 
>> +
>> +    if ( !d1 || !d2 )
>> +    {
>> +        printk(XENLOG_ERR "evtchn: could not find domains\n" );
>> +        return -EINVAL;
>> +    }
>> +
>> +    alloc_unbound.dom = d1->domain_id;
>> +    alloc_unbound.remote_dom = d2->domain_id;
>> +
>> +    rc = evtchn_alloc_unbound(&alloc_unbound, domU1_port);
>> +    if ( rc < 0 )
>> +    {
>> +        printk(XENLOG_ERR
>> +                "evtchn_alloc_unbound() failure (Error %d) \n", rc);
>> +        return rc;
>> +    }
>> +
>> +    bind_interdomain.remote_dom  = d1->domain_id;
>> +    bind_interdomain.remote_port = domU1_port;
>> +
>> +    rc = evtchn_bind_interdomain(&bind_interdomain, d2, domU2_port);
>> +    if ( rc < 0 )
>> +    {
>> +        printk(XENLOG_ERR
>> +                "evtchn_bind_interdomain() failure (Error %d) \n", rc);
>> +        return rc;
>> +    }
>> +
>> +    dt_device_set_static_evtchn_created(node);
>> +    dt_device_set_static_evtchn_created(remote_node);
>> +
>> +    return 0;
>> +}
>> +
>> +void __init process_static_evtchn_node(struct dt_device_node *node)
> 
> This is missing a prototype. So I guess this wants to be static?
> 
> That said, I think it would make more sense to fold process_static_evtchn_node() in alloc_domain_evtchn() or alloc_static-evtchn().

Ack. 
> 
>> +{
>> +    if ( dt_device_is_compatible(node, "xen,evtchn-v1") )
>> +    {
>> +        if ( alloc_domain_evtchn(node) != 0 )
>> +            panic("Could not set up domains evtchn\n");
>> +    }
>> +}
>> +
>> +void __init alloc_static_evtchn(void)
>> +{
>> +    struct dt_device_node *node, *evtchn_node;
>> +    struct dt_device_node *chosen = dt_find_node_by_path("/chosen");
>> +
>> +    BUG_ON(chosen == NULL);
>> +
>> +    if ( hardware_domain )
>> +        dt_device_set_used_by(chosen, hardware_domain->domain_id);
>> +
>> +    dt_for_each_child_node(chosen, node)
>> +    {
>> +        if ( hardware_domain )
>> +            process_static_evtchn_node(node);
>> +
>> +        dt_for_each_child_node(node, evtchn_node)
>> +            process_static_evtchn_node(evtchn_node);
>> +    }
>> +}
>> +
>>  static void __init find_gnttab_region(struct domain *d,
>>                                        struct kernel_info *kinfo)
>>  {
>> @@ -3364,6 +3491,7 @@ void __init create_domUs(void)
>>              panic("Error creating domain %s\n", dt_node_name(node));
>>            d->is_console = true;
>> +        dt_device_set_used_by(node, d->domain_id);
>>            if ( construct_domU(d, node) != 0 )
>>              panic("Could not set up domain %s\n", dt_node_name(node));
>> diff --git a/xen/arch/arm/include/asm/setup.h b/xen/arch/arm/include/asm/setup.h
>> index 5815ccf8c5..5ee28b270f 100644
>> --- a/xen/arch/arm/include/asm/setup.h
>> +++ b/xen/arch/arm/include/asm/setup.h
>> @@ -106,6 +106,7 @@ int acpi_make_efi_nodes(void *fdt, struct membank tbl_add[]);
>>    void create_domUs(void);
>>  void create_dom0(void);
>> +void alloc_static_evtchn(void);
>>    void discard_initial_modules(void);
>>  void fw_unreserved_regions(paddr_t s, paddr_t e,
>> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
>> index 6e0398f3f6..cf15d359d2 100644
>> --- a/xen/arch/arm/setup.c
>> +++ b/xen/arch/arm/setup.c
>> @@ -1077,6 +1077,8 @@ void __init start_xen(unsigned long boot_phys_offset,
>>      if ( acpi_disabled )
>>          create_domUs();
>>  +    alloc_static_evtchn();
>> +
>>      /*
>>       * This needs to be called **before** heap_init_late() so modules
>>       * will be scrubbed (unless suppressed).
>> diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
>> index 430a1ef445..5579c875d2 100644
>> --- a/xen/include/xen/device_tree.h
>> +++ b/xen/include/xen/device_tree.h
>> @@ -82,6 +82,7 @@ struct dt_device_node {
>>      dt_phandle phandle;
>>      char *full_name;
>>      domid_t used_by; /* By default it's used by dom0 */
>> +    bool_t static_evtchn_created;
> 
> I can see why you want to add the boolean in dt_device_node. However, I dislike this approach because this feels an abuse of dt_device_node and the field is only used at boot.
> 
> So this seems to be a bit of a waste to include it in the structure (even if we are re-using padding today).
> 
> I don't have a solution that is has trivial as this approach. However, at minimum we should document this is a HACK and should be remove if we need space in the structure.
 
Ok. I will add the comment that this is HACK.

….
     /* IOMMU specific fields */
     bool is_protected;
+
+    /* HACK: Remove this if there is a need of space */
+    bool_t static_evtchn_created;
+
     /*
      * The main purpose of this list is to link the structure in the list
      * of devices assigned to domain.
….

Regards,
Rahul

Re: [PATCH v3 7/7] xen/arm: introduce new xen,enhanced property value

[Rahul Singh]

Hi Julien,

> On 2 Sep 2022, at 4:05 pm, Julien Grall <julien@xen.org> wrote:
> 
> Hi Bertrand,
> 
> On 02/09/2022 15:51, Bertrand Marquis wrote:
>>> On 1 Sep 2022, at 19:15, Julien Grall <julien@xen.org> wrote:
>>> AFAIU, it is not possible to have *_xenstore = true and *_enhanced = false. I think it would be clearer if ``dom0less_enhanced`` is turned to an enum with 3 values:
>>> - None
>>> - NOXENSTORE/BASIC
>>> - FULLY_ENHANCED
>>> 
>>> If we want to be future proof, I would use a field 'flags' where non-zero means enhanced. Each bit would indicate which features of Xen is exposed.
>> I think that could be a good solution if we do it this way:
>> - define a dom0less feature field and have defines like the following:
>> #define DOM0LESS_GNTTAB
>> #define DOM0LESS_EVENTCHN
>> #define DOM0LESS_XENSTORE >
>> - define dom0less enhanced as the right combination:
>> #define DOM0LESS_ENHANCED = (DOM0LESS_GNTTAB| DOM0LESS_EVENTCHN| DOM0LESS_XENSTORE)
> 
> I would rather introduce DOM0LESS_ENHANCED_BASIC (or similar) instead of defining a bit for gnttab and evtchn. This will avoid the question of why we are introducing bits for both features but not the hypercall...
> 
> As this is an internal interface, it would be easier to modify afterwards.

How about this?

/*                                                                              
 * List of possible features for dom0less domUs                                 
 *                                                                              
 * DOM0LESS_ENHANCED_BASIC: Xen PV interfaces, including grant-table and        
 *                                                          evtchn, will be enabled for the VM.                 
 * DOM0LESS_XENSTORE:              Xenstore will be enabled for the VM.                
 * DOM0LESS_ENHANCED:              Xen PV interfaces, including grant-table xenstore   
 *                                                          and evtchn, will be enabled for the VM.             
 */                                                                             
#define DOM0LESS_ENHANCED_BASIC BIT(0, UL)                                      
#define DOM0LESS_XENSTORE       BIT(1, UL)                                      
#define DOM0LESS_ENHANCED       (DOM0LESS_ENHANCED_BASIC | DOM0LESS_XENSTORE)
 
Regards,
Rahul

Re: [PATCH v3 7/7] xen/arm: introduce new xen,enhanced property value

[Julien Grall]

On 02/09/2022 16:54, Rahul Singh wrote:
> Hi Julien,

Hi Rahul,

>> On 2 Sep 2022, at 4:05 pm, Julien Grall <julien@xen.org> wrote:
>>
>> Hi Bertrand,
>>
>> On 02/09/2022 15:51, Bertrand Marquis wrote:
>>>> On 1 Sep 2022, at 19:15, Julien Grall <julien@xen.org> wrote:
>>>> AFAIU, it is not possible to have *_xenstore = true and *_enhanced = false. I think it would be clearer if ``dom0less_enhanced`` is turned to an enum with 3 values:
>>>> - None
>>>> - NOXENSTORE/BASIC
>>>> - FULLY_ENHANCED
>>>>
>>>> If we want to be future proof, I would use a field 'flags' where non-zero means enhanced. Each bit would indicate which features of Xen is exposed.
>>> I think that could be a good solution if we do it this way:
>>> - define a dom0less feature field and have defines like the following:
>>> #define DOM0LESS_GNTTAB
>>> #define DOM0LESS_EVENTCHN
>>> #define DOM0LESS_XENSTORE >
>>> - define dom0less enhanced as the right combination:
>>> #define DOM0LESS_ENHANCED = (DOM0LESS_GNTTAB| DOM0LESS_EVENTCHN| DOM0LESS_XENSTORE)
>>
>> I would rather introduce DOM0LESS_ENHANCED_BASIC (or similar) instead of defining a bit for gnttab and evtchn. This will avoid the question of why we are introducing bits for both features but not the hypercall...
>>
>> As this is an internal interface, it would be easier to modify afterwards.
> 
> How about this?
> 
> /*
>   * List of possible features for dom0less domUs
>   *
>   * DOM0LESS_ENHANCED_BASIC: Xen PV interfaces, including grant-table and
>   *                                                          evtchn, will be enabled for the VM.

Technically, the guest can already use the grant-table and evtchn 
interfaces. This also reads quite odd to me because "including" doesn't 
tell what's not enabled. So one could assume Xenstored is also enabled. 
In fact the wording for ``DOM0LESS_ENHANCED`` is what makes it a lot 
more confusing.

So I would suggest the following wording:

"Notify the OS it is running on top of Xen. All the default features but 
Xenstore will be available. Note that an OS *must* not rely on the 
availability of Xen features if this is not set.
"

The wording can be updated once we properly disable event channel/grant 
table when the flag is not set.

>   * DOM0LESS_XENSTORE:              Xenstore will be enabled for the VM.

I would make clear this can't be used without the first one.

>   * DOM0LESS_ENHANCED:              Xen PV interfaces, including grant-table xenstore >   *                                                          and 
evtchn, will be enabled for the VM.

See above about "PV interfaces". So I would suggest to reword to:

"Notify the OS it is running on top of Xen. All the default features 
(including Xenstore) will be available".

>   */
> #define DOM0LESS_ENHANCED_BASIC BIT(0, UL)
> #define DOM0LESS_XENSTORE       BIT(1, UL)

Based on the comment above, I would consider to define DOM0LESS_XENSTORE 
as bit 0 and 1 set.

> #define DOM0LESS_ENHANCED       (DOM0LESS_ENHANCED_BASIC | DOM0LESS_XENSTORE)

Cheers,

-- 
Julien Grall

Re: [PATCH v3 7/7] xen/arm: introduce new xen,enhanced property value

[Rahul Singh]

Hi Julien,

> On 2 Sep 2022, at 5:20 pm, Julien Grall <julien@xen.org> wrote:
> 
> 
> 
> On 02/09/2022 16:54, Rahul Singh wrote:
>> Hi Julien,
> 
> Hi Rahul,
> 
>>> On 2 Sep 2022, at 4:05 pm, Julien Grall <julien@xen.org> wrote:
>>> 
>>> Hi Bertrand,
>>> 
>>> On 02/09/2022 15:51, Bertrand Marquis wrote:
>>>>> On 1 Sep 2022, at 19:15, Julien Grall <julien@xen.org> wrote:
>>>>> AFAIU, it is not possible to have *_xenstore = true and *_enhanced = false. I think it would be clearer if ``dom0less_enhanced`` is turned to an enum with 3 values:
>>>>> - None
>>>>> - NOXENSTORE/BASIC
>>>>> - FULLY_ENHANCED
>>>>> 
>>>>> If we want to be future proof, I would use a field 'flags' where non-zero means enhanced. Each bit would indicate which features of Xen is exposed.
>>>> I think that could be a good solution if we do it this way:
>>>> - define a dom0less feature field and have defines like the following:
>>>> #define DOM0LESS_GNTTAB
>>>> #define DOM0LESS_EVENTCHN
>>>> #define DOM0LESS_XENSTORE >
>>>> - define dom0less enhanced as the right combination:
>>>> #define DOM0LESS_ENHANCED = (DOM0LESS_GNTTAB| DOM0LESS_EVENTCHN| DOM0LESS_XENSTORE)
>>> 
>>> I would rather introduce DOM0LESS_ENHANCED_BASIC (or similar) instead of defining a bit for gnttab and evtchn. This will avoid the question of why we are introducing bits for both features but not the hypercall...
>>> 
>>> As this is an internal interface, it would be easier to modify afterwards.
>> How about this?
>> /*
>>  * List of possible features for dom0less domUs
>>  *
>>  * DOM0LESS_ENHANCED_BASIC: Xen PV interfaces, including grant-table and
>>  *                                                          evtchn, will be enabled for the VM.
> 
> Technically, the guest can already use the grant-table and evtchn interfaces. This also reads quite odd to me because "including" doesn't tell what's not enabled. So one could assume Xenstored is also enabled. In fact the wording for ``DOM0LESS_ENHANCED`` is what makes it a lot more confusing.
> 
> So I would suggest the following wording:
> 
> "Notify the OS it is running on top of Xen. All the default features but Xenstore will be available. Note that an OS *must* not rely on the availability of Xen features if this is not set.
> "
> 
> The wording can be updated once we properly disable event channel/grant table when the flag is not set.
> 
>>  * DOM0LESS_XENSTORE:              Xenstore will be enabled for the VM.
> 
> I would make clear this can't be used without the first one.
> 
>>  * DOM0LESS_ENHANCED:              Xen PV interfaces, including grant-table xenstore >   *                                                          and 
> evtchn, will be enabled for the VM.
> 
> See above about "PV interfaces". So I would suggest to reword to:
> 
> "Notify the OS it is running on top of Xen. All the default features (including Xenstore) will be available".
> 
>>  */
>> #define DOM0LESS_ENHANCED_BASIC BIT(0, UL)
>> #define DOM0LESS_XENSTORE       BIT(1, UL)
> 
> Based on the comment above, I would consider to define DOM0LESS_XENSTORE as bit 0 and 1 set.
> 
>> #define DOM0LESS_ENHANCED       (DOM0LESS_ENHANCED_BASIC | DOM0LESS_XENSTORE)
 
Bertrand and I discussed this again we came to the conclusion that DOM0LESS_ENHANCED_BASIC is not
the suitable name as this makes the code unclear and does not correspond to DT settings. We propose this
please let me know your thoughts.  
 
/*                                                                              
 * List of possible features for dom0less domUs                                 
 *                                                                              
 * DOM0LESS_XENSTORE:		Xenstore will be enabled for the VM. This feature   
 *                                              	can't be enabled without the DOM0LESS_ENHANCED.     
 * DOM0LESS_ENHANCED:       	Notify the OS it is running on top of Xen. All the  
 *                          				default features (including Xenstore) will be       
 *                          				available. Note that an OS *must* not rely on the   
 *                          				availability of Xen features if this is not set.    
 */                                                                             
#define DOM0LESS_XENSTORE       BIT(0, UL)                                      
#define DOM0LESS_ENHANCED       BIT(1,UL)                                       
#define DOM0LESS_ENHANCED_FULL  (DOM0LESS_XENSTORE | DOM0LESS_ENHANCED)

Regards,
Rahul

Re: [PATCH v3 7/7] xen/arm: introduce new xen,enhanced property value

[Julien Grall]

On 05/09/2022 12:12, Rahul Singh wrote:
> Hi Julien,

Hi Rahul,

> 
>> On 2 Sep 2022, at 5:20 pm, Julien Grall <julien@xen.org> wrote:
>>
>>
>>
>> On 02/09/2022 16:54, Rahul Singh wrote:
>>> Hi Julien,
>>
>> Hi Rahul,
>>
>>>> On 2 Sep 2022, at 4:05 pm, Julien Grall <julien@xen.org> wrote:
>>>>
>>>> Hi Bertrand,
>>>>
>>>> On 02/09/2022 15:51, Bertrand Marquis wrote:
>>>>>> On 1 Sep 2022, at 19:15, Julien Grall <julien@xen.org> wrote:
>>>>>> AFAIU, it is not possible to have *_xenstore = true and *_enhanced = false. I think it would be clearer if ``dom0less_enhanced`` is turned to an enum with 3 values:
>>>>>> - None
>>>>>> - NOXENSTORE/BASIC
>>>>>> - FULLY_ENHANCED
>>>>>>
>>>>>> If we want to be future proof, I would use a field 'flags' where non-zero means enhanced. Each bit would indicate which features of Xen is exposed.
>>>>> I think that could be a good solution if we do it this way:
>>>>> - define a dom0less feature field and have defines like the following:
>>>>> #define DOM0LESS_GNTTAB
>>>>> #define DOM0LESS_EVENTCHN
>>>>> #define DOM0LESS_XENSTORE >
>>>>> - define dom0less enhanced as the right combination:
>>>>> #define DOM0LESS_ENHANCED = (DOM0LESS_GNTTAB| DOM0LESS_EVENTCHN| DOM0LESS_XENSTORE)
>>>>
>>>> I would rather introduce DOM0LESS_ENHANCED_BASIC (or similar) instead of defining a bit for gnttab and evtchn. This will avoid the question of why we are introducing bits for both features but not the hypercall...
>>>>
>>>> As this is an internal interface, it would be easier to modify afterwards.
>>> How about this?
>>> /*
>>>   * List of possible features for dom0less domUs
>>>   *
>>>   * DOM0LESS_ENHANCED_BASIC: Xen PV interfaces, including grant-table and
>>>   *                                                          evtchn, will be enabled for the VM.
>>
>> Technically, the guest can already use the grant-table and evtchn interfaces. This also reads quite odd to me because "including" doesn't tell what's not enabled. So one could assume Xenstored is also enabled. In fact the wording for ``DOM0LESS_ENHANCED`` is what makes it a lot more confusing.
>>
>> So I would suggest the following wording:
>>
>> "Notify the OS it is running on top of Xen. All the default features but Xenstore will be available. Note that an OS *must* not rely on the availability of Xen features if this is not set.
>> "
>>
>> The wording can be updated once we properly disable event channel/grant table when the flag is not set.
>>
>>>   * DOM0LESS_XENSTORE:              Xenstore will be enabled for the VM.
>>
>> I would make clear this can't be used without the first one.
>>
>>>   * DOM0LESS_ENHANCED:              Xen PV interfaces, including grant-table xenstore >   *                                                          and
>> evtchn, will be enabled for the VM.
>>
>> See above about "PV interfaces". So I would suggest to reword to:
>>
>> "Notify the OS it is running on top of Xen. All the default features (including Xenstore) will be available".
>>
>>>   */
>>> #define DOM0LESS_ENHANCED_BASIC BIT(0, UL)
>>> #define DOM0LESS_XENSTORE       BIT(1, UL)
>>
>> Based on the comment above, I would consider to define DOM0LESS_XENSTORE as bit 0 and 1 set.
>>
>>> #define DOM0LESS_ENHANCED       (DOM0LESS_ENHANCED_BASIC | DOM0LESS_XENSTORE)
>   
> Bertrand and I discussed this again we came to the conclusion that DOM0LESS_ENHANCED_BASIC is not
> the suitable name as this makes the code unclear and does not correspond to DT settings. We propose this
> please let me know your thoughts.

To me the default of "enhanced" should be all Xen features. Anything 
else should be consider as reduced/basic/minimum. Hence why I still 
think we need to add it in the name even if this is not what we expose 
in the DT. In fact...
>   
> /*
>   * List of possible features for dom0less domUs
>   *
>   * DOM0LESS_XENSTORE:		Xenstore will be enabled for the VM. This feature
>   *                                              	can't be enabled without the DOM0LESS_ENHANCED.
>   * DOM0LESS_ENHANCED:       	Notify the OS it is running on top of Xen. All the
>   *                          				default features (including Xenstore) will be
>   *                          				available. Note that an OS *must* not rely on the
>   *                          				availability of Xen features if this is not set.

... what you wrote here match what I wrote above. So it is not clear to 
me what's the point of having a flag DOM0LESS_XENSTORE.

Cheers,

-- 
Julien Grall

Re: [PATCH v3 7/7] xen/arm: introduce new xen,enhanced property value

[Bertrand Marquis]

Hi Julien,

> On 5 Sep 2022, at 12:43, Julien Grall <julien@xen.org> wrote:
> 
> 
> 
> On 05/09/2022 12:12, Rahul Singh wrote:
>> Hi Julien,
> 
> Hi Rahul,
> 
>>> On 2 Sep 2022, at 5:20 pm, Julien Grall <julien@xen.org> wrote:
>>> 
>>> 
>>> 
>>> On 02/09/2022 16:54, Rahul Singh wrote:
>>>> Hi Julien,
>>> 
>>> Hi Rahul,
>>> 
>>>>> On 2 Sep 2022, at 4:05 pm, Julien Grall <julien@xen.org> wrote:
>>>>> 
>>>>> Hi Bertrand,
>>>>> 
>>>>> On 02/09/2022 15:51, Bertrand Marquis wrote:
>>>>>>> On 1 Sep 2022, at 19:15, Julien Grall <julien@xen.org> wrote:
>>>>>>> AFAIU, it is not possible to have *_xenstore = true and *_enhanced = false. I think it would be clearer if ``dom0less_enhanced`` is turned to an enum with 3 values:
>>>>>>> - None
>>>>>>> - NOXENSTORE/BASIC
>>>>>>> - FULLY_ENHANCED
>>>>>>> 
>>>>>>> If we want to be future proof, I would use a field 'flags' where non-zero means enhanced. Each bit would indicate which features of Xen is exposed.
>>>>>> I think that could be a good solution if we do it this way:
>>>>>> - define a dom0less feature field and have defines like the following:
>>>>>> #define DOM0LESS_GNTTAB
>>>>>> #define DOM0LESS_EVENTCHN
>>>>>> #define DOM0LESS_XENSTORE >
>>>>>> - define dom0less enhanced as the right combination:
>>>>>> #define DOM0LESS_ENHANCED = (DOM0LESS_GNTTAB| DOM0LESS_EVENTCHN| DOM0LESS_XENSTORE)
>>>>> 
>>>>> I would rather introduce DOM0LESS_ENHANCED_BASIC (or similar) instead of defining a bit for gnttab and evtchn. This will avoid the question of why we are introducing bits for both features but not the hypercall...
>>>>> 
>>>>> As this is an internal interface, it would be easier to modify afterwards.
>>>> How about this?
>>>> /*
>>>>  * List of possible features for dom0less domUs
>>>>  *
>>>>  * DOM0LESS_ENHANCED_BASIC: Xen PV interfaces, including grant-table and
>>>>  *                                                          evtchn, will be enabled for the VM.
>>> 
>>> Technically, the guest can already use the grant-table and evtchn interfaces. This also reads quite odd to me because "including" doesn't tell what's not enabled. So one could assume Xenstored is also enabled. In fact the wording for ``DOM0LESS_ENHANCED`` is what makes it a lot more confusing.
>>> 
>>> So I would suggest the following wording:
>>> 
>>> "Notify the OS it is running on top of Xen. All the default features but Xenstore will be available. Note that an OS *must* not rely on the availability of Xen features if this is not set.
>>> "
>>> 
>>> The wording can be updated once we properly disable event channel/grant table when the flag is not set.
>>> 
>>>>  * DOM0LESS_XENSTORE:              Xenstore will be enabled for the VM.
>>> 
>>> I would make clear this can't be used without the first one.
>>> 
>>>>  * DOM0LESS_ENHANCED:              Xen PV interfaces, including grant-table xenstore >   *                                                          and
>>> evtchn, will be enabled for the VM.
>>> 
>>> See above about "PV interfaces". So I would suggest to reword to:
>>> 
>>> "Notify the OS it is running on top of Xen. All the default features (including Xenstore) will be available".
>>> 
>>>>  */
>>>> #define DOM0LESS_ENHANCED_BASIC BIT(0, UL)
>>>> #define DOM0LESS_XENSTORE       BIT(1, UL)
>>> 
>>> Based on the comment above, I would consider to define DOM0LESS_XENSTORE as bit 0 and 1 set.
>>> 
>>>> #define DOM0LESS_ENHANCED       (DOM0LESS_ENHANCED_BASIC | DOM0LESS_XENSTORE)
>>  Bertrand and I discussed this again we came to the conclusion that DOM0LESS_ENHANCED_BASIC is not
>> the suitable name as this makes the code unclear and does not correspond to DT settings. We propose this
>> please let me know your thoughts.
> 
> To me the default of "enhanced" should be all Xen features. Anything else should be consider as reduced/basic/minimum. Hence why I still think we need to add it in the name even if this is not what we expose in the DT. In fact...
>>  /*
>>  * List of possible features for dom0less domUs
>>  *
>>  * DOM0LESS_XENSTORE:		Xenstore will be enabled for the VM. This feature
>>  *                                              	can't be enabled without the DOM0LESS_ENHANCED.
>>  * DOM0LESS_ENHANCED:       	Notify the OS it is running on top of Xen. All the
>>  *                          				default features (including Xenstore) will be
>>  *                          				available. Note that an OS *must* not rely on the
>>  *                          				availability of Xen features if this is not set.
> 
> ... what you wrote here match what I wrote above. So it is not clear to me what's the point of having a flag DOM0LESS_XENSTORE.

When we looked at the code with the solution using BASIC, it was really not easy to understand.
By the way the comment is wrong and correspond to what should be ENHANCED_FULL here
ENHANCED would be the base without Xenstore.

Bertrand

> 
> Cheers,
> 
> -- 
> Julien Grall

Re: [PATCH v3 7/7] xen/arm: introduce new xen,enhanced property value

[Julien Grall]

On 05/09/2022 12:54, Bertrand Marquis wrote:
> Hi Julien,
> 
>> On 5 Sep 2022, at 12:43, Julien Grall <julien@xen.org> wrote:
>>
>>
>>
>> On 05/09/2022 12:12, Rahul Singh wrote:
>>> Hi Julien,
>>
>> Hi Rahul,
>>
>>>> On 2 Sep 2022, at 5:20 pm, Julien Grall <julien@xen.org> wrote:
>>>>
>>>>
>>>>
>>>> On 02/09/2022 16:54, Rahul Singh wrote:
>>>>> Hi Julien,
>>>>
>>>> Hi Rahul,
>>>>
>>>>>> On 2 Sep 2022, at 4:05 pm, Julien Grall <julien@xen.org> wrote:
>>>>>>
>>>>>> Hi Bertrand,
>>>>>>
>>>>>> On 02/09/2022 15:51, Bertrand Marquis wrote:
>>>>>>>> On 1 Sep 2022, at 19:15, Julien Grall <julien@xen.org> wrote:
>>>>>>>> AFAIU, it is not possible to have *_xenstore = true and *_enhanced = false. I think it would be clearer if ``dom0less_enhanced`` is turned to an enum with 3 values:
>>>>>>>> - None
>>>>>>>> - NOXENSTORE/BASIC
>>>>>>>> - FULLY_ENHANCED
>>>>>>>>
>>>>>>>> If we want to be future proof, I would use a field 'flags' where non-zero means enhanced. Each bit would indicate which features of Xen is exposed.
>>>>>>> I think that could be a good solution if we do it this way:
>>>>>>> - define a dom0less feature field and have defines like the following:
>>>>>>> #define DOM0LESS_GNTTAB
>>>>>>> #define DOM0LESS_EVENTCHN
>>>>>>> #define DOM0LESS_XENSTORE >
>>>>>>> - define dom0less enhanced as the right combination:
>>>>>>> #define DOM0LESS_ENHANCED = (DOM0LESS_GNTTAB| DOM0LESS_EVENTCHN| DOM0LESS_XENSTORE)
>>>>>>
>>>>>> I would rather introduce DOM0LESS_ENHANCED_BASIC (or similar) instead of defining a bit for gnttab and evtchn. This will avoid the question of why we are introducing bits for both features but not the hypercall...
>>>>>>
>>>>>> As this is an internal interface, it would be easier to modify afterwards.
>>>>> How about this?
>>>>> /*
>>>>>   * List of possible features for dom0less domUs
>>>>>   *
>>>>>   * DOM0LESS_ENHANCED_BASIC: Xen PV interfaces, including grant-table and
>>>>>   *                                                          evtchn, will be enabled for the VM.
>>>>
>>>> Technically, the guest can already use the grant-table and evtchn interfaces. This also reads quite odd to me because "including" doesn't tell what's not enabled. So one could assume Xenstored is also enabled. In fact the wording for ``DOM0LESS_ENHANCED`` is what makes it a lot more confusing.
>>>>
>>>> So I would suggest the following wording:
>>>>
>>>> "Notify the OS it is running on top of Xen. All the default features but Xenstore will be available. Note that an OS *must* not rely on the availability of Xen features if this is not set.
>>>> "
>>>>
>>>> The wording can be updated once we properly disable event channel/grant table when the flag is not set.
>>>>
>>>>>   * DOM0LESS_XENSTORE:              Xenstore will be enabled for the VM.
>>>>
>>>> I would make clear this can't be used without the first one.
>>>>
>>>>>   * DOM0LESS_ENHANCED:              Xen PV interfaces, including grant-table xenstore >   *                                                          and
>>>> evtchn, will be enabled for the VM.
>>>>
>>>> See above about "PV interfaces". So I would suggest to reword to:
>>>>
>>>> "Notify the OS it is running on top of Xen. All the default features (including Xenstore) will be available".
>>>>
>>>>>   */
>>>>> #define DOM0LESS_ENHANCED_BASIC BIT(0, UL)
>>>>> #define DOM0LESS_XENSTORE       BIT(1, UL)
>>>>
>>>> Based on the comment above, I would consider to define DOM0LESS_XENSTORE as bit 0 and 1 set.
>>>>
>>>>> #define DOM0LESS_ENHANCED       (DOM0LESS_ENHANCED_BASIC | DOM0LESS_XENSTORE)
>>>   Bertrand and I discussed this again we came to the conclusion that DOM0LESS_ENHANCED_BASIC is not
>>> the suitable name as this makes the code unclear and does not correspond to DT settings. We propose this
>>> please let me know your thoughts.
>>
>> To me the default of "enhanced" should be all Xen features. Anything else should be consider as reduced/basic/minimum. Hence why I still think we need to add it in the name even if this is not what we expose in the DT. In fact...
>>>   /*
>>>   * List of possible features for dom0less domUs
>>>   *
>>>   * DOM0LESS_XENSTORE:		Xenstore will be enabled for the VM. This feature
>>>   *                                              	can't be enabled without the DOM0LESS_ENHANCED.
>>>   * DOM0LESS_ENHANCED:       	Notify the OS it is running on top of Xen. All the
>>>   *                          				default features (including Xenstore) will be
>>>   *                          				available. Note that an OS *must* not rely on the
>>>   *                          				availability of Xen features if this is not set.
>>
>> ... what you wrote here match what I wrote above. So it is not clear to me what's the point of having a flag DOM0LESS_XENSTORE.
> 
> When we looked at the code with the solution using BASIC, it was really not easy to understand.

I don't quite understand how this is different from ENHANCED, 
ENHANCED_FULL. In fact, without looking at the documentation, they mean 
exactly the same...

The difference between "BASIC" and "ENHANCED" is clear. You know that in 
one case, you would get less than the other.

> By the way the comment is wrong and correspond to what should be ENHANCED_FULL here
> ENHANCED would be the base without Xenstore.

Thanks for the confirmation. I am afraid, I am strongly against the 
terminology you proposed (see above why).

I think BASIC (or similar name) is better. But I am open to suggestion 
so long it is not "DOM0LESS_ENHANCED" vs "DOM0LESS_ENHANCED_FULL".

As an aside, I think it is more logical if you define DOM0LESS_XENSTORE 
as bit 1. But that's NIT at this point. What matters is the naming.

Cheers,

-- 
Julien Grall

Re: [PATCH v3 7/7] xen/arm: introduce new xen,enhanced property value

[Bertrand Marquis]

Hi Julien,

> On 5 Sep 2022, at 13:08, Julien Grall <julien@xen.org> wrote:
> 
> 
> 
> On 05/09/2022 12:54, Bertrand Marquis wrote:
>> Hi Julien,
>>> On 5 Sep 2022, at 12:43, Julien Grall <julien@xen.org> wrote:
>>> 
>>> 
>>> 
>>> On 05/09/2022 12:12, Rahul Singh wrote:
>>>> Hi Julien,
>>> 
>>> Hi Rahul,
>>> 
>>>>> On 2 Sep 2022, at 5:20 pm, Julien Grall <julien@xen.org> wrote:
>>>>> 
>>>>> 
>>>>> 
>>>>> On 02/09/2022 16:54, Rahul Singh wrote:
>>>>>> Hi Julien,
>>>>> 
>>>>> Hi Rahul,
>>>>> 
>>>>>>> On 2 Sep 2022, at 4:05 pm, Julien Grall <julien@xen.org> wrote:
>>>>>>> 
>>>>>>> Hi Bertrand,
>>>>>>> 
>>>>>>> On 02/09/2022 15:51, Bertrand Marquis wrote:
>>>>>>>>> On 1 Sep 2022, at 19:15, Julien Grall <julien@xen.org> wrote:
>>>>>>>>> AFAIU, it is not possible to have *_xenstore = true and *_enhanced = false. I think it would be clearer if ``dom0less_enhanced`` is turned to an enum with 3 values:
>>>>>>>>> - None
>>>>>>>>> - NOXENSTORE/BASIC
>>>>>>>>> - FULLY_ENHANCED
>>>>>>>>> 
>>>>>>>>> If we want to be future proof, I would use a field 'flags' where non-zero means enhanced. Each bit would indicate which features of Xen is exposed.
>>>>>>>> I think that could be a good solution if we do it this way:
>>>>>>>> - define a dom0less feature field and have defines like the following:
>>>>>>>> #define DOM0LESS_GNTTAB
>>>>>>>> #define DOM0LESS_EVENTCHN
>>>>>>>> #define DOM0LESS_XENSTORE >
>>>>>>>> - define dom0less enhanced as the right combination:
>>>>>>>> #define DOM0LESS_ENHANCED = (DOM0LESS_GNTTAB| DOM0LESS_EVENTCHN| DOM0LESS_XENSTORE)
>>>>>>> 
>>>>>>> I would rather introduce DOM0LESS_ENHANCED_BASIC (or similar) instead of defining a bit for gnttab and evtchn. This will avoid the question of why we are introducing bits for both features but not the hypercall...
>>>>>>> 
>>>>>>> As this is an internal interface, it would be easier to modify afterwards.
>>>>>> How about this?
>>>>>> /*
>>>>>>  * List of possible features for dom0less domUs
>>>>>>  *
>>>>>>  * DOM0LESS_ENHANCED_BASIC: Xen PV interfaces, including grant-table and
>>>>>>  *                                                          evtchn, will be enabled for the VM.
>>>>> 
>>>>> Technically, the guest can already use the grant-table and evtchn interfaces. This also reads quite odd to me because "including" doesn't tell what's not enabled. So one could assume Xenstored is also enabled. In fact the wording for ``DOM0LESS_ENHANCED`` is what makes it a lot more confusing.
>>>>> 
>>>>> So I would suggest the following wording:
>>>>> 
>>>>> "Notify the OS it is running on top of Xen. All the default features but Xenstore will be available. Note that an OS *must* not rely on the availability of Xen features if this is not set.
>>>>> "
>>>>> 
>>>>> The wording can be updated once we properly disable event channel/grant table when the flag is not set.
>>>>> 
>>>>>>  * DOM0LESS_XENSTORE:              Xenstore will be enabled for the VM.
>>>>> 
>>>>> I would make clear this can't be used without the first one.
>>>>> 
>>>>>>  * DOM0LESS_ENHANCED:              Xen PV interfaces, including grant-table xenstore >   *                                                          and
>>>>> evtchn, will be enabled for the VM.
>>>>> 
>>>>> See above about "PV interfaces". So I would suggest to reword to:
>>>>> 
>>>>> "Notify the OS it is running on top of Xen. All the default features (including Xenstore) will be available".
>>>>> 
>>>>>>  */
>>>>>> #define DOM0LESS_ENHANCED_BASIC BIT(0, UL)
>>>>>> #define DOM0LESS_XENSTORE       BIT(1, UL)
>>>>> 
>>>>> Based on the comment above, I would consider to define DOM0LESS_XENSTORE as bit 0 and 1 set.
>>>>> 
>>>>>> #define DOM0LESS_ENHANCED       (DOM0LESS_ENHANCED_BASIC | DOM0LESS_XENSTORE)
>>>>  Bertrand and I discussed this again we came to the conclusion that DOM0LESS_ENHANCED_BASIC is not
>>>> the suitable name as this makes the code unclear and does not correspond to DT settings. We propose this
>>>> please let me know your thoughts.
>>> 
>>> To me the default of "enhanced" should be all Xen features. Anything else should be consider as reduced/basic/minimum. Hence why I still think we need to add it in the name even if this is not what we expose in the DT. In fact...
>>>>  /*
>>>>  * List of possible features for dom0less domUs
>>>>  *
>>>>  * DOM0LESS_XENSTORE:		Xenstore will be enabled for the VM. This feature
>>>>  *                                              	can't be enabled without the DOM0LESS_ENHANCED.
>>>>  * DOM0LESS_ENHANCED:       	Notify the OS it is running on top of Xen. All the
>>>>  *                          				default features (including Xenstore) will be
>>>>  *                          				available. Note that an OS *must* not rely on the
>>>>  *                          				availability of Xen features if this is not set.
>>> 
>>> ... what you wrote here match what I wrote above. So it is not clear to me what's the point of having a flag DOM0LESS_XENSTORE.
>> When we looked at the code with the solution using BASIC, it was really not easy to understand.
> 
> I don't quite understand how this is different from ENHANCED, ENHANCED_FULL. In fact, without looking at the documentation, they mean exactly the same...
> 
> The difference between "BASIC" and "ENHANCED" is clear. You know that in one case, you would get less than the other.
> 
>> By the way the comment is wrong and correspond to what should be ENHANCED_FULL here
>> ENHANCED would be the base without Xenstore.
> 
> Thanks for the confirmation. I am afraid, I am strongly against the terminology you proposed (see above why).
> 
> I think BASIC (or similar name) is better. But I am open to suggestion so long it is not "DOM0LESS_ENHANCED" vs "DOM0LESS_ENHANCED_FULL".

I do not agree but I think this is only internal and could easily be modified one day if we have more use-cases.
So let’s go for BASIC and unblock this before the feature freeze.

Bertrand

> 
> As an aside, I think it is more logical if you define DOM0LESS_XENSTORE as bit 1. But that's NIT at this point. What matters is the naming.
> 
> Cheers,
> 
> -- 
> Julien Grall

Re: [PATCH v3 7/7] xen/arm: introduce new xen,enhanced property value

[Rahul Singh]

Hi Julien,

> On 5 Sep 2022, at 1:59 pm, Bertrand Marquis <Bertrand.Marquis@arm.com> wrote:
> 
> Hi Julien,
> 
>> On 5 Sep 2022, at 13:08, Julien Grall <julien@xen.org> wrote:
>> 
>> 
>> 
>> On 05/09/2022 12:54, Bertrand Marquis wrote:
>>> Hi Julien,
>>>> On 5 Sep 2022, at 12:43, Julien Grall <julien@xen.org> wrote:
>>>> 
>>>> 
>>>> 
>>>> On 05/09/2022 12:12, Rahul Singh wrote:
>>>>> Hi Julien,
>>>> 
>>>> Hi Rahul,
>>>> 
>>>>>> On 2 Sep 2022, at 5:20 pm, Julien Grall <julien@xen.org> wrote:
>>>>>> 
>>>>>> 
>>>>>> 
>>>>>> On 02/09/2022 16:54, Rahul Singh wrote:
>>>>>>> Hi Julien,
>>>>>> 
>>>>>> Hi Rahul,
>>>>>> 
>>>>>>>> On 2 Sep 2022, at 4:05 pm, Julien Grall <julien@xen.org> wrote:
>>>>>>>> 
>>>>>>>> Hi Bertrand,
>>>>>>>> 
>>>>>>>> On 02/09/2022 15:51, Bertrand Marquis wrote:
>>>>>>>>>> On 1 Sep 2022, at 19:15, Julien Grall <julien@xen.org> wrote:
>>>>>>>>>> AFAIU, it is not possible to have *_xenstore = true and *_enhanced = false. I think it would be clearer if ``dom0less_enhanced`` is turned to an enum with 3 values:
>>>>>>>>>> - None
>>>>>>>>>> - NOXENSTORE/BASIC
>>>>>>>>>> - FULLY_ENHANCED
>>>>>>>>>> 
>>>>>>>>>> If we want to be future proof, I would use a field 'flags' where non-zero means enhanced. Each bit would indicate which features of Xen is exposed.
>>>>>>>>> I think that could be a good solution if we do it this way:
>>>>>>>>> - define a dom0less feature field and have defines like the following:
>>>>>>>>> #define DOM0LESS_GNTTAB
>>>>>>>>> #define DOM0LESS_EVENTCHN
>>>>>>>>> #define DOM0LESS_XENSTORE >
>>>>>>>>> - define dom0less enhanced as the right combination:
>>>>>>>>> #define DOM0LESS_ENHANCED = (DOM0LESS_GNTTAB| DOM0LESS_EVENTCHN| DOM0LESS_XENSTORE)
>>>>>>>> 
>>>>>>>> I would rather introduce DOM0LESS_ENHANCED_BASIC (or similar) instead of defining a bit for gnttab and evtchn. This will avoid the question of why we are introducing bits for both features but not the hypercall...
>>>>>>>> 
>>>>>>>> As this is an internal interface, it would be easier to modify afterwards.
>>>>>>> How about this?
>>>>>>> /*
>>>>>>> * List of possible features for dom0less domUs
>>>>>>> *
>>>>>>> * DOM0LESS_ENHANCED_BASIC: Xen PV interfaces, including grant-table and
>>>>>>> *                                                          evtchn, will be enabled for the VM.
>>>>>> 
>>>>>> Technically, the guest can already use the grant-table and evtchn interfaces. This also reads quite odd to me because "including" doesn't tell what's not enabled. So one could assume Xenstored is also enabled. In fact the wording for ``DOM0LESS_ENHANCED`` is what makes it a lot more confusing.
>>>>>> 
>>>>>> So I would suggest the following wording:
>>>>>> 
>>>>>> "Notify the OS it is running on top of Xen. All the default features but Xenstore will be available. Note that an OS *must* not rely on the availability of Xen features if this is not set.
>>>>>> "
>>>>>> 
>>>>>> The wording can be updated once we properly disable event channel/grant table when the flag is not set.
>>>>>> 
>>>>>>> * DOM0LESS_XENSTORE:              Xenstore will be enabled for the VM.
>>>>>> 
>>>>>> I would make clear this can't be used without the first one.
>>>>>> 
>>>>>>> * DOM0LESS_ENHANCED:              Xen PV interfaces, including grant-table xenstore >   *                                                          and
>>>>>> evtchn, will be enabled for the VM.
>>>>>> 
>>>>>> See above about "PV interfaces". So I would suggest to reword to:
>>>>>> 
>>>>>> "Notify the OS it is running on top of Xen. All the default features (including Xenstore) will be available".
>>>>>> 
>>>>>>> */
>>>>>>> #define DOM0LESS_ENHANCED_BASIC BIT(0, UL)
>>>>>>> #define DOM0LESS_XENSTORE       BIT(1, UL)
>>>>>> 
>>>>>> Based on the comment above, I would consider to define DOM0LESS_XENSTORE as bit 0 and 1 set.
>>>>>> 
>>>>>>> #define DOM0LESS_ENHANCED       (DOM0LESS_ENHANCED_BASIC | DOM0LESS_XENSTORE)
>>>>> Bertrand and I discussed this again we came to the conclusion that DOM0LESS_ENHANCED_BASIC is not
>>>>> the suitable name as this makes the code unclear and does not correspond to DT settings. We propose this
>>>>> please let me know your thoughts.
>>>> 
>>>> To me the default of "enhanced" should be all Xen features. Anything else should be consider as reduced/basic/minimum. Hence why I still think we need to add it in the name even if this is not what we expose in the DT. In fact...
>>>>> /*
>>>>> * List of possible features for dom0less domUs
>>>>> *
>>>>> * DOM0LESS_XENSTORE:              Xenstore will be enabled for the VM. This feature
>>>>> *                                                 can't be enabled without the DOM0LESS_ENHANCED.
>>>>> * DOM0LESS_ENHANCED:              Notify the OS it is running on top of Xen. All the
>>>>> *                                                         default features (including Xenstore) will be
>>>>> *                                                         available. Note that an OS *must* not rely on the
>>>>> *                                                         availability of Xen features if this is not set.
>>>> 
>>>> ... what you wrote here match what I wrote above. So it is not clear to me what's the point of having a flag DOM0LESS_XENSTORE.
>>> When we looked at the code with the solution using BASIC, it was really not easy to understand.
>> 
>> I don't quite understand how this is different from ENHANCED, ENHANCED_FULL. In fact, without looking at the documentation, they mean exactly the same...
>> 
>> The difference between "BASIC" and "ENHANCED" is clear. You know that in one case, you would get less than the other.
>> 
>>> By the way the comment is wrong and correspond to what should be ENHANCED_FULL here
>>> ENHANCED would be the base without Xenstore.
>> 
>> Thanks for the confirmation. I am afraid, I am strongly against the terminology you proposed (see above why).
>> 
>> I think BASIC (or similar name) is better. But I am open to suggestion so long it is not "DOM0LESS_ENHANCED" vs "DOM0LESS_ENHANCED_FULL".
> 
> I do not agree but I think this is only internal and could easily be modified one day if we have more use-cases.
> So let’s go for BASIC and unblock this before the feature freeze.
> 
> Bertrand

Please have a look once if this looks okay.

/*                                                                              
 * List of possible features for dom0less domUs                                 
 *                                                                              
 * DOM0LESS_ENHANCED_BASIC:	Notify the OS it is running on top of Xen. All the  
 *                                                          	default features (excluding Xenstore) will be       
 *                          					available. Note that an OS *must* not rely on the   
 *                          					availability of Xen features if this is not set.    
 * DOM0LESS_XENSTORE:       		Xenstore will be enabled for the VM. This feature   
 *                          					can't be enabled without the DOM0LESS_ENHANCED_BASIC.                            
 * DOM0LESS_ENHANCED:			Notify the OS it is running on top of Xen. All the  
 *                          					default features (including Xenstore) will be       
 *                          					available. Note that an OS *must* not rely on the   
 *                          					availability of Xen features if this is not set.    
 */                                                                             
#define DOM0LESS_ENHANCED_BASIC     BIT(0, UL)                                  
#define DOM0LESS_XENSTORE                  BIT(1, UL)                                  
#define DOM0LESS_ENHANCED                 (DOM0LESS_ENHANCED_BASIC  |  DOM0LESS_XENSTORE)

Regards,
Rahul
 

Re: [PATCH v3 7/7] xen/arm: introduce new xen,enhanced property value

[Julien Grall]

Hi,

On 05/09/2022 14:36, Rahul Singh wrote:
> Please have a look once if this looks okay.
> 
> /*
>   * List of possible features for dom0less domUs
>   *
>   * DOM0LESS_ENHANCED_BASIC:	Notify the OS it is running on top of Xen. All the
>   *                                                          	default features (excluding Xenstore) will be
>   *                          					available. Note that an OS *must* not rely on the
>   *                          					availability of Xen features if this is not set.
>   * DOM0LESS_XENSTORE:       		Xenstore will be enabled for the VM. This feature
>   *                          					can't be enabled without the DOM0LESS_ENHANCED_BASIC.
>   * DOM0LESS_ENHANCED:			Notify the OS it is running on top of Xen. All the
>   *                          					default features (including Xenstore) will be
>   *                          					available. Note that an OS *must* not rely on the
>   *                          					availability of Xen features if this is not set.
>   */
> #define DOM0LESS_ENHANCED_BASIC     BIT(0, UL)
> #define DOM0LESS_XENSTORE                  BIT(1, UL)
> #define DOM0LESS_ENHANCED                 (DOM0LESS_ENHANCED_BASIC  |  DOM0LESS_XENSTORE)

The explanation looks good to me but the indentation looks odd. Also, I 
think it would be preferable to use U or ULL (if you want 64 bits) so 
the size of the bitfield is not arch depending.

Cheers,

-- 
Julien Grall

Re: [PATCH v3 7/7] xen/arm: introduce new xen,enhanced property value

[Rahul Singh]

Hi Julien, 

> On 5 Sep 2022, at 5:40 pm, Julien Grall <julien@xen.org> wrote:
> 
> Hi,
> 
> On 05/09/2022 14:36, Rahul Singh wrote:
>> Please have a look once if this looks okay.
>> /*
>>  * List of possible features for dom0less domUs
>>  *
>>  * DOM0LESS_ENHANCED_BASIC:	Notify the OS it is running on top of Xen. All the
>>  *                                                          	default features (excluding Xenstore) will be
>>  *                          					available. Note that an OS *must* not rely on the
>>  *                          					availability of Xen features if this is not set.
>>  * DOM0LESS_XENSTORE:       		Xenstore will be enabled for the VM. This feature
>>  *                          					can't be enabled without the DOM0LESS_ENHANCED_BASIC.
>>  * DOM0LESS_ENHANCED:			Notify the OS it is running on top of Xen. All the
>>  *                          					default features (including Xenstore) will be
>>  *                          					available. Note that an OS *must* not rely on the
>>  *                          					availability of Xen features if this is not set.
>>  */
>> #define DOM0LESS_ENHANCED_BASIC     BIT(0, UL)
>> #define DOM0LESS_XENSTORE                  BIT(1, UL)
>> #define DOM0LESS_ENHANCED                 (DOM0LESS_ENHANCED_BASIC  |  DOM0LESS_XENSTORE)
> 
> The explanation looks good to me but the indentation looks odd. Also, I think it would be preferable to use U or ULL (if you want 64 bits) so the size of the bitfield is not arch depending.
> 

I will fix the indentation when sending the official patch. I will use U as I am planning to use uint32_t.

Regards,
Rahul

Re: [PATCH v3 7/7] xen/arm: introduce new xen,enhanced property value

[Stefano Stabellini]

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

On Mon, 5 Sep 2022, Rahul Singh wrote:
> > On 5 Sep 2022, at 1:59 pm, Bertrand Marquis <Bertrand.Marquis@arm.com> wrote:
> > 
> > Hi Julien,
> > 
> >> On 5 Sep 2022, at 13:08, Julien Grall <julien@xen.org> wrote:
> >> 
> >> 
> >> 
> >> On 05/09/2022 12:54, Bertrand Marquis wrote:
> >>> Hi Julien,
> >>>> On 5 Sep 2022, at 12:43, Julien Grall <julien@xen.org> wrote:
> >>>> 
> >>>> 
> >>>> 
> >>>> On 05/09/2022 12:12, Rahul Singh wrote:
> >>>>> Hi Julien,
> >>>> 
> >>>> Hi Rahul,
> >>>> 
> >>>>>> On 2 Sep 2022, at 5:20 pm, Julien Grall <julien@xen.org> wrote:
> >>>>>> 
> >>>>>> 
> >>>>>> 
> >>>>>> On 02/09/2022 16:54, Rahul Singh wrote:
> >>>>>>> Hi Julien,
> >>>>>> 
> >>>>>> Hi Rahul,
> >>>>>> 
> >>>>>>>> On 2 Sep 2022, at 4:05 pm, Julien Grall <julien@xen.org> wrote:
> >>>>>>>> 
> >>>>>>>> Hi Bertrand,
> >>>>>>>> 
> >>>>>>>> On 02/09/2022 15:51, Bertrand Marquis wrote:
> >>>>>>>>>> On 1 Sep 2022, at 19:15, Julien Grall <julien@xen.org> wrote:
> >>>>>>>>>> AFAIU, it is not possible to have *_xenstore = true and *_enhanced = false. I think it would be clearer if ``dom0less_enhanced`` is turned to an enum with 3 values:
> >>>>>>>>>> - None
> >>>>>>>>>> - NOXENSTORE/BASIC
> >>>>>>>>>> - FULLY_ENHANCED
> >>>>>>>>>> 
> >>>>>>>>>> If we want to be future proof, I would use a field 'flags' where non-zero means enhanced. Each bit would indicate which features of Xen is exposed.
> >>>>>>>>> I think that could be a good solution if we do it this way:
> >>>>>>>>> - define a dom0less feature field and have defines like the following:
> >>>>>>>>> #define DOM0LESS_GNTTAB
> >>>>>>>>> #define DOM0LESS_EVENTCHN
> >>>>>>>>> #define DOM0LESS_XENSTORE >
> >>>>>>>>> - define dom0less enhanced as the right combination:
> >>>>>>>>> #define DOM0LESS_ENHANCED = (DOM0LESS_GNTTAB| DOM0LESS_EVENTCHN| DOM0LESS_XENSTORE)
> >>>>>>>> 
> >>>>>>>> I would rather introduce DOM0LESS_ENHANCED_BASIC (or similar) instead of defining a bit for gnttab and evtchn. This will avoid the question of why we are introducing bits for both features but not the hypercall...
> >>>>>>>> 
> >>>>>>>> As this is an internal interface, it would be easier to modify afterwards.
> >>>>>>> How about this?
> >>>>>>> /*
> >>>>>>> * List of possible features for dom0less domUs
> >>>>>>> *
> >>>>>>> * DOM0LESS_ENHANCED_BASIC: Xen PV interfaces, including grant-table and
> >>>>>>> *                                                          evtchn, will be enabled for the VM.
> >>>>>> 
> >>>>>> Technically, the guest can already use the grant-table and evtchn interfaces. This also reads quite odd to me because "including" doesn't tell what's not enabled. So one could assume Xenstored is also enabled. In fact the wording for ``DOM0LESS_ENHANCED`` is what makes it a lot more confusing.
> >>>>>> 
> >>>>>> So I would suggest the following wording:
> >>>>>> 
> >>>>>> "Notify the OS it is running on top of Xen. All the default features but Xenstore will be available. Note that an OS *must* not rely on the availability of Xen features if this is not set.
> >>>>>> "
> >>>>>> 
> >>>>>> The wording can be updated once we properly disable event channel/grant table when the flag is not set.
> >>>>>> 
> >>>>>>> * DOM0LESS_XENSTORE:              Xenstore will be enabled for the VM.
> >>>>>> 
> >>>>>> I would make clear this can't be used without the first one.
> >>>>>> 
> >>>>>>> * DOM0LESS_ENHANCED:              Xen PV interfaces, including grant-table xenstore >   *                                                          and
> >>>>>> evtchn, will be enabled for the VM.
> >>>>>> 
> >>>>>> See above about "PV interfaces". So I would suggest to reword to:
> >>>>>> 
> >>>>>> "Notify the OS it is running on top of Xen. All the default features (including Xenstore) will be available".
> >>>>>> 
> >>>>>>> */
> >>>>>>> #define DOM0LESS_ENHANCED_BASIC BIT(0, UL)
> >>>>>>> #define DOM0LESS_XENSTORE       BIT(1, UL)
> >>>>>> 
> >>>>>> Based on the comment above, I would consider to define DOM0LESS_XENSTORE as bit 0 and 1 set.
> >>>>>> 
> >>>>>>> #define DOM0LESS_ENHANCED       (DOM0LESS_ENHANCED_BASIC | DOM0LESS_XENSTORE)
> >>>>> Bertrand and I discussed this again we came to the conclusion that DOM0LESS_ENHANCED_BASIC is not
> >>>>> the suitable name as this makes the code unclear and does not correspond to DT settings. We propose this
> >>>>> please let me know your thoughts.
> >>>> 
> >>>> To me the default of "enhanced" should be all Xen features. Anything else should be consider as reduced/basic/minimum. Hence why I still think we need to add it in the name even if this is not what we expose in the DT. In fact...
> >>>>> /*
> >>>>> * List of possible features for dom0less domUs
> >>>>> *
> >>>>> * DOM0LESS_XENSTORE:              Xenstore will be enabled for the VM. This feature
> >>>>> *                                                 can't be enabled without the DOM0LESS_ENHANCED.
> >>>>> * DOM0LESS_ENHANCED:              Notify the OS it is running on top of Xen. All the
> >>>>> *                                                         default features (including Xenstore) will be
> >>>>> *                                                         available. Note that an OS *must* not rely on the
> >>>>> *                                                         availability of Xen features if this is not set.
> >>>> 
> >>>> ... what you wrote here match what I wrote above. So it is not clear to me what's the point of having a flag DOM0LESS_XENSTORE.
> >>> When we looked at the code with the solution using BASIC, it was really not easy to understand.
> >> 
> >> I don't quite understand how this is different from ENHANCED, ENHANCED_FULL. In fact, without looking at the documentation, they mean exactly the same...
> >> 
> >> The difference between "BASIC" and "ENHANCED" is clear. You know that in one case, you would get less than the other.
> >> 
> >>> By the way the comment is wrong and correspond to what should be ENHANCED_FULL here
> >>> ENHANCED would be the base without Xenstore.
> >> 
> >> Thanks for the confirmation. I am afraid, I am strongly against the terminology you proposed (see above why).
> >> 
> >> I think BASIC (or similar name) is better. But I am open to suggestion so long it is not "DOM0LESS_ENHANCED" vs "DOM0LESS_ENHANCED_FULL".
> > 
> > I do not agree but I think this is only internal and could easily be modified one day if we have more use-cases.
> > So let’s go for BASIC and unblock this before the feature freeze.
> > 
> > Bertrand
> 
> Please have a look once if this looks okay.
> 
> /*                                                                              
>  * List of possible features for dom0less domUs                                 
>  *                                                                              
>  * DOM0LESS_ENHANCED_BASIC:	Notify the OS it is running on top of Xen. All the  
>  *                                                          	default features (excluding Xenstore) will be       
>  *                          					available. Note that an OS *must* not rely on the   
>  *                          					availability of Xen features if this is not set.    
>  * DOM0LESS_XENSTORE:       		Xenstore will be enabled for the VM. This feature   
>  *                          					can't be enabled without the DOM0LESS_ENHANCED_BASIC.                            
>  * DOM0LESS_ENHANCED:			Notify the OS it is running on top of Xen. All the  
>  *                          					default features (including Xenstore) will be       
>  *                          					available. Note that an OS *must* not rely on the   
>  *                          					availability of Xen features if this is not set.    
>  */                                                                             
> #define DOM0LESS_ENHANCED_BASIC     BIT(0, UL)                                  
> #define DOM0LESS_XENSTORE                  BIT(1, UL)                                  
> #define DOM0LESS_ENHANCED                 (DOM0LESS_ENHANCED_BASIC  |  DOM0LESS_XENSTORE)

Let me have a chance to propose a naming scheme as well :-)

I agree with Julien: I prefer this proposal compared to the earlier one
by Bertrand and Rahul because I think it is a lot clearer and "ENHANCED"
should mean everything. Also, it makes it easier from a compatibility
perspective because it matches the current definition.

But I also agree with Bertrand that "BASIC" doesn't sound nice. I think
we should keep "DOM0LESS_ENHANCED" and "DOM0LESS_XENSTORE" as suggested
here, but replace "DOM0LESS_ENHANCED_BASIC" with something better. Some
ideas:

- DOM0LESS_ENHANCED_LIMITED
- DOM0LESS_ENHANCED_MINI
- DOM0LESS_ENHANCED_NO_XS
- DOM0LESS_ENHANCED_GNT_EVTCHN

Any of these are better than BASIC from my point of view. Now I am off
to get the green paint for my shed.

Re: [PATCH v3 7/7] xen/arm: introduce new xen,enhanced property value

[Bertrand Marquis]

Hi Stefano,

> On 5 Sep 2022, at 23:41, Stefano Stabellini <sstabellini@kernel.org> wrote:
> 
> On Mon, 5 Sep 2022, Rahul Singh wrote:
>>> On 5 Sep 2022, at 1:59 pm, Bertrand Marquis <Bertrand.Marquis@arm.com> wrote:
>>> 
>>> Hi Julien,
>>> 
>>>> On 5 Sep 2022, at 13:08, Julien Grall <julien@xen.org> wrote:
>>>> 
>>>> 
>>>> 
>>>> On 05/09/2022 12:54, Bertrand Marquis wrote:
>>>>> Hi Julien,
>>>>>> On 5 Sep 2022, at 12:43, Julien Grall <julien@xen.org> wrote:
>>>>>> 
>>>>>> 
>>>>>> 
>>>>>> On 05/09/2022 12:12, Rahul Singh wrote:
>>>>>>> Hi Julien,
>>>>>> 
>>>>>> Hi Rahul,
>>>>>> 
>>>>>>>> On 2 Sep 2022, at 5:20 pm, Julien Grall <julien@xen.org> wrote:
>>>>>>>> 
>>>>>>>> 
>>>>>>>> 
>>>>>>>> On 02/09/2022 16:54, Rahul Singh wrote:
>>>>>>>>> Hi Julien,
>>>>>>>> 
>>>>>>>> Hi Rahul,
>>>>>>>> 
>>>>>>>>>> On 2 Sep 2022, at 4:05 pm, Julien Grall <julien@xen.org> wrote:
>>>>>>>>>> 
>>>>>>>>>> Hi Bertrand,
>>>>>>>>>> 
>>>>>>>>>> On 02/09/2022 15:51, Bertrand Marquis wrote:
>>>>>>>>>>>> On 1 Sep 2022, at 19:15, Julien Grall <julien@xen.org> wrote:
>>>>>>>>>>>> AFAIU, it is not possible to have *_xenstore = true and *_enhanced = false. I think it would be clearer if ``dom0less_enhanced`` is turned to an enum with 3 values:
>>>>>>>>>>>> - None
>>>>>>>>>>>> - NOXENSTORE/BASIC
>>>>>>>>>>>> - FULLY_ENHANCED
>>>>>>>>>>>> 
>>>>>>>>>>>> If we want to be future proof, I would use a field 'flags' where non-zero means enhanced. Each bit would indicate which features of Xen is exposed.
>>>>>>>>>>> I think that could be a good solution if we do it this way:
>>>>>>>>>>> - define a dom0less feature field and have defines like the following:
>>>>>>>>>>> #define DOM0LESS_GNTTAB
>>>>>>>>>>> #define DOM0LESS_EVENTCHN
>>>>>>>>>>> #define DOM0LESS_XENSTORE >
>>>>>>>>>>> - define dom0less enhanced as the right combination:
>>>>>>>>>>> #define DOM0LESS_ENHANCED = (DOM0LESS_GNTTAB| DOM0LESS_EVENTCHN| DOM0LESS_XENSTORE)
>>>>>>>>>> 
>>>>>>>>>> I would rather introduce DOM0LESS_ENHANCED_BASIC (or similar) instead of defining a bit for gnttab and evtchn. This will avoid the question of why we are introducing bits for both features but not the hypercall...
>>>>>>>>>> 
>>>>>>>>>> As this is an internal interface, it would be easier to modify afterwards.
>>>>>>>>> How about this?
>>>>>>>>> /*
>>>>>>>>> * List of possible features for dom0less domUs
>>>>>>>>> *
>>>>>>>>> * DOM0LESS_ENHANCED_BASIC: Xen PV interfaces, including grant-table and
>>>>>>>>> *                                                          evtchn, will be enabled for the VM.
>>>>>>>> 
>>>>>>>> Technically, the guest can already use the grant-table and evtchn interfaces. This also reads quite odd to me because "including" doesn't tell what's not enabled. So one could assume Xenstored is also enabled. In fact the wording for ``DOM0LESS_ENHANCED`` is what makes it a lot more confusing.
>>>>>>>> 
>>>>>>>> So I would suggest the following wording:
>>>>>>>> 
>>>>>>>> "Notify the OS it is running on top of Xen. All the default features but Xenstore will be available. Note that an OS *must* not rely on the availability of Xen features if this is not set.
>>>>>>>> "
>>>>>>>> 
>>>>>>>> The wording can be updated once we properly disable event channel/grant table when the flag is not set.
>>>>>>>> 
>>>>>>>>> * DOM0LESS_XENSTORE:              Xenstore will be enabled for the VM.
>>>>>>>> 
>>>>>>>> I would make clear this can't be used without the first one.
>>>>>>>> 
>>>>>>>>> * DOM0LESS_ENHANCED:              Xen PV interfaces, including grant-table xenstore >   *                                                          and
>>>>>>>> evtchn, will be enabled for the VM.
>>>>>>>> 
>>>>>>>> See above about "PV interfaces". So I would suggest to reword to:
>>>>>>>> 
>>>>>>>> "Notify the OS it is running on top of Xen. All the default features (including Xenstore) will be available".
>>>>>>>> 
>>>>>>>>> */
>>>>>>>>> #define DOM0LESS_ENHANCED_BASIC BIT(0, UL)
>>>>>>>>> #define DOM0LESS_XENSTORE       BIT(1, UL)
>>>>>>>> 
>>>>>>>> Based on the comment above, I would consider to define DOM0LESS_XENSTORE as bit 0 and 1 set.
>>>>>>>> 
>>>>>>>>> #define DOM0LESS_ENHANCED       (DOM0LESS_ENHANCED_BASIC | DOM0LESS_XENSTORE)
>>>>>>> Bertrand and I discussed this again we came to the conclusion that DOM0LESS_ENHANCED_BASIC is not
>>>>>>> the suitable name as this makes the code unclear and does not correspond to DT settings. We propose this
>>>>>>> please let me know your thoughts.
>>>>>> 
>>>>>> To me the default of "enhanced" should be all Xen features. Anything else should be consider as reduced/basic/minimum. Hence why I still think we need to add it in the name even if this is not what we expose in the DT. In fact...
>>>>>>> /*
>>>>>>> * List of possible features for dom0less domUs
>>>>>>> *
>>>>>>> * DOM0LESS_XENSTORE:              Xenstore will be enabled for the VM. This feature
>>>>>>> *                                                 can't be enabled without the DOM0LESS_ENHANCED.
>>>>>>> * DOM0LESS_ENHANCED:              Notify the OS it is running on top of Xen. All the
>>>>>>> *                                                         default features (including Xenstore) will be
>>>>>>> *                                                         available. Note that an OS *must* not rely on the
>>>>>>> *                                                         availability of Xen features if this is not set.
>>>>>> 
>>>>>> ... what you wrote here match what I wrote above. So it is not clear to me what's the point of having a flag DOM0LESS_XENSTORE.
>>>>> When we looked at the code with the solution using BASIC, it was really not easy to understand.
>>>> 
>>>> I don't quite understand how this is different from ENHANCED, ENHANCED_FULL. In fact, without looking at the documentation, they mean exactly the same...
>>>> 
>>>> The difference between "BASIC" and "ENHANCED" is clear. You know that in one case, you would get less than the other.
>>>> 
>>>>> By the way the comment is wrong and correspond to what should be ENHANCED_FULL here
>>>>> ENHANCED would be the base without Xenstore.
>>>> 
>>>> Thanks for the confirmation. I am afraid, I am strongly against the terminology you proposed (see above why).
>>>> 
>>>> I think BASIC (or similar name) is better. But I am open to suggestion so long it is not "DOM0LESS_ENHANCED" vs "DOM0LESS_ENHANCED_FULL".
>>> 
>>> I do not agree but I think this is only internal and could easily be modified one day if we have more use-cases.
>>> So let’s go for BASIC and unblock this before the feature freeze.
>>> 
>>> Bertrand
>> 
>> Please have a look once if this looks okay.
>> 
>> /*                                                                              
>> * List of possible features for dom0less domUs                                 
>> *                                                                              
>> * DOM0LESS_ENHANCED_BASIC:	Notify the OS it is running on top of Xen. All the  
>> *                                                          	default features (excluding Xenstore) will be       
>> *                          					available. Note that an OS *must* not rely on the   
>> *                          					availability of Xen features if this is not set.    
>> * DOM0LESS_XENSTORE:       		Xenstore will be enabled for the VM. This feature   
>> *                          					can't be enabled without the DOM0LESS_ENHANCED_BASIC.                            
>> * DOM0LESS_ENHANCED:			Notify the OS it is running on top of Xen. All the  
>> *                          					default features (including Xenstore) will be       
>> *                          					available. Note that an OS *must* not rely on the   
>> *                          					availability of Xen features if this is not set.    
>> */                                                                             
>> #define DOM0LESS_ENHANCED_BASIC     BIT(0, UL)                                  
>> #define DOM0LESS_XENSTORE                  BIT(1, UL)                                  
>> #define DOM0LESS_ENHANCED                 (DOM0LESS_ENHANCED_BASIC  |  DOM0LESS_XENSTORE)
> 
> Let me have a chance to propose a naming scheme as well :-)
> 
> I agree with Julien: I prefer this proposal compared to the earlier one
> by Bertrand and Rahul because I think it is a lot clearer and "ENHANCED"
> should mean everything. Also, it makes it easier from a compatibility
> perspective because it matches the current definition.
> 
> But I also agree with Bertrand that "BASIC" doesn't sound nice. I think
> we should keep "DOM0LESS_ENHANCED" and "DOM0LESS_XENSTORE" as suggested
> here, but replace "DOM0LESS_ENHANCED_BASIC" with something better. Some
> ideas:
> 
> - DOM0LESS_ENHANCED_LIMITED
> - DOM0LESS_ENHANCED_MINI

Personally I do not find those more clear then BASIC

> - DOM0LESS_ENHANCED_NO_XS

This has the problem to be true now but would need renaming if we introduce a definition for an other bit.

> - DOM0LESS_ENHANCED_GNT_EVTCHN

I would vote for this one as it explicitly state what is in so the bitset system is even more meaningful.

> 
> Any of these are better than BASIC from my point of view. Now I am off
> to get the green paint for my shed.

Have fun ;-)

Cheers
Bertrand

Re: [PATCH v3 7/7] xen/arm: introduce new xen,enhanced property value

[Julien Grall]

Hi Bertrand,

On 06/09/2022 08:24, Bertrand Marquis wrote:
>> I agree with Julien: I prefer this proposal compared to the earlier one
>> by Bertrand and Rahul because I think it is a lot clearer and "ENHANCED"
>> should mean everything. Also, it makes it easier from a compatibility
>> perspective because it matches the current definition.
>>
>> But I also agree with Bertrand that "BASIC" doesn't sound nice. I think
>> we should keep "DOM0LESS_ENHANCED" and "DOM0LESS_XENSTORE" as suggested
>> here, but replace "DOM0LESS_ENHANCED_BASIC" with something better. Some
>> ideas:
>>
>> - DOM0LESS_ENHANCED_LIMITED
>> - DOM0LESS_ENHANCED_MINI
> 
> Personally I do not find those more clear then BASIC
> 
>> - DOM0LESS_ENHANCED_NO_XS
> 
> This has the problem to be true now but would need renaming if we introduce a definition for an other bit.

Internal renaming is not a problem.

> 
>> - DOM0LESS_ENHANCED_GNT_EVTCHN
> 
> I would vote for this one as it explicitly state what is in so the bitset system is even more meaningful.

This would be fine if the flag were doing what it is supposed to do (i.e 
enable grant-table and event-channel only). However, so far, it will 
expose any Xen features but Xenstore. So of the features are strictly 
not necessary for the grant-table/event-channel support (e.g. ballooning 
facilities, runstate...).

The name would also really confusing in the definition of ENHANCED 
(XENSTORE | GNT_EVTCHN). Does this mean the domain cannot use the runstate?

Cheers,

-- 
Julien Grall

Re: [PATCH v3 7/7] xen/arm: introduce new xen,enhanced property value

[Bertrand Marquis]

Hi,

> On 6 Sep 2022, at 08:54, Julien Grall <julien@xen.org> wrote:
> 
> Hi Bertrand,
> 
> On 06/09/2022 08:24, Bertrand Marquis wrote:
>>> I agree with Julien: I prefer this proposal compared to the earlier one
>>> by Bertrand and Rahul because I think it is a lot clearer and "ENHANCED"
>>> should mean everything. Also, it makes it easier from a compatibility
>>> perspective because it matches the current definition.
>>> 
>>> But I also agree with Bertrand that "BASIC" doesn't sound nice. I think
>>> we should keep "DOM0LESS_ENHANCED" and "DOM0LESS_XENSTORE" as suggested
>>> here, but replace "DOM0LESS_ENHANCED_BASIC" with something better. Some
>>> ideas:
>>> 
>>> - DOM0LESS_ENHANCED_LIMITED
>>> - DOM0LESS_ENHANCED_MINI
>> Personally I do not find those more clear then BASIC
>>> - DOM0LESS_ENHANCED_NO_XS
>> This has the problem to be true now but would need renaming if we introduce a definition for an other bit.
> 
> Internal renaming is not a problem.

Then let’s go for this.

Cheers
Bertrand

> 
>>> - DOM0LESS_ENHANCED_GNT_EVTCHN
>> I would vote for this one as it explicitly state what is in so the bitset system is even more meaningful.
> 
> This would be fine if the flag were doing what it is supposed to do (i.e enable grant-table and event-channel only). However, so far, it will expose any Xen features but Xenstore. So of the features are strictly not necessary for the grant-table/event-channel support (e.g. ballooning facilities, runstate...).
> 
> The name would also really confusing in the definition of ENHANCED (XENSTORE | GNT_EVTCHN). Does this mean the domain cannot use the runstate?
> 
> Cheers,
> 
> -- 
> Julien Grall
>