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

[PATCH v4 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 (5):
  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 new xen,enhanced property value
  xen/arm: introduce xen-evtchn dom0less property

Stanislav Kinsburskii (1):
  xen/evtchn: Add an helper to reserve/allocate a port

 docs/misc/arm/device-tree/booting.txt | 102 ++++++++++++++++
 xen/arch/arm/domain_build.c           | 167 +++++++++++++++++++++++++-
 xen/arch/arm/include/asm/kernel.h     |  23 +++-
 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         |  16 +++
 xen/include/xen/event.h               |   8 +-
 8 files changed, 387 insertions(+), 53 deletions(-)

-- 
2.25.1

[PATCH v4 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>
Reviewed-by: Michal Orzel <michal.orzel@amd.com>
---
Changes in v4:
 - fix comment to remove the reference to Guest Transparent Migration
   and Live Update
 - Added Michal Reviewed-by
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..f81c229358 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 allocating the static evtchn port
+ * numbers that are scattered in nature.
+ */
 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 v4 2/7] xen/evtchn: Add an helper to reserve/allocate a port

[Rahul Singh]

From: Stanislav Kinsburskii <staskins@amazon.com>

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 v4:
 - Change the Author to Stanislav Kinsburskii <staskins@amazon.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 f81c229358..565ab71881 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 v4 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. Another reason why we choose the value
of 1023 to follow the default behavior of libxl.

Signed-off-by: Rahul Singh <rahul.singh@arm.com>
Reviewed-by: Michal Orzel <michal.orzel@amd.com>
---
Changes in v4:
 - fix minor comments in commit msg
 - Added Michal Reviewed-by
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 v4 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>
Acked-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Julien Grall <jgrall@amazon.com>
---
Changes in v4:
 - no changes
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 565ab71881..f546e81758 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 v4 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 v4:
 - no changes
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 f546e81758..f5e0b12d15 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 v4 6/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 v4:
 - Implement defines for dom0less features
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     | 23 +++++++++++++++++++++--
 3 files changed, 31 insertions(+), 6 deletions(-)

diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt
index 98253414b8..1b0dca1454 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 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 707e247f6a..0b164ef595 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -2891,7 +2891,7 @@ static int __init prepare_dtb_domU(struct domain *d, struct kernel_info *kinfo)
             goto err;
     }
 
-    if ( kinfo->dom0less_enhanced )
+    if ( kinfo->dom0less_feature & DOM0LESS_ENHANCED_NO_XS )
     {
         ret = make_hypervisor_node(d, kinfo, addrcells, sizecells);
         if ( ret )
@@ -3209,10 +3209,12 @@ static int __init construct_domU(struct domain *d,
          (rc == 0 && !strcmp(dom0less_enhanced, "enabled")) )
     {
         if ( hardware_domain )
-            kinfo.dom0less_enhanced = true;
+            kinfo.dom0less_feature = DOM0LESS_ENHANCED;
         else
-            panic("Tried to use xen,enhanced without dom0\n");
+            panic("At the moment, Xenstore support requires dom0 to be present\n");
     }
+    else if ( rc == 0 && !strcmp(dom0less_enhanced, "no-xenstore") )
+        kinfo.dom0less_feature = DOM0LESS_ENHANCED_NO_XS;
 
     if ( vcpu_create(d, 0) == NULL )
         return -ENOMEM;
@@ -3252,7 +3254,7 @@ static int __init construct_domU(struct domain *d,
     if ( rc < 0 )
         return rc;
 
-    if ( kinfo.dom0less_enhanced )
+    if ( kinfo.dom0less_feature & 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..ad240494ea 100644
--- a/xen/arch/arm/include/asm/kernel.h
+++ b/xen/arch/arm/include/asm/kernel.h
@@ -9,6 +9,25 @@
 #include <xen/device_tree.h>
 #include <asm/setup.h>
 
+/*
+ * List of possible features for dom0less domUs
+ *
+ * DOM0LESS_ENHANCED_NO_XS: 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_NO_XS.
+ * 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_NO_XS  BIT(0, U)
+#define DOM0LESS_XENSTORE        BIT(1, U)
+#define DOM0LESS_ENHANCED        (DOM0LESS_ENHANCED_NO_XS | DOM0LESS_XENSTORE)
+
 struct kernel_info {
 #ifdef CONFIG_ARM_64
     enum domain_type type;
@@ -36,8 +55,8 @@ struct kernel_info {
     /* Enable pl011 emulation */
     bool vpl011;
 
-    /* Enable PV drivers */
-    bool dom0less_enhanced;
+    /* Enable/Disable PV drivers interface,grant table, evtchn or xenstore */
+    uint32_t dom0less_feature;
 
     /* GIC phandle */
     uint32_t phandle_gic;
-- 
2.25.1

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

[Rahul Singh]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=Y, Size: 11553 bytes --]

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 v4:
 - move documentation to common place for evtchn node in booting.txt
 - Add comment why we use dt_device_static_evtchn_created()
 - check if dt_get_parent() returns NULL
 - fold process_static_evtchn_node() in alloc_static_evtchn()
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 |  98 +++++++++++++++++
 xen/arch/arm/domain_build.c           | 147 ++++++++++++++++++++++++++
 xen/arch/arm/include/asm/setup.h      |   1 +
 xen/arch/arm/setup.c                  |   2 +
 xen/include/xen/device_tree.h         |  16 +++
 5 files changed, 264 insertions(+)

diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt
index 1b0dca1454..c8329b73e5 100644
--- a/docs/misc/arm/device-tree/booting.txt
+++ b/docs/misc/arm/device-tree/booting.txt
@@ -382,3 +382,101 @@ device-tree:
 
 This will reserve a 512MB region starting at the host physical address
 0x30000000 to be exclusively used by DomU1.
+
+Static Event Channel
+====================
+The event channel communication will be established statically between two
+domains (dom0 and domU also). Event channel connection information between
+domains will be passed to Xen via the device tree node. The event channel
+will be created and established in Xen before the domain started. The domain
+doesn’t need to do any operation to establish a connection. Domain only
+needs hypercall EVTCHNOP_send(local port) to send notifications to the
+remote guest.
+
+There is no need to describe the static event channel info in the domU device
+tree. Static event channels are only useful in fully static configurations,
+and in those configurations, the domU device tree dynamically generated by Xen
+is not needed.
+
+To enable the event-channel interface for domU guests include the
+"xen,enhanced = "no-xenstore"" property in the domU Xen device tree node.
+
+Under the "xen,domain" compatible node for domU, there needs to be sub-nodes
+with compatible "xen,evtchn" that describe the event channel connection
+between two domUs. For dom0, there needs to be sub-nodes with compatible
+"xen,evtchn" under the chosen node.
+
+The static event channel 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 {
+
+    /* 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>;
+        #size-cells = <0x1>;
+        xen,enhanced = "no-xenstore";
+
+        /* 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 {
+        compatible = "xen,domain";
+        #address-cells = <0x2>;
+        #size-cells = <0x1>;
+        xen,enhanced = "no-xenstore";
+
+        /* 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 0b164ef595..bb96fa5096 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,150 @@ 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;
+    const struct dt_device_node *p1_node, *p2_node;
+    struct evtchn_alloc_unbound alloc_unbound;
+    struct evtchn_bind_interdomain bind_interdomain;
+    struct domain *d1 = NULL, *d2 = NULL;
+
+    if ( !dt_device_is_compatible(node, "xen,evtchn-v1") )
+        return 0;
+
+    /*
+     * Event channel is already created while parsing the other side of
+     * evtchn node.
+     */
+    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;
+    }
+
+    p1_node = dt_get_parent(node);
+    if ( !p1_node )
+    {
+        printk(XENLOG_ERR "evtchn: evtchn parent node is NULL\n" );
+        return -EINVAL;
+    }
+
+    p2_node = dt_get_parent(remote_node);
+    if ( !p2_node )
+    {
+        printk(XENLOG_ERR "evtchn: remote parent node is NULL\n" );
+        return -EINVAL;
+    }
+
+    d1 = get_domain_by_id(p1_node->used_by);
+    d2 = get_domain_by_id(p2_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 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 )
+        {
+            if ( alloc_domain_evtchn(node) != 0 )
+                panic("Could not set up domains evtchn\n");
+        }
+
+        dt_for_each_child_node(node, evtchn_node)
+        {
+            if ( alloc_domain_evtchn(evtchn_node) != 0 )
+                panic("Could not set up domains evtchn\n");
+        }
+    }
+}
+
 static void __init find_gnttab_region(struct domain *d,
                                       struct kernel_info *kinfo)
 {
@@ -3366,6 +3512,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..a28937d12a 100644
--- a/xen/include/xen/device_tree.h
+++ b/xen/include/xen/device_tree.h
@@ -92,6 +92,10 @@ struct dt_device_node {
 
     /* 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.
@@ -317,6 +321,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

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

[Jan Beulich]

On 06.09.2022 15:40, 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>
> Reviewed-by: Michal Orzel <michal.orzel@amd.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

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

[Stefano Stabellini]

On Tue, 6 Sep 2022, 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 v4:
>  - Implement defines for dom0less features
> 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     | 23 +++++++++++++++++++++--
>  3 files changed, 31 insertions(+), 6 deletions(-)
> 
> diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt
> index 98253414b8..1b0dca1454 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 but xenstore
> +    will be disabled for the VM.

Please use "" for consistency:

    - "no-xenstore"


>      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 707e247f6a..0b164ef595 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -2891,7 +2891,7 @@ static int __init prepare_dtb_domU(struct domain *d, struct kernel_info *kinfo)
>              goto err;
>      }
>  
> -    if ( kinfo->dom0less_enhanced )
> +    if ( kinfo->dom0less_feature & DOM0LESS_ENHANCED_NO_XS )
>      {
>          ret = make_hypervisor_node(d, kinfo, addrcells, sizecells);
>          if ( ret )
> @@ -3209,10 +3209,12 @@ static int __init construct_domU(struct domain *d,
>           (rc == 0 && !strcmp(dom0less_enhanced, "enabled")) )
>      {
>          if ( hardware_domain )
> -            kinfo.dom0less_enhanced = true;
> +            kinfo.dom0less_feature = DOM0LESS_ENHANCED;
>          else
> -            panic("Tried to use xen,enhanced without dom0\n");
> +            panic("At the moment, Xenstore support requires dom0 to be present\n");
>      }
> +    else if ( rc == 0 && !strcmp(dom0less_enhanced, "no-xenstore") )
> +        kinfo.dom0less_feature = DOM0LESS_ENHANCED_NO_XS;
>  
>      if ( vcpu_create(d, 0) == NULL )
>          return -ENOMEM;
> @@ -3252,7 +3254,7 @@ static int __init construct_domU(struct domain *d,
>      if ( rc < 0 )
>          return rc;
>  
> -    if ( kinfo.dom0less_enhanced )
> +    if ( kinfo.dom0less_feature & 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..ad240494ea 100644
> --- a/xen/arch/arm/include/asm/kernel.h
> +++ b/xen/arch/arm/include/asm/kernel.h
> @@ -9,6 +9,25 @@
>  #include <xen/device_tree.h>
>  #include <asm/setup.h>
>  
> +/*
> + * List of possible features for dom0less domUs
> + *
> + * DOM0LESS_ENHANCED_NO_XS: 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_NO_XS.
> + * 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_NO_XS  BIT(0, U)
> +#define DOM0LESS_XENSTORE        BIT(1, U)
> +#define DOM0LESS_ENHANCED        (DOM0LESS_ENHANCED_NO_XS | DOM0LESS_XENSTORE)
> +
>  struct kernel_info {
>  #ifdef CONFIG_ARM_64
>      enum domain_type type;
> @@ -36,8 +55,8 @@ struct kernel_info {
>      /* Enable pl011 emulation */
>      bool vpl011;
>  
> -    /* Enable PV drivers */
> -    bool dom0less_enhanced;
> +    /* Enable/Disable PV drivers interface,grant table, evtchn or xenstore */

missing a whitespace


> +    uint32_t dom0less_feature;

Given that we only really need 2 bits today, and given that uint8_t and
uint16_t are free but uint32_t increases the size of the struct, could
we just use uint16_t dom0less_feature ?


Everything else looks good, these are just minor things.


>      /* GIC phandle */
>      uint32_t phandle_gic;
> -- 
> 2.25.1
> 

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

[Stefano Stabellini]

On Tue, 6 Sep 2022, 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 v4:
>  - move documentation to common place for evtchn node in booting.txt
>  - Add comment why we use dt_device_static_evtchn_created()
>  - check if dt_get_parent() returns NULL
>  - fold process_static_evtchn_node() in alloc_static_evtchn()
> 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 |  98 +++++++++++++++++

I have just reviewed the binding, only three minor comments below.
Everything looks good.


>  xen/arch/arm/domain_build.c           | 147 ++++++++++++++++++++++++++
>  xen/arch/arm/include/asm/setup.h      |   1 +
>  xen/arch/arm/setup.c                  |   2 +
>  xen/include/xen/device_tree.h         |  16 +++
>  5 files changed, 264 insertions(+)
> 
> diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt
> index 1b0dca1454..c8329b73e5 100644
> --- a/docs/misc/arm/device-tree/booting.txt
> +++ b/docs/misc/arm/device-tree/booting.txt
> @@ -382,3 +382,101 @@ device-tree:
>  
>  This will reserve a 512MB region starting at the host physical address
>  0x30000000 to be exclusively used by DomU1.
> +
> +Static Event Channel
> +====================
> +The event channel communication will be established statically between two
> +domains (dom0 and domU also). Event channel connection information between
> +domains will be passed to Xen via the device tree node. The event channel
> +will be created and established in Xen before the domain started. The domain
> +doesn???t need to do any operation to establish a connection. Domain only

doesn't

better to use ASCII if possible


> +needs hypercall EVTCHNOP_send(local port) to send notifications to the
> +remote guest.
> +
> +There is no need to describe the static event channel info in the domU device
> +tree. Static event channels are only useful in fully static configurations,
> +and in those configurations, the domU device tree dynamically generated by Xen
> +is not needed.
> +
> +To enable the event-channel interface for domU guests include the
> +"xen,enhanced = "no-xenstore"" property in the domU Xen device tree node.

double ""


> +
> +Under the "xen,domain" compatible node for domU, there needs to be sub-nodes
> +with compatible "xen,evtchn" that describe the event channel connection
> +between two domUs. For dom0, there needs to be sub-nodes with compatible
> +"xen,evtchn" under the chosen node.
> +
> +The static event channel 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 {
> +
> +    /* one sub-node per local event channel */

It would be good to say that this is for dom0 in the comment, e.g.:

/* this is for Dom0 */


> +    ec1: evtchn@1 {
> +         compatible = "xen,evtchn-v1";
> +         /* local-evtchn link-to-foreign-evtchn */
> +         xen,evtchn = <0xa &ec2>;
> +    };
> +
> +    domU1 {
> +        compatible = "xen,domain";
> +        #address-cells = <0x2>;
> +        #size-cells = <0x1>;
> +        xen,enhanced = "no-xenstore";
> +
> +        /* 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 {
> +        compatible = "xen,domain";
> +        #address-cells = <0x2>;
> +        #size-cells = <0x1>;
> +        xen,enhanced = "no-xenstore";
> +
> +        /* 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>;
> +        };
> +    };
> +};

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

[Rahul Singh]

Hi Stefano,

> On 6 Sep 2022, at 11:22 pm, Stefano Stabellini <sstabellini@kernel.org> wrote:
> 
> On Tue, 6 Sep 2022, 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 v4:
>> - move documentation to common place for evtchn node in booting.txt
>> - Add comment why we use dt_device_static_evtchn_created()
>> - check if dt_get_parent() returns NULL
>> - fold process_static_evtchn_node() in alloc_static_evtchn()
>> 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 |  98 +++++++++++++++++
> 
> I have just reviewed the binding, only three minor comments below.
> Everything looks good.

Thanks for reviewing the code.
> 
> 
>> xen/arch/arm/domain_build.c           | 147 ++++++++++++++++++++++++++
>> xen/arch/arm/include/asm/setup.h      |   1 +
>> xen/arch/arm/setup.c                  |   2 +
>> xen/include/xen/device_tree.h         |  16 +++
>> 5 files changed, 264 insertions(+)
>> 
>> diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt
>> index 1b0dca1454..c8329b73e5 100644
>> --- a/docs/misc/arm/device-tree/booting.txt
>> +++ b/docs/misc/arm/device-tree/booting.txt
>> @@ -382,3 +382,101 @@ device-tree:
>> 
>> This will reserve a 512MB region starting at the host physical address
>> 0x30000000 to be exclusively used by DomU1.
>> +
>> +Static Event Channel
>> +====================
>> +The event channel communication will be established statically between two
>> +domains (dom0 and domU also). Event channel connection information between
>> +domains will be passed to Xen via the device tree node. The event channel
>> +will be created and established in Xen before the domain started. The domain
>> +doesn???t need to do any operation to establish a connection. Domain only
> 
> doesn't
> 
> better to use ASCII if possible

Ack.
> 
> 
>> +needs hypercall EVTCHNOP_send(local port) to send notifications to the
>> +remote guest.
>> +
>> +There is no need to describe the static event channel info in the domU device
>> +tree. Static event channels are only useful in fully static configurations,
>> +and in those configurations, the domU device tree dynamically generated by Xen
>> +is not needed.
>> +
>> +To enable the event-channel interface for domU guests include the
>> +"xen,enhanced = "no-xenstore"" property in the domU Xen device tree node.
> 
> double ""

Ack. 
> 
> 
>> +
>> +Under the "xen,domain" compatible node for domU, there needs to be sub-nodes
>> +with compatible "xen,evtchn" that describe the event channel connection
>> +between two domUs. For dom0, there needs to be sub-nodes with compatible
>> +"xen,evtchn" under the chosen node.
>> +
>> +The static event channel 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 {
>> +
>> +    /* one sub-node per local event channel */
> 
> It would be good to say that this is for dom0 in the comment, e.g.:
> 
> /* this is for Dom0 */

Ack.

Regards,
Rahul

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

[Rahul Singh]

Hi Stefano,

> On 6 Sep 2022, at 11:12 pm, Stefano Stabellini <sstabellini@kernel.org> wrote:
> 
> On Tue, 6 Sep 2022, 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 v4:
>> - Implement defines for dom0less features
>> 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     | 23 +++++++++++++++++++++--
>> 3 files changed, 31 insertions(+), 6 deletions(-)
>> 
>> diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt
>> index 98253414b8..1b0dca1454 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 but xenstore
>> +    will be disabled for the VM.
> 
> Please use "" for consistency:
> 
>    - "no-xenstore"
> 

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 707e247f6a..0b164ef595 100644
>> --- a/xen/arch/arm/domain_build.c
>> +++ b/xen/arch/arm/domain_build.c
>> @@ -2891,7 +2891,7 @@ static int __init prepare_dtb_domU(struct domain *d, struct kernel_info *kinfo)
>>             goto err;
>>     }
>> 
>> -    if ( kinfo->dom0less_enhanced )
>> +    if ( kinfo->dom0less_feature & DOM0LESS_ENHANCED_NO_XS )
>>     {
>>         ret = make_hypervisor_node(d, kinfo, addrcells, sizecells);
>>         if ( ret )
>> @@ -3209,10 +3209,12 @@ static int __init construct_domU(struct domain *d,
>>          (rc == 0 && !strcmp(dom0less_enhanced, "enabled")) )
>>     {
>>         if ( hardware_domain )
>> -            kinfo.dom0less_enhanced = true;
>> +            kinfo.dom0less_feature = DOM0LESS_ENHANCED;
>>         else
>> -            panic("Tried to use xen,enhanced without dom0\n");
>> +            panic("At the moment, Xenstore support requires dom0 to be present\n");
>>     }
>> +    else if ( rc == 0 && !strcmp(dom0less_enhanced, "no-xenstore") )
>> +        kinfo.dom0less_feature = DOM0LESS_ENHANCED_NO_XS;
>> 
>>     if ( vcpu_create(d, 0) == NULL )
>>         return -ENOMEM;
>> @@ -3252,7 +3254,7 @@ static int __init construct_domU(struct domain *d,
>>     if ( rc < 0 )
>>         return rc;
>> 
>> -    if ( kinfo.dom0less_enhanced )
>> +    if ( kinfo.dom0less_feature & 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..ad240494ea 100644
>> --- a/xen/arch/arm/include/asm/kernel.h
>> +++ b/xen/arch/arm/include/asm/kernel.h
>> @@ -9,6 +9,25 @@
>> #include <xen/device_tree.h>
>> #include <asm/setup.h>
>> 
>> +/*
>> + * List of possible features for dom0less domUs
>> + *
>> + * DOM0LESS_ENHANCED_NO_XS: 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_NO_XS.
>> + * 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_NO_XS  BIT(0, U)
>> +#define DOM0LESS_XENSTORE        BIT(1, U)
>> +#define DOM0LESS_ENHANCED        (DOM0LESS_ENHANCED_NO_XS | DOM0LESS_XENSTORE)
>> +
>> struct kernel_info {
>> #ifdef CONFIG_ARM_64
>>     enum domain_type type;
>> @@ -36,8 +55,8 @@ struct kernel_info {
>>     /* Enable pl011 emulation */
>>     bool vpl011;
>> 
>> -    /* Enable PV drivers */
>> -    bool dom0less_enhanced;
>> +    /* Enable/Disable PV drivers interface,grant table, evtchn or xenstore */
> 
> missing a whitespace

Ack. 
> 
> 
>> +    uint32_t dom0less_feature;
> 
> Given that we only really need 2 bits today, and given that uint8_t and
> uint16_t are free but uint32_t increases the size of the struct, could
> we just use uint16_t dom0less_feature ?

Yes, I will change to uint16_t in next version.

Regards,
Rahul

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

[Julien Grall]

On 06/09/2022 14:40, 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

To me, domUs and guests mean the same. So s/guests//

> physical interrupts to event channels. The only use of the evtchn port
> is inter-domain communications. Another reason why we choose the value
> of 1023 to follow the default behavior of libxl.
> 
> Signed-off-by: Rahul Singh <rahul.singh@arm.com>
> Reviewed-by: Michal Orzel <michal.orzel@amd.com>
> ---
> Changes in v4:
>   - fix minor comments in commit msg
>   - Added Michal Reviewed-by
> 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

To me, domUs and guests mean the same. So s/guests//

Same here. With that:

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

Cheers,

-- 
Julien Grall

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

[Julien Grall]

Hi Rahul

On 06/09/2022 14:40, 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 v4:
>   - Implement defines for dom0less features
> 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     | 23 +++++++++++++++++++++--
>   3 files changed, 31 insertions(+), 6 deletions(-)
> 
> diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt
> index 98253414b8..1b0dca1454 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 but xenstore

Please use "All default" in front. So it is clear that everything is 
enabled 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 707e247f6a..0b164ef595 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -2891,7 +2891,7 @@ static int __init prepare_dtb_domU(struct domain *d, struct kernel_info *kinfo)
>               goto err;
>       }
>   
> -    if ( kinfo->dom0less_enhanced )
> +    if ( kinfo->dom0less_feature & DOM0LESS_ENHANCED_NO_XS )
>       {
>           ret = make_hypervisor_node(d, kinfo, addrcells, sizecells);
>           if ( ret )
> @@ -3209,10 +3209,12 @@ static int __init construct_domU(struct domain *d,
>            (rc == 0 && !strcmp(dom0less_enhanced, "enabled")) )
>       {
>           if ( hardware_domain )
> -            kinfo.dom0less_enhanced = true;
> +            kinfo.dom0less_feature = DOM0LESS_ENHANCED;
>           else
> -            panic("Tried to use xen,enhanced without dom0\n");
> +            panic("At the moment, Xenstore support requires dom0 to be present\n");
>       }
> +    else if ( rc == 0 && !strcmp(dom0less_enhanced, "no-xenstore") )
> +        kinfo.dom0less_feature = DOM0LESS_ENHANCED_NO_XS;
>   
>       if ( vcpu_create(d, 0) == NULL )
>           return -ENOMEM;
> @@ -3252,7 +3254,7 @@ static int __init construct_domU(struct domain *d,
>       if ( rc < 0 )
>           return rc;
>   
> -    if ( kinfo.dom0less_enhanced )
> +    if ( kinfo.dom0less_feature & 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..ad240494ea 100644
> --- a/xen/arch/arm/include/asm/kernel.h
> +++ b/xen/arch/arm/include/asm/kernel.h
> @@ -9,6 +9,25 @@
>   #include <xen/device_tree.h>
>   #include <asm/setup.h>
>   
> +/*
> + * List of possible features for dom0less domUs
> + *
> + * DOM0LESS_ENHANCED_NO_XS: 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_NO_XS.
> + * 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_NO_XS  BIT(0, U)
> +#define DOM0LESS_XENSTORE        BIT(1, U)
> +#define DOM0LESS_ENHANCED        (DOM0LESS_ENHANCED_NO_XS | DOM0LESS_XENSTORE)
> +
>   struct kernel_info {
>   #ifdef CONFIG_ARM_64
>       enum domain_type type;
> @@ -36,8 +55,8 @@ struct kernel_info {
>       /* Enable pl011 emulation */
>       bool vpl011;
>   
> -    /* Enable PV drivers */
> -    bool dom0less_enhanced;
> +    /* Enable/Disable PV drivers interface,grant table, evtchn or xenstore */

The part after "," is technically wrong because it also affects other 
interfaces. But I would drop it to avoid any stale comment (we may add 
new one in the futures).

Cheers,

-- 
Julien Grall

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

[Rahul Singh]

Hi Julien,

> On 7 Sep 2022, at 2:09 pm, Julien Grall <julien@xen.org> wrote:
> 
> Hi Rahul
> 
> On 06/09/2022 14:40, 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 v4:
>>  - Implement defines for dom0less features
>> 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     | 23 +++++++++++++++++++++--
>>  3 files changed, 31 insertions(+), 6 deletions(-)
>> diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt
>> index 98253414b8..1b0dca1454 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 but xenstore
> 
> Please use "All default" in front. So it is clear that everything is enabled but xenstore.

Ack. 
> 
>> +    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 707e247f6a..0b164ef595 100644
>> --- a/xen/arch/arm/domain_build.c
>> +++ b/xen/arch/arm/domain_build.c
>> @@ -2891,7 +2891,7 @@ static int __init prepare_dtb_domU(struct domain *d, struct kernel_info *kinfo)
>>              goto err;
>>      }
>>  -    if ( kinfo->dom0less_enhanced )
>> +    if ( kinfo->dom0less_feature & DOM0LESS_ENHANCED_NO_XS )
>>      {
>>          ret = make_hypervisor_node(d, kinfo, addrcells, sizecells);
>>          if ( ret )
>> @@ -3209,10 +3209,12 @@ static int __init construct_domU(struct domain *d,
>>           (rc == 0 && !strcmp(dom0less_enhanced, "enabled")) )
>>      {
>>          if ( hardware_domain )
>> -            kinfo.dom0less_enhanced = true;
>> +            kinfo.dom0less_feature = DOM0LESS_ENHANCED;
>>          else
>> -            panic("Tried to use xen,enhanced without dom0\n");
>> +            panic("At the moment, Xenstore support requires dom0 to be present\n");
>>      }
>> +    else if ( rc == 0 && !strcmp(dom0less_enhanced, "no-xenstore") )
>> +        kinfo.dom0less_feature = DOM0LESS_ENHANCED_NO_XS;
>>        if ( vcpu_create(d, 0) == NULL )
>>          return -ENOMEM;
>> @@ -3252,7 +3254,7 @@ static int __init construct_domU(struct domain *d,
>>      if ( rc < 0 )
>>          return rc;
>>  -    if ( kinfo.dom0less_enhanced )
>> +    if ( kinfo.dom0less_feature & 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..ad240494ea 100644
>> --- a/xen/arch/arm/include/asm/kernel.h
>> +++ b/xen/arch/arm/include/asm/kernel.h
>> @@ -9,6 +9,25 @@
>>  #include <xen/device_tree.h>
>>  #include <asm/setup.h>
>>  +/*
>> + * List of possible features for dom0less domUs
>> + *
>> + * DOM0LESS_ENHANCED_NO_XS: 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_NO_XS.
>> + * 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_NO_XS  BIT(0, U)
>> +#define DOM0LESS_XENSTORE        BIT(1, U)
>> +#define DOM0LESS_ENHANCED        (DOM0LESS_ENHANCED_NO_XS | DOM0LESS_XENSTORE)
>> +
>>  struct kernel_info {
>>  #ifdef CONFIG_ARM_64
>>      enum domain_type type;
>> @@ -36,8 +55,8 @@ struct kernel_info {
>>      /* Enable pl011 emulation */
>>      bool vpl011;
>>  -    /* Enable PV drivers */
>> -    bool dom0less_enhanced;
>> +    /* Enable/Disable PV drivers interface,grant table, evtchn or xenstore */
> 
> The part after "," is technically wrong because it also affects other interfaces. But I would drop it to avoid any stale comment (we may add new one in the futures).

Ok . I will remove and will comment like this:
/* Enable/Disable PV drivers interfaces */
 
Regards,
Rahul

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

[Rahul Singh]

Hi Julien,

> On 7 Sep 2022, at 2:01 pm, Julien Grall <julien@xen.org> wrote:
> 
> 
> 
> On 06/09/2022 14:40, 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
> 
> To me, domUs and guests mean the same. So s/guests//

Ack. 
> 
>> physical interrupts to event channels. The only use of the evtchn port
>> is inter-domain communications. Another reason why we choose the value
>> of 1023 to follow the default behavior of libxl.
>> Signed-off-by: Rahul Singh <rahul.singh@arm.com>
>> Reviewed-by: Michal Orzel <michal.orzel@amd.com>
>> ---
>> Changes in v4:
>>  - fix minor comments in commit msg
>>  - Added Michal Reviewed-by
>> 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
> 
> To me, domUs and guests mean the same. So s/guests//
> 
> Same here. With that:
> 
> Acked-by: Julien Grall <jgrall@amazon.com>
> 
> Cheers,

Ack. 

Regards,
Rahul