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

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

[Rahul Singh]

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

This patch series depends on patch series [2] to create event channel in Xen. 

[1] https://lists.xenproject.org/archives/html/xen-devel/2022-05/msg01160.html
[2] https://patchwork.kernel.org/project/xen-devel/list/?series=646289

Rahul Singh (8):
  xen/evtchn: make evtchn_bind_interdomain global
  xen/evtchn: modify evtchn_alloc_unbound to allocate specified port
  xen/evtchn: modify evtchn_bind_interdomain to allocate specified port
  xen/evtchn: modify evtchn_bind_interdomain to pass domain as argument
  xen/evtchn: don't close the static event channel.
  xen/evtchn: don't set notification in evtchn_bind_interdomain()
  xen: introduce xen-evtchn dom0less property
  xen/arm: introduce new xen,enhanced property value

 docs/misc/arm/device-tree/booting.txt |  62 +++++-
 xen/arch/arm/domain_build.c           | 290 +++++++++++++++++++-------
 xen/arch/arm/include/asm/domain.h     |   1 +
 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            |  68 ++++--
 xen/include/xen/event.h               |   8 +-
 xen/include/xen/sched.h               |   1 +
 9 files changed, 351 insertions(+), 85 deletions(-)

-- 
2.25.1

[PATCH 1/8] xen/evtchn: make evtchn_bind_interdomain global

[Rahul Singh]

Event channel support will be added for dom0less domains to allocate
static event channel. 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.

Signed-off-by: Rahul Singh <rahul.singh@arm.com>
---
 xen/common/event_channel.c | 2 +-
 xen/include/xen/event.h    | 3 +++
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
index e60cd98d75..8cbe9681da 100644
--- a/xen/common/event_channel.c
+++ b/xen/common/event_channel.c
@@ -347,7 +347,7 @@ 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)
+int evtchn_bind_interdomain(evtchn_bind_interdomain_t *bind)
 {
     struct evtchn *lchn, *rchn;
     struct domain *ld = current->domain, *rd;
diff --git a/xen/include/xen/event.h b/xen/include/xen/event.h
index f3021fe304..61615ebbe3 100644
--- a/xen/include/xen/event.h
+++ b/xen/include/xen/event.h
@@ -74,6 +74,9 @@ 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);
 
+/* Bind an event channel port to interdomain */
+int __must_check evtchn_bind_interdomain(evtchn_bind_interdomain_t *bind);
+
 /* Unmask a local event-channel port. */
 int evtchn_unmask(unsigned int port);
 
-- 
2.25.1

[PATCH 2/8] xen/evtchn: modify evtchn_alloc_unbound to allocate specified port

[Rahul Singh]

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>
---
 xen/arch/arm/domain_build.c |  2 +-
 xen/common/event_channel.c  | 26 +++++++++++++++++++++-----
 xen/include/xen/event.h     |  3 ++-
 3 files changed, 24 insertions(+), 7 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 7ddd16c26d..5f97d9d181 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 8cbe9681da..80a88c1544 100644
--- a/xen/common/event_channel.c
+++ b/xen/common/event_channel.c
@@ -290,11 +290,15 @@ void evtchn_free(struct domain *d, struct evtchn *chn)
     xsm_evtchn_close_post(chn);
 }
 
-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);
@@ -303,8 +307,20 @@ int evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc)
 
     spin_lock(&d->event_lock);
 
-    if ( (port = get_free_port(d)) < 0 )
-        ERROR_EXIT_DOM(port, d);
+    if ( port != 0 )
+    {
+        if ( (rc = evtchn_allocate_port(d, port)) != 0 )
+            ERROR_EXIT_DOM(rc, d);
+    }
+    else
+    {
+        int alloc_port = get_free_port(d);
+
+        if ( alloc_port < 0 )
+            ERROR_EXIT_DOM(alloc_port, d);
+        port = alloc_port;
+    }
+
     chn = evtchn_from_port(d, port);
 
     rc = xsm_evtchn_unbound(XSM_TARGET, d, chn, alloc->remote_dom);
@@ -1206,7 +1222,7 @@ long cf_check 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 61615ebbe3..48820e393e 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);
 
 /* Bind an event channel port to interdomain */
 int __must_check evtchn_bind_interdomain(evtchn_bind_interdomain_t *bind);
-- 
2.25.1

[PATCH 3/8] xen/evtchn: modify evtchn_bind_interdomain to allocate specified port

[Rahul Singh]

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.

Signed-off-by: Rahul Singh <rahul.singh@arm.com>
---
 xen/common/event_channel.c | 26 +++++++++++++++++++++-----
 xen/include/xen/event.h    |  3 ++-
 2 files changed, 23 insertions(+), 6 deletions(-)

diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
index 80a88c1544..bf5dc2c8ad 100644
--- a/xen/common/event_channel.c
+++ b/xen/common/event_channel.c
@@ -363,11 +363,16 @@ static void double_evtchn_unlock(struct evtchn *lchn, struct evtchn *rchn)
     evtchn_write_unlock(rchn);
 }
 
-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,
+                            evtchn_port_t lport)
 {
     struct evtchn *lchn, *rchn;
     struct domain *ld = current->domain, *rd;
-    int            lport, rc;
+    int            rc;
     evtchn_port_t  rport = bind->remote_port;
     domid_t        rdom = bind->remote_dom;
 
@@ -387,8 +392,19 @@ int evtchn_bind_interdomain(evtchn_bind_interdomain_t *bind)
         spin_lock(&ld->event_lock);
     }
 
-    if ( (lport = get_free_port(ld)) < 0 )
-        ERROR_EXIT(lport);
+    if ( lport != 0 )
+    {
+        if ( (rc = evtchn_allocate_port(ld, lport)) != 0 )
+            ERROR_EXIT_DOM(rc, ld);
+    }
+    else
+    {
+        int alloc_port = get_free_port(ld);
+
+        if ( alloc_port < 0 )
+            ERROR_EXIT_DOM(alloc_port, ld);
+        lport = alloc_port;
+    }
     lchn = evtchn_from_port(ld, lport);
 
     rchn = _evtchn_from_port(rd, rport);
@@ -1232,7 +1248,7 @@ long cf_check 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, 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 48820e393e..6e26879793 100644
--- a/xen/include/xen/event.h
+++ b/xen/include/xen/event.h
@@ -76,7 +76,8 @@ 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);
+int __must_check evtchn_bind_interdomain(evtchn_bind_interdomain_t *bind,
+                                         evtchn_port_t port);
 
 /* Unmask a local event-channel port. */
 int evtchn_unmask(unsigned int port);
-- 
2.25.1

[PATCH 4/8] xen/evtchn: modify evtchn_bind_interdomain to pass domain as argument

[Rahul Singh]

evtchn_bind_interdomain() finds the local domain from "current->domain"
pointer.

evtchn_bind_interdomain() will be called from the XEN to support 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>
---
 xen/common/event_channel.c | 6 +++---
 xen/include/xen/event.h    | 1 +
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
index bf5dc2c8ad..84f0055a5a 100644
--- a/xen/common/event_channel.c
+++ b/xen/common/event_channel.c
@@ -367,11 +367,11 @@ static void double_evtchn_unlock(struct evtchn *lchn, struct evtchn *rchn)
  * 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,
+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;
+    struct domain *rd;
     int            rc;
     evtchn_port_t  rport = bind->remote_port;
     domid_t        rdom = bind->remote_dom;
@@ -1248,7 +1248,7 @@ long cf_check 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, 0);
+        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 6e26879793..8eae9984a9 100644
--- a/xen/include/xen/event.h
+++ b/xen/include/xen/event.h
@@ -77,6 +77,7 @@ int __must_check evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc,
 
 /* 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. */
-- 
2.25.1

[PATCH 5/8] xen/evtchn: don't close the static event channel.

[Rahul Singh]

Guest can request the Xen to close the event channels. Ignore the
request from guest to close the static channels as static event channels
should not be closed.

Add the new bool variable "is_static" in "struct evtchn" to mark the
event channel static when creating the event channel.

Signed-off-by: Rahul Singh <rahul.singh@arm.com>
---
 xen/arch/arm/domain_build.c |  2 +-
 xen/common/event_channel.c  | 15 +++++++++++----
 xen/include/xen/event.h     |  4 ++--
 xen/include/xen/sched.h     |  1 +
 4 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 5f97d9d181..89195b042c 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, 0);
+    rc = evtchn_alloc_unbound(&alloc, 0, false);
     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 84f0055a5a..cedc98ccaf 100644
--- a/xen/common/event_channel.c
+++ b/xen/common/event_channel.c
@@ -294,7 +294,8 @@ void evtchn_free(struct domain *d, struct evtchn *chn)
  * 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)
+int evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc, evtchn_port_t port,
+                         bool is_static)
 {
     struct evtchn *chn;
     struct domain *d;
@@ -330,6 +331,7 @@ int evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc, evtchn_port_t port)
     evtchn_write_lock(chn);
 
     chn->state = ECS_UNBOUND;
+    chn->is_static = is_static;
     if ( (chn->u.unbound.remote_domid = alloc->remote_dom) == DOMID_SELF )
         chn->u.unbound.remote_domid = current->domain->domain_id;
     evtchn_port_init(d, chn);
@@ -368,7 +370,7 @@ static void double_evtchn_unlock(struct evtchn *lchn, struct evtchn *rchn)
  * allocate the specified lport.
  */
 int evtchn_bind_interdomain(evtchn_bind_interdomain_t *bind, struct domain *ld,
-                            evtchn_port_t lport)
+                            evtchn_port_t lport, bool is_static)
 {
     struct evtchn *lchn, *rchn;
     struct domain *rd;
@@ -423,6 +425,7 @@ int evtchn_bind_interdomain(evtchn_bind_interdomain_t *bind, struct domain *ld,
     lchn->u.interdomain.remote_dom  = rd;
     lchn->u.interdomain.remote_port = rport;
     lchn->state                     = ECS_INTERDOMAIN;
+    lchn->is_static                 = is_static;
     evtchn_port_init(ld, lchn);
     
     rchn->u.interdomain.remote_dom  = ld;
@@ -659,6 +662,9 @@ int evtchn_close(struct domain *d1, int port1, bool guest)
         rc = -EINVAL;
         goto out;
     }
+    /* Guest cannot close a static event channel. */
+    if ( chn1->is_static && guest )
+        goto out;
 
     switch ( chn1->state )
     {
@@ -1238,7 +1244,7 @@ long cf_check 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, 0);
+        rc = evtchn_alloc_unbound(&alloc_unbound, 0, false);
         if ( !rc && __copy_to_guest(arg, &alloc_unbound, 1) )
             rc = -EFAULT; /* Cleaning up here would be a mess! */
         break;
@@ -1248,7 +1254,8 @@ long cf_check 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, current->domain, 0);
+        rc = evtchn_bind_interdomain(&bind_interdomain, current->domain,
+                                     0, false);
         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 8eae9984a9..71ad4c5bfd 100644
--- a/xen/include/xen/event.h
+++ b/xen/include/xen/event.h
@@ -73,12 +73,12 @@ 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,
-                                      evtchn_port_t port);
+                                      evtchn_port_t port, bool is_static);
 
 /* 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);
+                                         evtchn_port_t port, bool is_static);
 
 /* Unmask a local event-channel port. */
 int evtchn_unmask(unsigned int port);
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index 463d41ffb6..da823c8091 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -119,6 +119,7 @@ struct evtchn
     unsigned char priority;        /* FIFO event channels only. */
     unsigned short notify_vcpu_id; /* VCPU for local delivery notification */
     uint32_t fifo_lastq;           /* Data for identifying last queue. */
+    bool is_static;                /* Static event channels. */
 
 #ifdef CONFIG_XSM
     union {
-- 
2.25.1

[PATCH 6/8] xen/evtchn: don't set notification in evtchn_bind_interdomain()

[Rahul Singh]

evtchn_bind_interdomain() sets the notification on the local port to
handle the lost notification on remote unbound port.

Static event-channel will be created during domain creation, there is no
need to set the notification as remote domain is not alive.

Signed-off-by: Rahul Singh <rahul.singh@arm.com>
---
 xen/common/event_channel.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
index cedc98ccaf..420d18b986 100644
--- a/xen/common/event_channel.c
+++ b/xen/common/event_channel.c
@@ -435,8 +435,13 @@ int evtchn_bind_interdomain(evtchn_bind_interdomain_t *bind, struct domain *ld,
     /*
      * We may have lost notifications on the remote unbound port. Fix that up
      * here by conservatively always setting a notification on the local port.
+     *
+     * There is no need to set the notification if event channel is created in
+     * Xen because domain is not created at this time and no chance of lost
+     * notification.
      */
-    evtchn_port_set_pending(ld, lchn->notify_vcpu_id, lchn);
+    if ( !is_static )
+        evtchn_port_set_pending(ld, lchn->notify_vcpu_id, lchn);
 
     double_evtchn_unlock(lchn, rchn);
 
-- 
2.25.1

[PATCH 7/8] xen: 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>
---
 docs/misc/arm/device-tree/booting.txt |  62 +++++++++++-
 xen/arch/arm/domain_build.c           | 139 ++++++++++++++++++++++++++
 xen/arch/arm/include/asm/domain.h     |   1 +
 xen/arch/arm/include/asm/setup.h      |   1 +
 xen/arch/arm/setup.c                  |   2 +
 5 files changed, 204 insertions(+), 1 deletion(-)

diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt
index 98253414b8..83e914b505 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,42 @@ 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.
+
+    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 +308,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 +344,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 89195b042c..8925f0d80c 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -3052,6 +3052,144 @@ void __init evtchn_allocate(struct domain *d)
     d->arch.hvm.params[HVM_PARAM_CALLBACK_IRQ] = val;
 }
 
+static int __init allocate_domain_evtchn(const struct dt_device_node *node)
+{
+    const void *prop = NULL;
+    const __be32 *cell;
+    uint32_t len, domU1_port, domU2_port, remote_phandle;
+    const struct dt_device_node *evtchn_node, *remote_node;
+    struct evtchn_alloc_unbound alloc_unbound;
+    struct evtchn_bind_interdomain bind_interdomain;
+    int rc;
+
+    dt_for_each_child_node(node, evtchn_node)
+    {
+        struct domain *d, *d1 = NULL, *d2 = NULL;
+
+        if ( !dt_device_is_compatible(evtchn_node, "xen,evtchn-v1") )
+            continue;
+
+        prop = dt_get_property(evtchn_node, "xen,evtchn", &len);
+        /* If the property is not found, return without errors */
+        if ( !prop )
+            return 0;
+
+        if ( !len )
+        {
+            printk(XENLOG_ERR "xen,evtchn property cannot be empty.\n");
+            return -EINVAL;
+        }
+
+        cell = (const __be32 *)prop;
+        domU1_port = dt_next_cell(1, &cell);
+        remote_phandle = dt_next_cell(1, &cell);
+
+        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;
+        }
+
+        prop = dt_get_property(remote_node, "xen,evtchn", &len);
+        /* If the property is not found, return without errors */
+        if ( !prop )
+            return 0;
+
+        if ( !len )
+        {
+            printk(XENLOG_ERR "xen,evtchn property cannot be empty.\n");
+            return -EINVAL;
+        }
+
+        cell = (const __be32 *)prop;
+        domU2_port = dt_next_cell(1, &cell);
+        remote_phandle = dt_next_cell(1, &cell);
+
+        if ( evtchn_node->phandle != remote_phandle )
+        {
+            printk(XENLOG_ERR "xen,evtchn property is not setup correctly.\n");
+            return -EINVAL;
+        }
+
+        for_each_domain ( d )
+        {
+            if ( d->arch.node == node )
+            {
+                d1 = d;
+                continue;
+            }
+            if ( d->arch.node == dt_get_parent(remote_node) )
+                d2 = d;
+        }
+
+        if ( d1 == NULL )
+        {
+            if ( dt_device_is_compatible(node, "multiboot,kernel") )
+                d1 = hardware_domain;
+            else
+            {
+                printk(XENLOG_ERR "evtchn: could not find domain\n" );
+                return -EINVAL;
+            }
+        }
+
+        if ( d2 == NULL )
+        {
+            if ( dt_device_is_compatible(dt_get_parent(remote_node),
+                                         "multiboot,kernel") )
+                d2 = hardware_domain;
+            else
+            {
+                printk(XENLOG_ERR "evtchn: could not find domain\n" );
+                return -EINVAL;
+            }
+        }
+
+        alloc_unbound.dom = d1->domain_id;
+        alloc_unbound.remote_dom = d2->domain_id;
+
+        rc = evtchn_alloc_unbound(&alloc_unbound, domU1_port, true);
+        if ( rc < 0 && rc != -EBUSY )
+        {
+            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, true);
+        if ( rc < 0 && rc != -EBUSY )
+        {
+            printk(XENLOG_ERR
+                   "evtchn_bind_interdomain() failure (Error %d) \n", rc);
+            return rc;
+        }
+    }
+
+    return 0;
+}
+
+void __init allocate_static_evtchn(void)
+{
+    struct dt_device_node *node;
+    const struct dt_device_node *chosen = dt_find_node_by_path("/chosen");
+
+    BUG_ON(chosen == NULL);
+    dt_for_each_child_node(chosen, node)
+    {
+        if ( dt_device_is_compatible(node, "xen,domain") ||
+             dt_device_is_compatible(node, "multiboot,kernel") )
+        {
+            if ( allocate_domain_evtchn(node) != 0 )
+                panic("Could not set up domains evtchn\n");
+        }
+    }
+}
+
 static void __init find_gnttab_region(struct domain *d,
                                       struct kernel_info *kinfo)
 {
@@ -3358,6 +3496,7 @@ void __init create_domUs(void)
             panic("Error creating domain %s\n", dt_node_name(node));
 
         d->is_console = true;
+        d->arch.node = node;
 
         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/domain.h b/xen/arch/arm/include/asm/domain.h
index ed63c2b6f9..7c22cbabcc 100644
--- a/xen/arch/arm/include/asm/domain.h
+++ b/xen/arch/arm/include/asm/domain.h
@@ -104,6 +104,7 @@ struct arch_domain
 #endif
 
     bool directmap;
+    struct dt_device_node *node;
 }  __cacheline_aligned;
 
 struct arch_vcpu
diff --git a/xen/arch/arm/include/asm/setup.h b/xen/arch/arm/include/asm/setup.h
index 2bb01ecfa8..bac876e68e 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 allocate_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 05d97a1cfb..0936db58b2 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -1046,6 +1046,8 @@ void __init start_xen(unsigned long boot_phys_offset,
     if ( acpi_disabled )
         create_domUs();
 
+    allocate_static_evtchn();
+
     /*
      * This needs to be called **before** heap_init_late() so modules
      * will be scrubbed (unless suppressed).
-- 
2.25.1

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

[Rahul Singh]

Introduce a new "xen,enhanced" dom0less property value "evtchn" to
enable/disable event-channel interfaces for dom0less guests.

The configurable option is for domUs only. For dom0 we always set the
corresponding property in the Xen code to true.

Signed-off-by: Rahul Singh <rahul.singh@arm.com>
---
 xen/arch/arm/domain_build.c       | 149 ++++++++++++++++--------------
 xen/arch/arm/include/asm/kernel.h |   3 +
 2 files changed, 82 insertions(+), 70 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 8925f0d80c..a1c1ab5877 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -1396,85 +1396,92 @@ static int __init make_hypervisor_node(struct domain *d,
     if ( res )
         return res;
 
-    if ( !opt_ext_regions )
-    {
-        printk(XENLOG_INFO "%pd: extended regions support is disabled\n", d);
-        nr_ext_regions = 0;
-    }
-    else if ( is_32bit_domain(d) )
-    {
-        printk(XENLOG_WARNING
-               "%pd: extended regions not supported for 32-bit guests\n", d);
-        nr_ext_regions = 0;
-    }
-    else
+    if ( kinfo->dom0less_enhanced )
     {
-        ext_regions = xzalloc(struct meminfo);
-        if ( !ext_regions )
-            return -ENOMEM;
-
-        if ( is_domain_direct_mapped(d) )
+        if ( !opt_ext_regions )
         {
-            if ( !is_iommu_enabled(d) )
-                res = find_unallocated_memory(kinfo, ext_regions);
-            else
-                res = find_memory_holes(kinfo, ext_regions);
+            printk(XENLOG_INFO
+                   "%pd: extended regions support is disabled\n", d);
+            nr_ext_regions = 0;
         }
-        else
+        else if ( is_32bit_domain(d) )
         {
-            res = find_domU_holes(kinfo, ext_regions);
+            printk(XENLOG_WARNING
+                   "%pd: extended regions not supported for 32-bit guests\n", d);
+            nr_ext_regions = 0;
         }
+        else
+        {
+            ext_regions = xzalloc(struct meminfo);
+            if ( !ext_regions )
+                return -ENOMEM;
 
-        if ( res )
-            printk(XENLOG_WARNING "%pd: failed to allocate extended regions\n",
-                   d);
-        nr_ext_regions = ext_regions->nr_banks;
-    }
+            if ( is_domain_direct_mapped(d) )
+            {
+                if ( !is_iommu_enabled(d) )
+                    res = find_unallocated_memory(kinfo, ext_regions);
+                else
+                    res = find_memory_holes(kinfo, ext_regions);
+            }
+            else
+            {
+                res = find_domU_holes(kinfo, ext_regions);
+            }
 
-    reg = xzalloc_array(__be32, (nr_ext_regions + 1) * (addrcells + sizecells));
-    if ( !reg )
-    {
-        xfree(ext_regions);
-        return -ENOMEM;
-    }
+            if ( res )
+                printk(XENLOG_WARNING
+                       "%pd: failed to allocate extended regions\n", d);
+            nr_ext_regions = ext_regions->nr_banks;
+        }
 
-    /* reg 0 is grant table space */
-    cells = &reg[0];
-    dt_child_set_range(&cells, addrcells, sizecells,
-                       kinfo->gnttab_start, kinfo->gnttab_size);
-    /* reg 1...N are extended regions */
-    for ( i = 0; i < nr_ext_regions; i++ )
-    {
-        u64 start = ext_regions->bank[i].start;
-        u64 size = ext_regions->bank[i].size;
+        reg = xzalloc_array(__be32, (nr_ext_regions + 1) * (addrcells + sizecells));
+        if ( !reg )
+        {
+            xfree(ext_regions);
+            return -ENOMEM;
+        }
 
-        printk("%pd: extended region %d: %#"PRIx64"->%#"PRIx64"\n",
-               d, i, start, start + size);
+        /* reg 0 is grant table space */
+        cells = &reg[0];
+        dt_child_set_range(&cells, addrcells, sizecells,
+                           kinfo->gnttab_start, kinfo->gnttab_size);
+        /* reg 1...N are extended regions */
+        for ( i = 0; i < nr_ext_regions; i++ )
+        {
+            u64 start = ext_regions->bank[i].start;
+            u64 size = ext_regions->bank[i].size;
 
-        dt_child_set_range(&cells, addrcells, sizecells, start, size);
-    }
+            printk("%pd: extended region %d: %#"PRIx64"->%#"PRIx64"\n",
+                   d, i, start, start + size);
 
-    res = fdt_property(fdt, "reg", reg,
-                       dt_cells_to_size(addrcells + sizecells) *
-                       (nr_ext_regions + 1));
-    xfree(ext_regions);
-    xfree(reg);
+            dt_child_set_range(&cells, addrcells, sizecells, start, size);
+        }
 
-    if ( res )
-        return res;
+        res = fdt_property(fdt, "reg", reg,
+                           dt_cells_to_size(addrcells + sizecells) *
+                           (nr_ext_regions + 1));
+        xfree(ext_regions);
+        xfree(reg);
 
-    BUG_ON(d->arch.evtchn_irq == 0);
+        if ( res )
+            return res;
+    }
 
-    /*
-     * Interrupt event channel upcall:
-     *  - Active-low level-sensitive
-     *  - All CPUs
-     *  TODO: Handle properly the cpumask;
-     */
-    set_interrupt(intr, d->arch.evtchn_irq, 0xf, DT_IRQ_TYPE_LEVEL_LOW);
-    res = fdt_property_interrupts(kinfo, &intr, 1);
-    if ( res )
-        return res;
+    if ( kinfo->dom0less_evtchn )
+    {
+        BUG_ON(d->arch.evtchn_irq == 0);
+
+        /*
+         * Interrupt event channel upcall:
+         *  - Active-low level-sensitive
+         *  - All CPUs
+         *  TODO: Handle properly the cpumask;
+        */
+        set_interrupt(intr, d->arch.evtchn_irq, 0xf, DT_IRQ_TYPE_LEVEL_LOW);
+        res = fdt_property_interrupts(kinfo, &intr, 1);
+        if ( res )
+            return res;
+    }
 
     res = fdt_end_node(fdt);
 
@@ -2891,7 +2898,7 @@ static int __init prepare_dtb_domU(struct domain *d, struct kernel_info *kinfo)
             goto err;
     }
 
-    if ( kinfo->dom0less_enhanced )
+    if ( kinfo->dom0less_enhanced || kinfo->dom0less_evtchn )
     {
         ret = make_hypervisor_node(d, kinfo, addrcells, sizecells);
         if ( ret )
@@ -3346,11 +3353,11 @@ static int __init construct_domU(struct domain *d,
          rc == -ENODATA ||
          (rc == 0 && !strcmp(dom0less_enhanced, "enabled")) )
     {
-        if ( hardware_domain )
-            kinfo.dom0less_enhanced = true;
-        else
-            panic("Tried to use xen,enhanced without dom0\n");
+        kinfo.dom0less_enhanced = true;
+        kinfo.dom0less_evtchn = true;
     }
+    else if ( rc == 0 && !strcmp(dom0less_enhanced, "evtchn") )
+        kinfo.dom0less_evtchn = true;
 
     if ( vcpu_create(d, 0) == NULL )
         return -ENOMEM;
@@ -3529,6 +3536,8 @@ static int __init construct_dom0(struct domain *d)
 
     kinfo.unassigned_mem = dom0_mem;
     kinfo.d = d;
+    kinfo.dom0less_enhanced = true;
+    kinfo.dom0less_evtchn = true;
 
     rc = kernel_probe(&kinfo, NULL);
     if ( rc < 0 )
diff --git a/xen/arch/arm/include/asm/kernel.h b/xen/arch/arm/include/asm/kernel.h
index c4dc039b54..7cff19b997 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 event-channel interface */
+    bool dom0less_evtchn;
+
     /* GIC phandle */
     uint32_t phandle_gic;
 
-- 
2.25.1

Re: [PATCH 2/8] xen/evtchn: modify evtchn_alloc_unbound to allocate specified port

[Julien Grall]

Hi,

On 22/06/2022 15:37, Rahul Singh wrote:
> 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.

I haven't yet fully reviewed this series. But I would like to point out 
that this opening a security hole (which I thought I had mention before) 
that could be exploited by a guest at runtime.

You would need [1] or similar in order to fix the issue. I am wrote 
"similar" because the patch could potentially be a problem if you allow 
a guest to use FIFO (you may need to allocate a lot of memory to fill 
the hole).

Cheers,

[1] 
https://xenbits.xen.org/gitweb/?p=people/julieng/xen-unstable.git;a=commit;h=2d89486fcf11216331e58a21b367b8a9be1af725

-- 
Julien Grall

Re: [PATCH 5/8] xen/evtchn: don't close the static event channel.

[Julien Grall]

Hi,

On 22/06/2022 15:38, Rahul Singh wrote:
> Guest can request the Xen to close the event channels. Ignore the
> request from guest to close the static channels as static event channels
> should not be closed.

Why do you want to prevent the guest to close static ports? The problem 
I can see is...

[...]

> diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
> index 84f0055a5a..cedc98ccaf 100644
> --- a/xen/common/event_channel.c
> +++ b/xen/common/event_channel.c
> @@ -294,7 +294,8 @@ void evtchn_free(struct domain *d, struct evtchn *chn)
>    * 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)
> +int evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc, evtchn_port_t port,
> +                         bool is_static)
>   {
>       struct evtchn *chn;
>       struct domain *d;
> @@ -330,6 +331,7 @@ int evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc, evtchn_port_t port)
>       evtchn_write_lock(chn);
>   
>       chn->state = ECS_UNBOUND;
> +    chn->is_static = is_static;
>       if ( (chn->u.unbound.remote_domid = alloc->remote_dom) == DOMID_SELF )
>           chn->u.unbound.remote_domid = current->domain->domain_id;
>       evtchn_port_init(d, chn);
> @@ -368,7 +370,7 @@ static void double_evtchn_unlock(struct evtchn *lchn, struct evtchn *rchn)
>    * allocate the specified lport.
>    */
>   int evtchn_bind_interdomain(evtchn_bind_interdomain_t *bind, struct domain *ld,
> -                            evtchn_port_t lport)
> +                            evtchn_port_t lport, bool is_static)
>   {
>       struct evtchn *lchn, *rchn;
>       struct domain *rd;
> @@ -423,6 +425,7 @@ int evtchn_bind_interdomain(evtchn_bind_interdomain_t *bind, struct domain *ld,
>       lchn->u.interdomain.remote_dom  = rd;
>       lchn->u.interdomain.remote_port = rport;
>       lchn->state                     = ECS_INTERDOMAIN;
> +    lchn->is_static                 = is_static;
>       evtchn_port_init(ld, lchn);
>       
>       rchn->u.interdomain.remote_dom  = ld;
> @@ -659,6 +662,9 @@ int evtchn_close(struct domain *d1, int port1, bool guest)
>           rc = -EINVAL;
>           goto out;
>       }
> +    /* Guest cannot close a static event channel. */
> +    if ( chn1->is_static && guest )
> +        goto out;

... at least the interdomain structure store pointer to the domain. I am 
a bit concerned that we would end up to leave dangling pointers (such as 
chn->u.interdomain.remote_domain) as evtchn_close() is also used while 
destroying the domain.

Also, AFAICT Xen will return 0 (i.e. success) to the caller. I think 
this is a mistake because we didn't close the port as requested.

Cheers,

-- 
Julien Grall

Re: [PATCH 5/8] xen/evtchn: don't close the static event channel.

[Rahul Singh]

Hi Julien,


> On 22 Jun 2022, at 4:05 pm, Julien Grall <julien@xen.org> wrote:
> 
> Hi,
> 
> On 22/06/2022 15:38, Rahul Singh wrote:
>> Guest can request the Xen to close the event channels. Ignore the
>> request from guest to close the static channels as static event channels
>> should not be closed.
> 
> Why do you want to prevent the guest to close static ports? The problem I can see is...

As a static event channel should be available during the lifetime of the guest we want to prevent
the guest to close the static ports. 

I tested this series to send/receive event notification from the Linux user-space application via "/dev/xen/evtchn” interface and
ioctl ( IOCTL_EVTCHN_*) calls. When we close the "/dev/xen/evtchn” interface Linux event channel
driver will try to close the static event channel also, that why we need this patch to avoid guests to close 
the event channel as we don’t want to close the static event channel.

>  
> [...]
> 
>> diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
>> index 84f0055a5a..cedc98ccaf 100644
>> --- a/xen/common/event_channel.c
>> +++ b/xen/common/event_channel.c
>> @@ -294,7 +294,8 @@ void evtchn_free(struct domain *d, struct evtchn *chn)
>>   * 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)
>> +int evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc, evtchn_port_t port,
>> +                         bool is_static)
>>  {
>>      struct evtchn *chn;
>>      struct domain *d;
>> @@ -330,6 +331,7 @@ int evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc, evtchn_port_t port)
>>      evtchn_write_lock(chn);
>>        chn->state = ECS_UNBOUND;
>> +    chn->is_static = is_static;
>>      if ( (chn->u.unbound.remote_domid = alloc->remote_dom) == DOMID_SELF )
>>          chn->u.unbound.remote_domid = current->domain->domain_id;
>>      evtchn_port_init(d, chn);
>> @@ -368,7 +370,7 @@ static void double_evtchn_unlock(struct evtchn *lchn, struct evtchn *rchn)
>>   * allocate the specified lport.
>>   */
>>  int evtchn_bind_interdomain(evtchn_bind_interdomain_t *bind, struct domain *ld,
>> -                            evtchn_port_t lport)
>> +                            evtchn_port_t lport, bool is_static)
>>  {
>>      struct evtchn *lchn, *rchn;
>>      struct domain *rd;
>> @@ -423,6 +425,7 @@ int evtchn_bind_interdomain(evtchn_bind_interdomain_t *bind, struct domain *ld,
>>      lchn->u.interdomain.remote_dom  = rd;
>>      lchn->u.interdomain.remote_port = rport;
>>      lchn->state                     = ECS_INTERDOMAIN;
>> +    lchn->is_static                 = is_static;
>>      evtchn_port_init(ld, lchn);
>>            rchn->u.interdomain.remote_dom  = ld;
>> @@ -659,6 +662,9 @@ int evtchn_close(struct domain *d1, int port1, bool guest)
>>          rc = -EINVAL;
>>          goto out;
>>      }
>> +    /* Guest cannot close a static event channel. */
>> +    if ( chn1->is_static && guest )
>> +        goto out;
> 
> ... at least the interdomain structure store pointer to the domain. I am a bit concerned that we would end up to leave dangling pointers (such as chn->u.interdomain.remote_domain) as evtchn_close() is also used while destroying the domain.

Let me have a look again if we have to do the cleanup when we destroy the guest and close the static event channel.
> 
> Also, AFAICT Xen will return 0 (i.e. success) to the caller. I think this is a mistake because we didn't close the port as requested.

If we return non-zero to guest (in particular if linux guest), Linux will report the BUG(). Therefore I decided to return 0. 

if (HYPERVISOR_event_channel_op(EVTCHNOP_close, &close) != 0)               
        BUG();

Regards,
Rahul

Re: [PATCH 5/8] xen/evtchn: don't close the static event channel.

[Julien Grall]

On 23/06/2022 16:10, Rahul Singh wrote:
> Hi Julien,
> 
> 
>> On 22 Jun 2022, at 4:05 pm, Julien Grall <julien@xen.org> wrote:
>>
>> Hi,
>>
>> On 22/06/2022 15:38, Rahul Singh wrote:
>>> Guest can request the Xen to close the event channels. Ignore the
>>> request from guest to close the static channels as static event channels
>>> should not be closed.
>>
>> Why do you want to prevent the guest to close static ports? The problem I can see is...
> 
> As a static event channel should be available during the lifetime of the guest we want to prevent
> the guest to close the static ports.
I don't think it is Xen job to prevent a guest to close a static port. 
If the guest decide to do it, then it will just break itself and not Xen.

> 
> I tested this series to send/receive event notification from the Linux user-space application via "/dev/xen/evtchn” interface and
> ioctl ( IOCTL_EVTCHN_*) calls. When we close the "/dev/xen/evtchn” interface Linux event channel
> driver will try to close the static event channel also, that why we need this patch to avoid guests to close
> the event channel as we don’t want to close the static event channel.

To me, this reads as Linux should be modified in order to avoid closing 
static event channel. In fact...

>>   
>> [...]
>>
>>> diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
>>> index 84f0055a5a..cedc98ccaf 100644
>>> --- a/xen/common/event_channel.c
>>> +++ b/xen/common/event_channel.c
>>> @@ -294,7 +294,8 @@ void evtchn_free(struct domain *d, struct evtchn *chn)
>>>    * 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)
>>> +int evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc, evtchn_port_t port,
>>> +                         bool is_static)
>>>   {
>>>       struct evtchn *chn;
>>>       struct domain *d;
>>> @@ -330,6 +331,7 @@ int evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc, evtchn_port_t port)
>>>       evtchn_write_lock(chn);
>>>         chn->state = ECS_UNBOUND;
>>> +    chn->is_static = is_static;
>>>       if ( (chn->u.unbound.remote_domid = alloc->remote_dom) == DOMID_SELF )
>>>           chn->u.unbound.remote_domid = current->domain->domain_id;
>>>       evtchn_port_init(d, chn);
>>> @@ -368,7 +370,7 @@ static void double_evtchn_unlock(struct evtchn *lchn, struct evtchn *rchn)
>>>    * allocate the specified lport.
>>>    */
>>>   int evtchn_bind_interdomain(evtchn_bind_interdomain_t *bind, struct domain *ld,
>>> -                            evtchn_port_t lport)
>>> +                            evtchn_port_t lport, bool is_static)
>>>   {
>>>       struct evtchn *lchn, *rchn;
>>>       struct domain *rd;
>>> @@ -423,6 +425,7 @@ int evtchn_bind_interdomain(evtchn_bind_interdomain_t *bind, struct domain *ld,
>>>       lchn->u.interdomain.remote_dom  = rd;
>>>       lchn->u.interdomain.remote_port = rport;
>>>       lchn->state                     = ECS_INTERDOMAIN;
>>> +    lchn->is_static                 = is_static;
>>>       evtchn_port_init(ld, lchn);
>>>             rchn->u.interdomain.remote_dom  = ld;
>>> @@ -659,6 +662,9 @@ int evtchn_close(struct domain *d1, int port1, bool guest)
>>>           rc = -EINVAL;
>>>           goto out;
>>>       }
>>> +    /* Guest cannot close a static event channel. */
>>> +    if ( chn1->is_static && guest )
>>> +        goto out;
>>
>> ... at least the interdomain structure store pointer to the domain. I am a bit concerned that we would end up to leave dangling pointers (such as chn->u.interdomain.remote_domain) as evtchn_close() is also used while destroying the domain.
> 
> Let me have a look again if we have to do the cleanup when we destroy the guest and close the static event channel.
>>
>> Also, AFAICT Xen will return 0 (i.e. success) to the caller. I think this is a mistake because we didn't close the port as requested.
> 
> If we return non-zero to guest (in particular if linux guest), Linux will report the BUG(). Therefore I decided to return 0.

... this shows that we are papering over a bigger problem: Linux is not 
ready for static event channels.

> 
> if (HYPERVISOR_event_channel_op(EVTCHNOP_close, &close) != 0)
>          BUG();
The BUG() in Linux is definitely not a reason to lie and claim the port 
was closed.

If you tell that to an OS, it may validly think that it know need to 
call bind interdomain in order to "re-open" the port. So your Linux will 
already need some information to know that the port is "static".

At which point, you can modify Linux to also prevent the port to be closed.

Cheers,

-- 
Julien Grall

Re: [PATCH 5/8] xen/evtchn: don't close the static event channel.

[Jan Beulich]

On 23.06.2022 17:30, Julien Grall wrote:
> On 23/06/2022 16:10, Rahul Singh wrote:
>>> On 22 Jun 2022, at 4:05 pm, Julien Grall <julien@xen.org> wrote:
>>> On 22/06/2022 15:38, Rahul Singh wrote:
>>>> Guest can request the Xen to close the event channels. Ignore the
>>>> request from guest to close the static channels as static event channels
>>>> should not be closed.
>>>
>>> Why do you want to prevent the guest to close static ports? The problem I can see is...
>>
>> As a static event channel should be available during the lifetime of the guest we want to prevent
>> the guest to close the static ports.
> I don't think it is Xen job to prevent a guest to close a static port. 
> If the guest decide to do it, then it will just break itself and not Xen.

+1, fwiw.

Jan

Re: [PATCH 5/8] xen/evtchn: don't close the static event channel.

[Rahul Singh]

Hi Julien

> On 23 Jun 2022, at 4:30 pm, Julien Grall <julien@xen.org> wrote:
> 
> 
> 
> On 23/06/2022 16:10, Rahul Singh wrote:
>> Hi Julien,
>>> On 22 Jun 2022, at 4:05 pm, Julien Grall <julien@xen.org> wrote:
>>> 
>>> Hi,
>>> 
>>> On 22/06/2022 15:38, Rahul Singh wrote:
>>>> Guest can request the Xen to close the event channels. Ignore the
>>>> request from guest to close the static channels as static event channels
>>>> should not be closed.
>>> 
>>> Why do you want to prevent the guest to close static ports? The problem I can see is...
>> As a static event channel should be available during the lifetime of the guest we want to prevent
>> the guest to close the static ports.
> I don't think it is Xen job to prevent a guest to close a static port. If the guest decide to do it, then it will just break itself and not Xen.

It is okay for the guest to close a port, port is not allocated by the guest in case of a static event channel. 
Xen has nothing to do for close the static event channel and just return 0.

Regards,
Rahul
 

Re: [PATCH 5/8] xen/evtchn: don't close the static event channel.

[Julien Grall]

On 28/06/2022 14:53, Rahul Singh wrote:
> Hi Julien

Hi Rahul,

>> On 23 Jun 2022, at 4:30 pm, Julien Grall <julien@xen.org> wrote:
>>
>>
>>
>> On 23/06/2022 16:10, Rahul Singh wrote:
>>> Hi Julien,
>>>> On 22 Jun 2022, at 4:05 pm, Julien Grall <julien@xen.org> wrote:
>>>>
>>>> Hi,
>>>>
>>>> On 22/06/2022 15:38, Rahul Singh wrote:
>>>>> Guest can request the Xen to close the event channels. Ignore the
>>>>> request from guest to close the static channels as static event channels
>>>>> should not be closed.
>>>>
>>>> Why do you want to prevent the guest to close static ports? The problem I can see is...
>>> As a static event channel should be available during the lifetime of the guest we want to prevent
>>> the guest to close the static ports.
>> I don't think it is Xen job to prevent a guest to close a static port. If the guest decide to do it, then it will just break itself and not Xen.
> 
> It is okay for the guest to close a port, port is not allocated by the guest in case of a static event channel.
As I wrote before, the OS will need to know that the port is statically 
allocated when initializing the port (we don't want to call the 
hypercall to bind the event channel). By extend, the OS should be able 
to know that when closing it and skip the hypercall.

> Xen has nothing to do for close the static event channel and just return 0.

Xen would not need to be modified if the OS was doing the right (i.e. no 
calling close).

So it is still unclear why papering over the issue in Xen is the best 
solution.

Cheers,

-- 
Julien Grall

Re: [PATCH 5/8] xen/evtchn: don't close the static event channel.

[Bertrand Marquis]

Hi Julien,

> On 28 Jun 2022, at 15:26, Julien Grall <julien@xen.org> wrote:
> 
> 
> 
> On 28/06/2022 14:53, Rahul Singh wrote:
>> Hi Julien
> 
> Hi Rahul,
> 
>>> On 23 Jun 2022, at 4:30 pm, Julien Grall <julien@xen.org> wrote:
>>> 
>>> 
>>> 
>>> On 23/06/2022 16:10, Rahul Singh wrote:
>>>> Hi Julien,
>>>>> On 22 Jun 2022, at 4:05 pm, Julien Grall <julien@xen.org> wrote:
>>>>> 
>>>>> Hi,
>>>>> 
>>>>> On 22/06/2022 15:38, Rahul Singh wrote:
>>>>>> Guest can request the Xen to close the event channels. Ignore the
>>>>>> request from guest to close the static channels as static event channels
>>>>>> should not be closed.
>>>>> 
>>>>> Why do you want to prevent the guest to close static ports? The problem I can see is...
>>>> As a static event channel should be available during the lifetime of the guest we want to prevent
>>>> the guest to close the static ports.
>>> I don't think it is Xen job to prevent a guest to close a static port. If the guest decide to do it, then it will just break itself and not Xen.
>> It is okay for the guest to close a port, port is not allocated by the guest in case of a static event channel.
> As I wrote before, the OS will need to know that the port is statically allocated when initializing the port (we don't want to call the hypercall to bind the event channel). By extend, the OS should be able to know that when closing it and skip the hypercall.
> 
>> Xen has nothing to do for close the static event channel and just return 0.
> 
> Xen would not need to be modified if the OS was doing the right (i.e. no calling close).
> 
> So it is still unclear why papering over the issue in Xen is the best solution.

It is not that a static event channel cannot be closed, it is just that during a close there is nothing to do for Xen as the event channel is static and hence is never removed so none of the operations to be done for a non static one are needed (maybe some day some will be, who knows).

Why requiring the OS to have the knowledge of the fact that an event channel is static or not and introduce some complexity on guest code if we can prevent it ?

Doing so would need to have a specific binding in device tree (not to mention the issue on ACPI), a new driver in linux kernel, etc where right now we just need to introduce an extra IOCTL in linux to support this feature.

Cheers
Bertrand

Re: [PATCH 5/8] xen/evtchn: don't close the static event channel.

[Julien Grall]

On 28/06/2022 15:52, Bertrand Marquis wrote:
> Hi Julien,
> 
>> On 28 Jun 2022, at 15:26, Julien Grall <julien@xen.org> wrote:
>>
>>
>>
>> On 28/06/2022 14:53, Rahul Singh wrote:
>>> Hi Julien
>>
>> Hi Rahul,
>>
>>>> On 23 Jun 2022, at 4:30 pm, Julien Grall <julien@xen.org> wrote:
>>>>
>>>>
>>>>
>>>> On 23/06/2022 16:10, Rahul Singh wrote:
>>>>> Hi Julien,
>>>>>> On 22 Jun 2022, at 4:05 pm, Julien Grall <julien@xen.org> wrote:
>>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> On 22/06/2022 15:38, Rahul Singh wrote:
>>>>>>> Guest can request the Xen to close the event channels. Ignore the
>>>>>>> request from guest to close the static channels as static event channels
>>>>>>> should not be closed.
>>>>>>
>>>>>> Why do you want to prevent the guest to close static ports? The problem I can see is...
>>>>> As a static event channel should be available during the lifetime of the guest we want to prevent
>>>>> the guest to close the static ports.
>>>> I don't think it is Xen job to prevent a guest to close a static port. If the guest decide to do it, then it will just break itself and not Xen.
>>> It is okay for the guest to close a port, port is not allocated by the guest in case of a static event channel.
>> As I wrote before, the OS will need to know that the port is statically allocated when initializing the port (we don't want to call the hypercall to bind the event channel). By extend, the OS should be able to know that when closing it and skip the hypercall.
>>
>>> Xen has nothing to do for close the static event channel and just return 0.
>>
>> Xen would not need to be modified if the OS was doing the right (i.e. no calling close).
>>
>> So it is still unclear why papering over the issue in Xen is the best solution.
> 
> It is not that a static event channel cannot be closed, it is just that during a close there is nothing to do for Xen as the event channel is static and hence is never removed so none of the operations to be done for a non static one are needed (maybe some day some will be, who knows).

I feel there are some disagreement on the meaning of "close" here. In 
the context of event channel, "close" means that the port is marked as 
ECS_FREE.

So I think this is wrong to say that there is nothing to do for "close" 
because after this operation the port will still be "open" (the port 
state will be ECS_INTERDOMAIN).

In fact, to me, a "static" port is the same as if the event channel was 
allocated from the toolstack (for instance this is the case for 
Xenstored). In such case, we are still allowing the guest to close the 
port and then re-opening. So I don't really see why we should diverge here.

> 
> Why requiring the OS to have the knowledge of the fact that an event channel is static or not and introduce some complexity on guest code if we can prevent it ?

I am confused. Your OS already need to know that this is a static port 
(so it doesn't call the hypercall to "open" the port). So why is it a 
non-issue for "opening" but one for "closing" ?

> 
> Doing so would need to have a specific binding in device tree (not to mention the issue on ACPI), 

You already need to create a Device-Tree binding to expose the static 
event-channel. So why is this a new problem?

Likewise for ACPI, you already have this issue with your current proposal.

> a new driver in linux kernel, etc where right now we just need to introduce an extra IOCTL in linux to support this feature.

I don't understand why would need a new driver, etc. Given that you are 
introducing a new IOCTL you could pass a flag to say "This is a static 
event channel so don't close it".

Cheers,

-- 
Julien Grall

Re: [PATCH 5/8] xen/evtchn: don't close the static event channel.

[Jan Beulich]

On 28.06.2022 17:18, Julien Grall wrote:
> In fact, to me, a "static" port is the same as if the event channel was 
> allocated from the toolstack (for instance this is the case for 
> Xenstored). In such case, we are still allowing the guest to close the 
> port and then re-opening. So I don't really see why we should diverge here.

Fwiw, I agree with Julien's view here.

Jan

Re: [PATCH 5/8] xen/evtchn: don't close the static event channel.

[Rahul Singh]

Hi Julien,


> On 28 Jun 2022, at 4:18 pm, Julien Grall <julien@xen.org> wrote:
> 
> 
> 
> On 28/06/2022 15:52, Bertrand Marquis wrote:
>> Hi Julien,
>>> On 28 Jun 2022, at 15:26, Julien Grall <julien@xen.org> wrote:
>>> 
>>> 
>>> 
>>> On 28/06/2022 14:53, Rahul Singh wrote:
>>>> Hi Julien
>>> 
>>> Hi Rahul,
>>> 
>>>>> On 23 Jun 2022, at 4:30 pm, Julien Grall <julien@xen.org> wrote:
>>>>> 
>>>>> 
>>>>> 
>>>>> On 23/06/2022 16:10, Rahul Singh wrote:
>>>>>> Hi Julien,
>>>>>>> On 22 Jun 2022, at 4:05 pm, Julien Grall <julien@xen.org> wrote:
>>>>>>> 
>>>>>>> Hi,
>>>>>>> 
>>>>>>> On 22/06/2022 15:38, Rahul Singh wrote:
>>>>>>>> Guest can request the Xen to close the event channels. Ignore the
>>>>>>>> request from guest to close the static channels as static event channels
>>>>>>>> should not be closed.
>>>>>>> 
>>>>>>> Why do you want to prevent the guest to close static ports? The problem I can see is...
>>>>>> As a static event channel should be available during the lifetime of the guest we want to prevent
>>>>>> the guest to close the static ports.
>>>>> I don't think it is Xen job to prevent a guest to close a static port. If the guest decide to do it, then it will just break itself and not Xen.
>>>> It is okay for the guest to close a port, port is not allocated by the guest in case of a static event channel.
>>> As I wrote before, the OS will need to know that the port is statically allocated when initializing the port (we don't want to call the hypercall to bind the event channel). By extend, the OS should be able to know that when closing it and skip the hypercall.
>>> 
>>>> Xen has nothing to do for close the static event channel and just return 0.
>>> 
>>> Xen would not need to be modified if the OS was doing the right (i.e. no calling close).
>>> 
>>> So it is still unclear why papering over the issue in Xen is the best solution.
>> It is not that a static event channel cannot be closed, it is just that during a close there is nothing to do for Xen as the event channel is static and hence is never removed so none of the operations to be done for a non static one are needed (maybe some day some will be, who knows).
> 
> I feel there are some disagreement on the meaning of "close" here. In the context of event channel, "close" means that the port is marked as ECS_FREE.
> 
> So I think this is wrong to say that there is nothing to do for "close" because after this operation the port will still be "open" (the port state will be ECS_INTERDOMAIN).
> 
> In fact, to me, a "static" port is the same as if the event channel was allocated from the toolstack (for instance this is the case for Xenstored). In such case, we are still allowing the guest to close the port and then re-opening. So I don't really see why we should diverge here.
> 
>> Why requiring the OS to have the knowledge of the fact that an event channel is static or not and introduce some complexity on guest code if we can prevent it ?
> 
> I am confused. Your OS already need to know that this is a static port (so it doesn't call the hypercall to "open" the port). So why is it a non-issue for "opening" but one for "closing" ?
> 
>> Doing so would need to have a specific binding in device tree (not to mention the issue on ACPI), 
> 
> You already need to create a Device-Tree binding to expose the static event-channel. So why is this a new problem?
> 
> Likewise for ACPI, you already have this issue with your current proposal.
> 
>> a new driver in linux kernel, etc where right now we just need to introduce an extra IOCTL in linux to support this feature.
> 
> I don't understand why would need a new driver, etc. Given that you are introducing a new IOCTL you could pass a flag to say "This is a static event channel so don't close it".

I tried to implement other solutions to this issue. We can introduce a new event channel state “ECS_STATIC” and set the
event channel state to ECS_STATIC when Xen allocate and create the static event channels.

From guest OS we can check if the event channel is static (via EVTCHNOP_status()  hypercall ), if the event channel is
static don’t try to close the event channel. If guest OS try to close the static event channel Xen will return error as static event channel can’t be closed.

diff --git a/drivers/xen/events/events_base.c b/drivers/xen/events/events_base.c
index 46d9295d9a6e..c5ca29b8ed70 100644
--- a/drivers/xen/events/events_base.c
+++ b/drivers/xen/events/events_base.c
@@ -815,8 +815,17 @@ static void xen_free_irq(unsigned irq)
 
 static void xen_evtchn_close(evtchn_port_t port)
 {
+       struct evtchn_status status;
        struct evtchn_close close;
 
+       status.dom = DOMID_SELF;
+       status.port = port;
+       if (HYPERVISOR_event_channel_op(EVTCHNOP_status, &status) != 0)
+               BUG();
+
+       if (status.status == EVTCHNSTAT_static)
+               return;
+
        close.port = port;
        if (HYPERVISOR_event_channel_op(EVTCHNOP_close, &close) != 0)
                BUG();


Regards,
Rahul
> 
> Cheers,
> 
> -- 
> Julien Grall

Re: [PATCH 5/8] xen/evtchn: don't close the static event channel.

[Julien Grall]

On 05/07/2022 14:28, Rahul Singh wrote:
> Hi Julien,

Hi Rahul,

>> On 28 Jun 2022, at 4:18 pm, Julien Grall <julien@xen.org> wrote:
>>> a new driver in linux kernel, etc where right now we just need to introduce an extra IOCTL in linux to support this feature.
>>
>> I don't understand why would need a new driver, etc. Given that you are introducing a new IOCTL you could pass a flag to say "This is a static event channel so don't close it".
> 
> I tried to implement other solutions to this issue. We can introduce a new event channel state “ECS_STATIC” and set the
> event channel state to ECS_STATIC when Xen allocate and create the static event channels.

 From what you wrote, ECS_STATIC is just an interdomain behind but where 
you want Xen to prevent closing the port.

 From Xen PoV, it is still not clear why this is a problem to let Linux 
closing such port. From the guest PoV, there are other way to pass this 
information (see below).

> 
>  From guest OS we can check if the event channel is static (via EVTCHNOP_status()  hypercall ), if the event channel is
> static don’t try to close the event channel. If guest OS try to close the static event channel Xen will return error as static event channel can’t be closed.
Why do you need this? You already need a binding indicating which ports 
will be pre-allocated. So you could update your binding to pass a flag 
telling Linux "don't close it".

I have already proposed that before and I haven't seen any explanation 
why this is not a viable solution.

Cheers,

-- 
Julien Grall

Re: [PATCH 1/8] xen/evtchn: make evtchn_bind_interdomain global

[Jan Beulich]

On 22.06.2022 16:37, Rahul Singh wrote:
> Event channel support will be added for dom0less domains to allocate
> static event channel. 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.
> 
> Signed-off-by: Rahul Singh <rahul.singh@arm.com>

In light of MISRA I don't think this should be a separate change.

Jan

Re: [PATCH 2/8] xen/evtchn: modify evtchn_alloc_unbound to allocate specified port

[Jan Beulich]

On 22.06.2022 16:51, Julien Grall wrote:
> On 22/06/2022 15:37, Rahul Singh wrote:
>> 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.
> 
> I haven't yet fully reviewed this series. But I would like to point out 
> that this opening a security hole (which I thought I had mention before) 
> that could be exploited by a guest at runtime.
> 
> You would need [1] or similar in order to fix the issue. I am wrote 
> "similar" because the patch could potentially be a problem if you allow 
> a guest to use FIFO (you may need to allocate a lot of memory to fill 
> the hole).

At least from an abstract pov this is an issue with the shim then as
well, at the very least when shim's and the underlying Xen's alloc
algorithms would differ. With the nature of the shim that's not a
security concern, though.

Jan

Re: [PATCH 3/8] xen/evtchn: modify evtchn_bind_interdomain to allocate specified port

[Jan Beulich]

On 22.06.2022 16:38, Rahul Singh wrote:
> @@ -387,8 +392,19 @@ int evtchn_bind_interdomain(evtchn_bind_interdomain_t *bind)
>          spin_lock(&ld->event_lock);
>      }
>  
> -    if ( (lport = get_free_port(ld)) < 0 )
> -        ERROR_EXIT(lport);
> +    if ( lport != 0 )
> +    {
> +        if ( (rc = evtchn_allocate_port(ld, lport)) != 0 )
> +            ERROR_EXIT_DOM(rc, ld);
> +    }
> +    else
> +    {
> +        int alloc_port = get_free_port(ld);
> +
> +        if ( alloc_port < 0 )
> +            ERROR_EXIT_DOM(alloc_port, ld);
> +        lport = alloc_port;
> +    }

This is then the 3rd instance of this pattern. I think the logic
wants moving into get_free_port() (perhaps with a name change).

And of course like in the earlier patch the issue with sparse port
numbers needs to be resolved.

Jan

Re: [PATCH 4/8] xen/evtchn: modify evtchn_bind_interdomain to pass domain as argument

[Jan Beulich]

On 22.06.2022 16:38, Rahul Singh wrote:
> evtchn_bind_interdomain() finds the local domain from "current->domain"
> pointer.
> 
> evtchn_bind_interdomain() will be called from the XEN to support 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>

I think this wants folding with the previous patch.

Jan

Re: [PATCH 5/8] xen/evtchn: don't close the static event channel.

[Jan Beulich]

On 22.06.2022 16:38, Rahul Singh wrote:
> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -119,6 +119,7 @@ struct evtchn
>      unsigned char priority;        /* FIFO event channels only. */
>      unsigned short notify_vcpu_id; /* VCPU for local delivery notification */
>      uint32_t fifo_lastq;           /* Data for identifying last queue. */
> +    bool is_static;                /* Static event channels. */
>  
>  #ifdef CONFIG_XSM
>      union {

_If_ this is the behavior we want in the first place (which I'm
unconvinced of, seeing your discussion with Julien), then please
be conservative with growing the structure. There are available
(padding) bits, so you should first try to use any of those. If
that's impossible (or undesirable), please at least briefly say
why in the description.

Jan

Re: [PATCH 3/8] xen/evtchn: modify evtchn_bind_interdomain to allocate specified port

[Julien Grall]

Hi Jan,

On 05/07/2022 16:11, Jan Beulich wrote:
> On 22.06.2022 16:38, Rahul Singh wrote:
>> @@ -387,8 +392,19 @@ int evtchn_bind_interdomain(evtchn_bind_interdomain_t *bind)
>>           spin_lock(&ld->event_lock);
>>       }
>>   
>> -    if ( (lport = get_free_port(ld)) < 0 )
>> -        ERROR_EXIT(lport);
>> +    if ( lport != 0 )
>> +    {
>> +        if ( (rc = evtchn_allocate_port(ld, lport)) != 0 )
>> +            ERROR_EXIT_DOM(rc, ld);
>> +    }
>> +    else
>> +    {
>> +        int alloc_port = get_free_port(ld);
>> +
>> +        if ( alloc_port < 0 )
>> +            ERROR_EXIT_DOM(alloc_port, ld);
>> +        lport = alloc_port;
>> +    }
> 
> This is then the 3rd instance of this pattern. I think the logic
> wants moving into get_free_port() (perhaps with a name change).

I think the code below would be suitable. I can send it or Rahul can 
re-use the commit [1]:

diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
index 
e6fb024218e949529c587b7c4755295786d6e7a7..757088580c2b2a3e55774f50b8ad7b3ec9243788 
100644 (file)
--- a/xen/common/event_channel.c
+++ b/xen/common/event_channel.c
@@ -292,6 +292,18 @@ void evtchn_free(struct domain *d, struct evtchn *chn)
      xsm_evtchn_close_post(chn);
  }

+static int evtchn_get_port(struct domain *d, uint32_t port)
+{
+    int rc;
+
+    if ( port != 0 )
+        rc = evtchn_allocate_port(d, port);
+    else
+        rc = get_free_port(d);
+
+    return rc != 0 ? rc : port;
+}
+
  static long evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc)
  {
      struct evtchn *chn;
@@ -451,19 +463,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);

[1] 
https://xenbits.xen.org/gitweb/?p=people/julieng/xen-unstable.git;a=commit;h=3860ed9915d6fee97a1088110ffd0a6f29f04d9a

> 
> And of course like in the earlier patch the issue with sparse port
> numbers needs to be resolved.
> 
> Jan

-- 
Julien Grall

Re: [PATCH 6/8] xen/evtchn: don't set notification in evtchn_bind_interdomain()

[Jan Beulich]

On 22.06.2022 16:38, Rahul Singh wrote:
> evtchn_bind_interdomain() sets the notification on the local port to
> handle the lost notification on remote unbound port.
> 
> Static event-channel will be created during domain creation, there is no
> need to set the notification as remote domain is not alive.

So _both_ sides are (to be) created by Xen? I think this wants to be
an optimization _after_ the new users have been introduced; I don't
think the excess notification is a problem? That way one can actually
verify that the uses are as (at this point) guessed.

Also the title shouldn't say (just) "don't" because it skips the step
only conditionally.

Jan

Re: [PATCH 5/8] xen/evtchn: don't close the static event channel.

[Jan Beulich]

On 22.06.2022 16:38, Rahul Singh wrote:
> --- a/xen/include/xen/event.h
> +++ b/xen/include/xen/event.h
> @@ -73,12 +73,12 @@ 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,
> -                                      evtchn_port_t port);
> +                                      evtchn_port_t port, bool is_static);
>  
>  /* 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);
> +                                         evtchn_port_t port, bool is_static);

Didn't even pay attention to this the first time through: You're
again touching functions you did alter already in earlier patches,
and with them their pre-existing call sites. This is not only
unnecessary code churn but also makes it harder to follow where a
change came from when (perhaps much later) using "git blame" or
alike. Please bring these functions into their intended shape in
a single step (each).

Jan

Re: [PATCH 5/8] xen/evtchn: don't close the static event channel.

[Jan Beulich]

On 05.07.2022 17:26, Jan Beulich wrote:
> On 22.06.2022 16:38, Rahul Singh wrote:
>> --- a/xen/include/xen/event.h
>> +++ b/xen/include/xen/event.h
>> @@ -73,12 +73,12 @@ 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,
>> -                                      evtchn_port_t port);
>> +                                      evtchn_port_t port, bool is_static);
>>  
>>  /* 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);
>> +                                         evtchn_port_t port, bool is_static);
> 
> Didn't even pay attention to this the first time through: You're
> again touching functions you did alter already in earlier patches,
> and with them their pre-existing call sites. This is not only
> unnecessary code churn but also makes it harder to follow where a
> change came from when (perhaps much later) using "git blame" or
> alike. Please bring these functions into their intended shape in
> a single step (each).

One more thing: Especially "bind" now has quite a few parameters
for which without dom0less (i.e. particularly on x86) only a single
value would ever be passed. Without LTO the compiler could still
deal with this if the function remained static in all non-dom0less
cases. Please consider whether you want to do so, or whether you
want to find another solution to address this concern.

Jan

Re: [PATCH 3/8] xen/evtchn: modify evtchn_bind_interdomain to allocate specified port

[Jan Beulich]

On 05.07.2022 17:22, Julien Grall wrote:
> Hi Jan,
> 
> On 05/07/2022 16:11, Jan Beulich wrote:
>> On 22.06.2022 16:38, Rahul Singh wrote:
>>> @@ -387,8 +392,19 @@ int evtchn_bind_interdomain(evtchn_bind_interdomain_t *bind)
>>>           spin_lock(&ld->event_lock);
>>>       }
>>>   
>>> -    if ( (lport = get_free_port(ld)) < 0 )
>>> -        ERROR_EXIT(lport);
>>> +    if ( lport != 0 )
>>> +    {
>>> +        if ( (rc = evtchn_allocate_port(ld, lport)) != 0 )
>>> +            ERROR_EXIT_DOM(rc, ld);
>>> +    }
>>> +    else
>>> +    {
>>> +        int alloc_port = get_free_port(ld);
>>> +
>>> +        if ( alloc_port < 0 )
>>> +            ERROR_EXIT_DOM(alloc_port, ld);
>>> +        lport = alloc_port;
>>> +    }
>>
>> This is then the 3rd instance of this pattern. I think the logic
>> wants moving into get_free_port() (perhaps with a name change).
> 
> I think the code below would be suitable. I can send it or Rahul can 
> re-use the commit [1]:

Ah yes; probably makes sense (right now) only in the context of his
series.

> --- a/xen/common/event_channel.c
> +++ b/xen/common/event_channel.c
> @@ -292,6 +292,18 @@ void evtchn_free(struct domain *d, struct evtchn *chn)
>       xsm_evtchn_close_post(chn);
>   }
> 
> +static int evtchn_get_port(struct domain *d, uint32_t port)

Preferably evtchn_port_t.

> +{
> +    int rc;
> +
> +    if ( port != 0 )
> +        rc = evtchn_allocate_port(d, port);
> +    else
> +        rc = get_free_port(d);
> +
> +    return rc != 0 ? rc : port;

We commonly use "rc ?: port" in such cases. At which point I'd be
inclined to make it just

static int evtchn_get_port(struct domain *d, evtchn_port_t port)
{
    return (port ? evtchn_allocate_port(d, port)
                 : get_free_port(d)) ?: port;
}

But I can see you or others having reservations against such ...

Jan

Re: [PATCH 5/8] xen/evtchn: don't close the static event channel.

[Rahul Singh]

Hi Julien,

> On 5 Jul 2022, at 2:56 pm, Julien Grall <julien@xen.org> wrote:
> 
> 
> 
> On 05/07/2022 14:28, Rahul Singh wrote:
>> Hi Julien,
> 
> Hi Rahul,
> 
>>> On 28 Jun 2022, at 4:18 pm, Julien Grall <julien@xen.org> wrote:
>>>> a new driver in linux kernel, etc where right now we just need to introduce an extra IOCTL in linux to support this feature.
>>> 
>>> I don't understand why would need a new driver, etc. Given that you are introducing a new IOCTL you could pass a flag to say "This is a static event channel so don't close it".
>> I tried to implement other solutions to this issue. We can introduce a new event channel state “ECS_STATIC” and set the
>> event channel state to ECS_STATIC when Xen allocate and create the static event channels.
> 
> From what you wrote, ECS_STATIC is just an interdomain behind but where you want Xen to prevent closing the port.
> 
> From Xen PoV, it is still not clear why this is a problem to let Linux closing such port. From the guest PoV, there are other way to pass this information (see below).

If Linux closes the port, the static event channel created by Xen associated with such port will not be available to use afterward.

When I started implemented the static event channel series, I thought the static event channel has to be available for use during
the lifetime of the guest. This patch avoids closing the port if the Linux user-space application wants to use the event channel again.

This patch is fixing the problem for Linux OS, and I agree with you that we should not modify the Xen to fix the Linux problem.
Therefore, If the guest decided to close the static event channel, Xen will close the port. Event Chanel associated with the port
will not be available for use after that.I will discard this patch in the next series.

> 
>> From guest OS we can check if the event channel is static (via EVTCHNOP_status()  hypercall ), if the event channel is
>> static don’t try to close the event channel. If guest OS try to close the static event channel Xen will return error as static event channel can’t be closed.
> Why do you need this? You already need a binding indicating which ports will be pre-allocated. So you could update your binding to pass a flag telling Linux "don't close it".
> 
> I have already proposed that before and I haven't seen any explanation why this is not a viable solution.

Sorry I didn’t mention this earlier, I started with your suggestion to fix the issue but after going through the Linux evtchn driver code
it is not straight forward to tell Linux don’t close the port. Let me try to explain.

In Linux, struct user_evtchn {} is the struct that hold the information for each user evtchn opened. We can add one bool parameter in this struct to tell Linux driver
via IOCTL if evtchn is static. When user application close the fd "/dev/xen/evtchn” , evtchn_release() will traverse all the evtchn and call evtchn_unbind_from_user()
for each evtchn. evtchn_unbind_from_user() will call  __unbind_from_irq(irq) that will call xen_evtchn_close() . We need references to "struct user_evtchn” in
function __unbind_from_irq() to pass as argument to xen_evtchn_close() not to close the static event channel.  I am not able to find any way to get 
struct user_evtchn in function __unbind_from_irq() , without modifying the other Linux structure.

Regards,
Rahul

Re: [PATCH 5/8] xen/evtchn: don't close the static event channel.

[Julien Grall]

(+ Juergen for the Linux question)

On 06/07/2022 11:42, Rahul Singh wrote:
> Hi Julien,
> 
>> On 5 Jul 2022, at 2:56 pm, Julien Grall <julien@xen.org> wrote:
>>
>>
>>
>> On 05/07/2022 14:28, Rahul Singh wrote:
>>> Hi Julien,
>>
>> Hi Rahul,
>>
>>>> On 28 Jun 2022, at 4:18 pm, Julien Grall <julien@xen.org> wrote:
>>>>> a new driver in linux kernel, etc where right now we just need to introduce an extra IOCTL in linux to support this feature.
>>>>
>>>> I don't understand why would need a new driver, etc. Given that you are introducing a new IOCTL you could pass a flag to say "This is a static event channel so don't close it".
>>> I tried to implement other solutions to this issue. We can introduce a new event channel state “ECS_STATIC” and set the
>>> event channel state to ECS_STATIC when Xen allocate and create the static event channels.
>>
>>  From what you wrote, ECS_STATIC is just an interdomain behind but where you want Xen to prevent closing the port.
>>
>>  From Xen PoV, it is still not clear why this is a problem to let Linux closing such port. From the guest PoV, there are other way to pass this information (see below).
> 
> If Linux closes the port, the static event channel created by Xen associated with such port will not be available to use afterward.
> 
> When I started implemented the static event channel series, I thought the static event channel has to be available for use during
> the lifetime of the guest. This patch avoids closing the port if the Linux user-space application wants to use the event channel again.
> 
> This patch is fixing the problem for Linux OS, and I agree with you that we should not modify the Xen to fix the Linux problem.
> Therefore, If the guest decided to close the static event channel, Xen will close the port. Event Chanel associated with the port
> will not be available for use after that.I will discard this patch in the next series.
> 
>>
>>>  From guest OS we can check if the event channel is static (via EVTCHNOP_status()  hypercall ), if the event channel is
>>> static don’t try to close the event channel. If guest OS try to close the static event channel Xen will return error as static event channel can’t be closed.
>> Why do you need this? You already need a binding indicating which ports will be pre-allocated. So you could update your binding to pass a flag telling Linux "don't close it".
>>
>> I have already proposed that before and I haven't seen any explanation why this is not a viable solution.
> 
> Sorry I didn’t mention this earlier, I started with your suggestion to fix the issue but after going through the Linux evtchn driver code
> it is not straight forward to tell Linux don’t close the port. Let me try to explain.
> 
> In Linux, struct user_evtchn {} is the struct that hold the information for each user evtchn opened. We can add one bool parameter in this struct to tell Linux driver
> via IOCTL if evtchn is static. When user application close the fd "/dev/xen/evtchn” , evtchn_release() will traverse all the evtchn and call evtchn_unbind_from_user()
> for each evtchn. evtchn_unbind_from_user() will call  __unbind_from_irq(irq) that will call xen_evtchn_close() . We need references to "struct user_evtchn” in
> function __unbind_from_irq() to pass as argument to xen_evtchn_close() not to close the static event channel.  I am not able to find any way to get
> struct user_evtchn in function __unbind_from_irq() , without modifying the other Linux structure.
> 
> Regards,
> Rahul
> 

-- 
Julien Grall

Re: [PATCH 5/8] xen/evtchn: don't close the static event channel.

[Juergen Gross]

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

On 06.07.22 13:04, Julien Grall wrote:
> (+ Juergen for the Linux question)
> 
> On 06/07/2022 11:42, Rahul Singh wrote:
>> Hi Julien,
>>
>>> On 5 Jul 2022, at 2:56 pm, Julien Grall <julien@xen.org> wrote:
>>>
>>>
>>>
>>> On 05/07/2022 14:28, Rahul Singh wrote:
>>>> Hi Julien,
>>>
>>> Hi Rahul,
>>>
>>>>> On 28 Jun 2022, at 4:18 pm, Julien Grall <julien@xen.org> wrote:
>>>>>> a new driver in linux kernel, etc where right now we just need to 
>>>>>> introduce an extra IOCTL in linux to support this feature.
>>>>>
>>>>> I don't understand why would need a new driver, etc. Given that you are 
>>>>> introducing a new IOCTL you could pass a flag to say "This is a static 
>>>>> event channel so don't close it".
>>>> I tried to implement other solutions to this issue. We can introduce a new 
>>>> event channel state “ECS_STATIC” and set the
>>>> event channel state to ECS_STATIC when Xen allocate and create the static 
>>>> event channels.
>>>
>>>  From what you wrote, ECS_STATIC is just an interdomain behind but where you 
>>> want Xen to prevent closing the port.
>>>
>>>  From Xen PoV, it is still not clear why this is a problem to let Linux 
>>> closing such port. From the guest PoV, there are other way to pass this 
>>> information (see below).
>>
>> If Linux closes the port, the static event channel created by Xen associated 
>> with such port will not be available to use afterward.
>>
>> When I started implemented the static event channel series, I thought the 
>> static event channel has to be available for use during
>> the lifetime of the guest. This patch avoids closing the port if the Linux 
>> user-space application wants to use the event channel again.
>>
>> This patch is fixing the problem for Linux OS, and I agree with you that we 
>> should not modify the Xen to fix the Linux problem.
>> Therefore, If the guest decided to close the static event channel, Xen will 
>> close the port. Event Chanel associated with the port
>> will not be available for use after that.I will discard this patch in the next 
>> series.
>>
>>>
>>>>  From guest OS we can check if the event channel is static (via 
>>>> EVTCHNOP_status()  hypercall ), if the event channel is
>>>> static don’t try to close the event channel. If guest OS try to close the 
>>>> static event channel Xen will return error as static event channel can’t be 
>>>> closed.
>>> Why do you need this? You already need a binding indicating which ports will 
>>> be pre-allocated. So you could update your binding to pass a flag telling 
>>> Linux "don't close it".
>>>
>>> I have already proposed that before and I haven't seen any explanation why 
>>> this is not a viable solution.
>>
>> Sorry I didn’t mention this earlier, I started with your suggestion to fix the 
>> issue but after going through the Linux evtchn driver code
>> it is not straight forward to tell Linux don’t close the port. Let me try to 
>> explain.
>>
>> In Linux, struct user_evtchn {} is the struct that hold the information for 
>> each user evtchn opened. We can add one bool parameter in this struct to tell 
>> Linux driver
>> via IOCTL if evtchn is static. When user application close the fd 
>> "/dev/xen/evtchn” , evtchn_release() will traverse all the evtchn and call 
>> evtchn_unbind_from_user()
>> for each evtchn. evtchn_unbind_from_user() will call  __unbind_from_irq(irq) 
>> that will call xen_evtchn_close() . We need references to "struct user_evtchn” in
>> function __unbind_from_irq() to pass as argument to xen_evtchn_close() not to 
>> close the static event channel.  I am not able to find any way to get
>> struct user_evtchn in function __unbind_from_irq() , without modifying the 
>> other Linux structure.

The "static" flag should be added to struct irq_info. In case all relevant
event channels are really user ones, we could easily add another "static"
flag to evtchn_make_refcounted(), which is already used to set a user
event channel specific value into struct irq_info when binding the event
channel.


Juergen



[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH 1/8] xen/evtchn: make evtchn_bind_interdomain global

[Rahul Singh]

Hi Jan,


> On 5 Jul 2022, at 3:56 pm, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 22.06.2022 16:37, Rahul Singh wrote:
>> Event channel support will be added for dom0less domains to allocate
>> static event channel. 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.
>> 
>> Signed-off-by: Rahul Singh <rahul.singh@arm.com>
> 
> In light of MISRA I don't think this should be a separate change.

I will merge patch #1 #3 and #4 in next version.

Regards,
Rahul

Re: [PATCH 4/8] xen/evtchn: modify evtchn_bind_interdomain to pass domain as argument

[Rahul Singh]

Hi Jan,

> On 5 Jul 2022, at 4:13 pm, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 22.06.2022 16:38, Rahul Singh wrote:
>> evtchn_bind_interdomain() finds the local domain from "current->domain"
>> pointer.
>> 
>> evtchn_bind_interdomain() will be called from the XEN to support 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>
> 
> I think this wants folding with the previous patch.

Ack.

Regards,
Rahul

Re: [PATCH 3/8] xen/evtchn: modify evtchn_bind_interdomain to allocate specified port

[Rahul Singh]

Hi Julien,

> On 5 Jul 2022, at 4:22 pm, Julien Grall <julien@xen.org> wrote:
> 
> Hi Jan,
> 
> On 05/07/2022 16:11, Jan Beulich wrote:
>> On 22.06.2022 16:38, Rahul Singh wrote:
>>> @@ -387,8 +392,19 @@ int evtchn_bind_interdomain(evtchn_bind_interdomain_t *bind)
>>> spin_lock(&ld->event_lock);
>>> }
>>> - if ( (lport = get_free_port(ld)) < 0 )
>>> - ERROR_EXIT(lport);
>>> + if ( lport != 0 )
>>> + {
>>> + if ( (rc = evtchn_allocate_port(ld, lport)) != 0 )
>>> + ERROR_EXIT_DOM(rc, ld);
>>> + }
>>> + else
>>> + {
>>> + int alloc_port = get_free_port(ld);
>>> +
>>> + if ( alloc_port < 0 )
>>> + ERROR_EXIT_DOM(alloc_port, ld);
>>> + lport = alloc_port;
>>> + }
>> This is then the 3rd instance of this pattern. I think the logic
>> wants moving into get_free_port() (perhaps with a name change).
> 
> I think the code below would be suitable. I can send it or Rahul can re-use the commit [1]:

Thanks for the code. I will include this patch in next version.

Regards,
Rahul

Re: [PATCH 3/8] xen/evtchn: modify evtchn_bind_interdomain to allocate specified port

[Rahul Singh]

Hi Jan

> On 5 Jul 2022, at 4:42 pm, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 05.07.2022 17:22, Julien Grall wrote:
>> Hi Jan,
>> 
>> On 05/07/2022 16:11, Jan Beulich wrote:
>>> On 22.06.2022 16:38, Rahul Singh wrote:
>>>> @@ -387,8 +392,19 @@ int evtchn_bind_interdomain(evtchn_bind_interdomain_t *bind)
>>>> spin_lock(&ld->event_lock);
>>>> }
>>>> 
>>>> - if ( (lport = get_free_port(ld)) < 0 )
>>>> - ERROR_EXIT(lport);
>>>> + if ( lport != 0 )
>>>> + {
>>>> + if ( (rc = evtchn_allocate_port(ld, lport)) != 0 )
>>>> + ERROR_EXIT_DOM(rc, ld);
>>>> + }
>>>> + else
>>>> + {
>>>> + int alloc_port = get_free_port(ld);
>>>> +
>>>> + if ( alloc_port < 0 )
>>>> + ERROR_EXIT_DOM(alloc_port, ld);
>>>> + lport = alloc_port;
>>>> + }
>>> 
>>> This is then the 3rd instance of this pattern. I think the logic
>>> wants moving into get_free_port() (perhaps with a name change).
>> 
>> I think the code below would be suitable. I can send it or Rahul can 
>> re-use the commit [1]:
> 
> Ah yes; probably makes sense (right now) only in the context of his
> series.

Ack. 
> 
>> --- a/xen/common/event_channel.c
>> +++ b/xen/common/event_channel.c
>> @@ -292,6 +292,18 @@ void evtchn_free(struct domain *d, struct evtchn *chn)
>> xsm_evtchn_close_post(chn);
>> }
>> 
>> +static int evtchn_get_port(struct domain *d, uint32_t port)
> 
> Preferably evtchn_port_t.

Ack. 
> 
>> +{
>> + int rc;
>> +
>> + if ( port != 0 )
>> + rc = evtchn_allocate_port(d, port);
>> + else
>> + rc = get_free_port(d);
>> +
>> + return rc != 0 ? rc : port;
> 
> We commonly use "rc ?: port" in such cases. At which point I'd be
> inclined to make it just
> 
> static int evtchn_get_port(struct domain *d, evtchn_port_t port)
> {
> return (port ? evtchn_allocate_port(d, port)
> : get_free_port(d)) ?: port;
> }
> 
> But I can see you or others having reservations against such ...

If everyone agree with above code I will modify the Julien patch to include
this code in next version.

Regards,
Rahul

Re: [PATCH 5/8] xen/evtchn: don't close the static event channel.

[Rahul Singh]

Hi Juergen,

> On 6 Jul 2022, at 12:33 pm, Juergen Gross <jgross@suse.com> wrote:
> 
> On 06.07.22 13:04, Julien Grall wrote:
>> (+ Juergen for the Linux question)
>> On 06/07/2022 11:42, Rahul Singh wrote:
>>> Hi Julien,
>>> 
>>>> On 5 Jul 2022, at 2:56 pm, Julien Grall <julien@xen.org> wrote:
>>>> 
>>>> 
>>>> 
>>>> On 05/07/2022 14:28, Rahul Singh wrote:
>>>>> Hi Julien,
>>>> 
>>>> Hi Rahul,
>>>> 
>>>>>> On 28 Jun 2022, at 4:18 pm, Julien Grall <julien@xen.org> wrote:
>>>>>>> a new driver in linux kernel, etc where right now we just need to introduce an extra IOCTL in linux to support this feature.
>>>>>> 
>>>>>> I don't understand why would need a new driver, etc. Given that you are introducing a new IOCTL you could pass a flag to say "This is a static event channel so don't close it".
>>>>> I tried to implement other solutions to this issue. We can introduce a new event channel state “ECS_STATIC” and set the
>>>>> event channel state to ECS_STATIC when Xen allocate and create the static event channels.
>>>> 
>>>> From what you wrote, ECS_STATIC is just an interdomain behind but where you want Xen to prevent closing the port.
>>>> 
>>>> From Xen PoV, it is still not clear why this is a problem to let Linux closing such port. From the guest PoV, there are other way to pass this information (see below).
>>> 
>>> If Linux closes the port, the static event channel created by Xen associated with such port will not be available to use afterward.
>>> 
>>> When I started implemented the static event channel series, I thought the static event channel has to be available for use during
>>> the lifetime of the guest. This patch avoids closing the port if the Linux user-space application wants to use the event channel again.
>>> 
>>> This patch is fixing the problem for Linux OS, and I agree with you that we should not modify the Xen to fix the Linux problem.
>>> Therefore, If the guest decided to close the static event channel, Xen will close the port. Event Chanel associated with the port
>>> will not be available for use after that.I will discard this patch in the next series.
>>> 
>>>> 
>>>>> From guest OS we can check if the event channel is static (via EVTCHNOP_status()  hypercall ), if the event channel is
>>>>> static don’t try to close the event channel. If guest OS try to close the static event channel Xen will return error as static event channel can’t be closed.
>>>> Why do you need this? You already need a binding indicating which ports will be pre-allocated. So you could update your binding to pass a flag telling Linux "don't close it".
>>>> 
>>>> I have already proposed that before and I haven't seen any explanation why this is not a viable solution.
>>> 
>>> Sorry I didn’t mention this earlier, I started with your suggestion to fix the issue but after going through the Linux evtchn driver code
>>> it is not straight forward to tell Linux don’t close the port. Let me try to explain.
>>> 
>>> In Linux, struct user_evtchn {} is the struct that hold the information for each user evtchn opened. We can add one bool parameter in this struct to tell Linux driver
>>> via IOCTL if evtchn is static. When user application close the fd "/dev/xen/evtchn” , evtchn_release() will traverse all the evtchn and call evtchn_unbind_from_user()
>>> for each evtchn. evtchn_unbind_from_user() will call  __unbind_from_irq(irq) that will call xen_evtchn_close() . We need references to "struct user_evtchn” in
>>> function __unbind_from_irq() to pass as argument to xen_evtchn_close() not to close the static event channel.  I am not able to find any way to get
>>> struct user_evtchn in function __unbind_from_irq() , without modifying the other Linux structure.
> 
> The "static" flag should be added to struct irq_info. In case all relevant
> event channels are really user ones, we could easily add another "static"
> flag to evtchn_make_refcounted(), which is already used to set a user
> event channel specific value into struct irq_info when binding the event
> channel.
> 

As suggested by you, I modified the Linux Kernel by adding “static" flag in struct irq_info and
it works fine. We can skip the closing of static channel if required. 

I will send the patch for review once I will send the patch for new ioctl for static event channel.

Regards,
Rahul

Re: [PATCH 2/8] xen/evtchn: modify evtchn_alloc_unbound to allocate specified port

[Rahul Singh]

Hi Julien,

> On 22 Jun 2022, at 3:51 pm, Julien Grall <julien@xen.org> wrote:
> 
> Hi,
> 
> On 22/06/2022 15:37, Rahul Singh wrote:
>> 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.
> 
> I haven't yet fully reviewed this series. But I would like to point out that this opening a security hole (which I thought I had mention before) that could be exploited by a guest at runtime.
> 
> You would need [1] or similar in order to fix the issue. I am wrote "similar" because the patch could potentially be a problem if you allow a guest to use FIFO (you may need to allocate a lot of memory to fill the hole).
> 
> Cheers,
> 
> [1] https://xenbits.xen.org/gitweb/?p=people/julieng/xen-unstable.git;a=commit;h=2d89486fcf11216331e58a21b367b8a9be1af725

Thanks for sharing the patch.  If you are okay I can use this patch in next version to fix the security hole.

For the FIFO issue, we can introduce the new config option to restrict the maximum number of static
port supported in Xen. We can check the user-defined static port when we parse the device tree and if
a user-defined static port is greater than the maximum allowed static port will return an error to the user.
In this way, we can avoid allocating a lot of memory to fill the hole.

Let me know your view on this.

config MAX_STATIC_PORT
    int "Maximum number of static ports”
    range 1 4095
    help                                                                        
       Controls the build-time maximum number of static port supported.

Regards,
Rahul

Re: [PATCH 2/8] xen/evtchn: modify evtchn_alloc_unbound to allocate specified port

[Julien Grall]

On 11/07/2022 17:08, Rahul Singh wrote:
> Hi Julien,

Hi Rahul,
>> On 22 Jun 2022, at 3:51 pm, Julien Grall <julien@xen.org> wrote:
>>
>> Hi,
>>
>> On 22/06/2022 15:37, Rahul Singh wrote:
>>> 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.
>>
>> I haven't yet fully reviewed this series. But I would like to point out that this opening a security hole (which I thought I had mention before) that could be exploited by a guest at runtime.
>>
>> You would need [1] or similar in order to fix the issue. I am wrote "similar" because the patch could potentially be a problem if you allow a guest to use FIFO (you may need to allocate a lot of memory to fill the hole).
>>
>> Cheers,
>>
>> [1] https://xenbits.xen.org/gitweb/?p=people/julieng/xen-unstable.git;a=commit;h=2d89486fcf11216331e58a21b367b8a9be1af725
> 
> Thanks for sharing the patch.  If you are okay I can use this patch in next version to fix the security hole.

I am fine with that.

> 
> For the FIFO issue, we can introduce the new config option to restrict the maximum number of static
> port supported in Xen. We can check the user-defined static port when we parse the device tree and if
> a user-defined static port is greater than the maximum allowed static port will return an error to the user.
> In this way, we can avoid allocating a lot of memory to fill the hole.
> 
> Let me know your view on this.
> 
> config MAX_STATIC_PORT
>      int "Maximum number of static ports”
>      range 1 4095
>      help
>         Controls the build-time maximum number of static port supported

The problem is not exclusive to the static event channel. So I don't 
think this is right to introduce MAX_STATIC_PORT to mitigate the issue 
(even though this is the only user today).

A few of alternative solutions:
   1) Handle preemption in alloc_evtchn_bucket()
   2) Allocate all the buckets when the domain is created (the max 
numbers event channel is known). We may need to think about preemption
   3) Tweak is_port_valid() to check if the bucket is valid. This would 
introduce a couple of extra memory access (might be OK as the bucket 
would be accessed afterwards) and we would need to update some users.

At the moment, 3) is appealing me the most. I would be interested to 
have an opionions from the other maintainers.

Cheers,

-- 
Julien Grall

Re: [PATCH 2/8] xen/evtchn: modify evtchn_alloc_unbound to allocate specified port

[Jan Beulich]

On 12.07.2022 19:28, Julien Grall wrote:
> On 11/07/2022 17:08, Rahul Singh wrote:
>>> On 22 Jun 2022, at 3:51 pm, Julien Grall <julien@xen.org> wrote:
>>> On 22/06/2022 15:37, Rahul Singh wrote:
>>>> 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.
>>>
>>> I haven't yet fully reviewed this series. But I would like to point out that this opening a security hole (which I thought I had mention before) that could be exploited by a guest at runtime.
>>>
>>> You would need [1] or similar in order to fix the issue. I am wrote "similar" because the patch could potentially be a problem if you allow a guest to use FIFO (you may need to allocate a lot of memory to fill the hole).
>>>
>>> Cheers,
>>>
>>> [1] https://xenbits.xen.org/gitweb/?p=people/julieng/xen-unstable.git;a=commit;h=2d89486fcf11216331e58a21b367b8a9be1af725
>>
>> Thanks for sharing the patch.  If you are okay I can use this patch in next version to fix the security hole.
> 
> I am fine with that.
> 
>>
>> For the FIFO issue, we can introduce the new config option to restrict the maximum number of static
>> port supported in Xen. We can check the user-defined static port when we parse the device tree and if
>> a user-defined static port is greater than the maximum allowed static port will return an error to the user.
>> In this way, we can avoid allocating a lot of memory to fill the hole.
>>
>> Let me know your view on this.
>>
>> config MAX_STATIC_PORT
>>      int "Maximum number of static ports”
>>      range 1 4095
>>      help
>>         Controls the build-time maximum number of static port supported
> 
> The problem is not exclusive to the static event channel. So I don't 
> think this is right to introduce MAX_STATIC_PORT to mitigate the issue 
> (even though this is the only user today).
> 
> A few of alternative solutions:
>    1) Handle preemption in alloc_evtchn_bucket()
>    2) Allocate all the buckets when the domain is created (the max 
> numbers event channel is known). We may need to think about preemption
>    3) Tweak is_port_valid() to check if the bucket is valid. This would 
> introduce a couple of extra memory access (might be OK as the bucket 
> would be accessed afterwards) and we would need to update some users.
> 
> At the moment, 3) is appealing me the most. I would be interested to 
> have an opionions from the other maintainers.

Fwiw of the named alternatives I would also prefer 3. Whether things
really need generalizing at this point I'm not sure, though.

Jan

Re: [PATCH 2/8] xen/evtchn: modify evtchn_alloc_unbound to allocate specified port

[Julien Grall]

Hi,

On 13/07/2022 07:21, Jan Beulich wrote:
>>> For the FIFO issue, we can introduce the new config option to restrict the maximum number of static
>>> port supported in Xen. We can check the user-defined static port when we parse the device tree and if
>>> a user-defined static port is greater than the maximum allowed static port will return an error to the user.
>>> In this way, we can avoid allocating a lot of memory to fill the hole.
>>>
>>> Let me know your view on this.
>>>
>>> config MAX_STATIC_PORT
>>>       int "Maximum number of static ports”
>>>       range 1 4095
>>>       help
>>>          Controls the build-time maximum number of static port supported
>>
>> The problem is not exclusive to the static event channel. So I don't
>> think this is right to introduce MAX_STATIC_PORT to mitigate the issue
>> (even though this is the only user today).
>>
>> A few of alternative solutions:
>>     1) Handle preemption in alloc_evtchn_bucket()
>>     2) Allocate all the buckets when the domain is created (the max
>> numbers event channel is known). We may need to think about preemption
>>     3) Tweak is_port_valid() to check if the bucket is valid. This would
>> introduce a couple of extra memory access (might be OK as the bucket
>> would be accessed afterwards) and we would need to update some users.
>>
>> At the moment, 3) is appealing me the most. I would be interested to
>> have an opionions from the other maintainers.
> 
> Fwiw of the named alternatives I would also prefer 3. Whether things
> really need generalizing at this point I'm not sure, though.
I am worry that we may end up to forget that we had non-generaic way 
(e.g. MAX_STATIC_PORT) to prevent trigger the issue. So we could end up 
to mistakenly introduce a security issue.

However, my point was less about generalization but more about 
introducing CONFIG_MAX_STATIC_PORT.

It seems strange to let the admin to decide the maximum number of static 
port supported.

If we want to rely on non-generic mechanism, then I think the right way 
to go is to restrict max_evtchn_port for domUs to 4096 (it is -1 at the 
moment). If we want to give more flexibility then it should be a 
per-domain property in the DT.

Cheers,

-- 
Julien Grall

Re: [PATCH 2/8] xen/evtchn: modify evtchn_alloc_unbound to allocate specified port

[Jan Beulich]

On 13.07.2022 11:35, Julien Grall wrote:
> Hi,
> 
> On 13/07/2022 07:21, Jan Beulich wrote:
>>>> For the FIFO issue, we can introduce the new config option to restrict the maximum number of static
>>>> port supported in Xen. We can check the user-defined static port when we parse the device tree and if
>>>> a user-defined static port is greater than the maximum allowed static port will return an error to the user.
>>>> In this way, we can avoid allocating a lot of memory to fill the hole.
>>>>
>>>> Let me know your view on this.
>>>>
>>>> config MAX_STATIC_PORT
>>>>       int "Maximum number of static ports”
>>>>       range 1 4095
>>>>       help
>>>>          Controls the build-time maximum number of static port supported
>>>
>>> The problem is not exclusive to the static event channel. So I don't
>>> think this is right to introduce MAX_STATIC_PORT to mitigate the issue
>>> (even though this is the only user today).
>>>
>>> A few of alternative solutions:
>>>     1) Handle preemption in alloc_evtchn_bucket()
>>>     2) Allocate all the buckets when the domain is created (the max
>>> numbers event channel is known). We may need to think about preemption
>>>     3) Tweak is_port_valid() to check if the bucket is valid. This would
>>> introduce a couple of extra memory access (might be OK as the bucket
>>> would be accessed afterwards) and we would need to update some users.
>>>
>>> At the moment, 3) is appealing me the most. I would be interested to
>>> have an opionions from the other maintainers.
>>
>> Fwiw of the named alternatives I would also prefer 3. Whether things
>> really need generalizing at this point I'm not sure, though.
> I am worry that we may end up to forget that we had non-generaic way 
> (e.g. MAX_STATIC_PORT) to prevent trigger the issue. So we could end up 
> to mistakenly introduce a security issue.
> 
> However, my point was less about generalization but more about 
> introducing CONFIG_MAX_STATIC_PORT.
> 
> It seems strange to let the admin to decide the maximum number of static 
> port supported.

Why (assuming s/admin/build admin/)? I view both a build time upper bound
as well as (alternatively) a command line driven upper bound as reasonable
(in the latter case there of course still would want/need to be an upper
bound on what is legitimate to specify from the command line). Using
static ports after all means there's a static largest port number.
Determined across all intended uses of a build such an upper bound can be
a feasible mechanism.

Jan

Re: [PATCH 2/8] xen/evtchn: modify evtchn_alloc_unbound to allocate specified port

[Bertrand Marquis]

> On 13 Jul 2022, at 10:53, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 13.07.2022 11:35, Julien Grall wrote:
>> Hi,
>> 
>> On 13/07/2022 07:21, Jan Beulich wrote:
>>>>> For the FIFO issue, we can introduce the new config option to restrict the maximum number of static
>>>>> port supported in Xen. We can check the user-defined static port when we parse the device tree and if
>>>>> a user-defined static port is greater than the maximum allowed static port will return an error to the user.
>>>>> In this way, we can avoid allocating a lot of memory to fill the hole.
>>>>> 
>>>>> Let me know your view on this.
>>>>> 
>>>>> config MAX_STATIC_PORT
>>>>> int "Maximum number of static ports”
>>>>> range 1 4095
>>>>> help
>>>>> Controls the build-time maximum number of static port supported
>>>> 
>>>> The problem is not exclusive to the static event channel. So I don't
>>>> think this is right to introduce MAX_STATIC_PORT to mitigate the issue
>>>> (even though this is the only user today).
>>>> 
>>>> A few of alternative solutions:
>>>> 1) Handle preemption in alloc_evtchn_bucket()
>>>> 2) Allocate all the buckets when the domain is created (the max
>>>> numbers event channel is known). We may need to think about preemption
>>>> 3) Tweak is_port_valid() to check if the bucket is valid. This would
>>>> introduce a couple of extra memory access (might be OK as the bucket
>>>> would be accessed afterwards) and we would need to update some users.
>>>> 
>>>> At the moment, 3) is appealing me the most. I would be interested to
>>>> have an opionions from the other maintainers.
>>> 
>>> Fwiw of the named alternatives I would also prefer 3. Whether things
>>> really need generalizing at this point I'm not sure, though.
>> I am worry that we may end up to forget that we had non-generaic way 
>> (e.g. MAX_STATIC_PORT) to prevent trigger the issue. So we could end up 
>> to mistakenly introduce a security issue.
>> 
>> However, my point was less about generalization but more about 
>> introducing CONFIG_MAX_STATIC_PORT.
>> 
>> It seems strange to let the admin to decide the maximum number of static 
>> port supported.
> 
> Why (assuming s/admin/build admin/)? I view both a build time upper bound
> as well as (alternatively) a command line driven upper bound as reasonable
> (in the latter case there of course still would want/need to be an upper
> bound on what is legitimate to specify from the command line). Using
> static ports after all means there's a static largest port number.
> Determined across all intended uses of a build such an upper bound can be
> a feasible mechanism.

I agree with this. Right now all static port must be defined on boot so this is fully
ok to have a maximum compilation value or something that can be customised
on command line.
In this case the admin must be fully aware of what he does from the start.

Also whatever value we define as default will probably be far bigger than any
real use case I think.

Bertrand

> 
> Jan

Re: [PATCH 2/8] xen/evtchn: modify evtchn_alloc_unbound to allocate specified port

[Julien Grall]

Hi Jan,

On 13/07/2022 10:53, Jan Beulich wrote:
> On 13.07.2022 11:35, Julien Grall wrote:
>> Hi,
>>
>> On 13/07/2022 07:21, Jan Beulich wrote:
>>>>> For the FIFO issue, we can introduce the new config option to restrict the maximum number of static
>>>>> port supported in Xen. We can check the user-defined static port when we parse the device tree and if
>>>>> a user-defined static port is greater than the maximum allowed static port will return an error to the user.
>>>>> In this way, we can avoid allocating a lot of memory to fill the hole.
>>>>>
>>>>> Let me know your view on this.
>>>>>
>>>>> config MAX_STATIC_PORT
>>>>>        int "Maximum number of static ports”
>>>>>        range 1 4095
>>>>>        help
>>>>>           Controls the build-time maximum number of static port supported
>>>>
>>>> The problem is not exclusive to the static event channel. So I don't
>>>> think this is right to introduce MAX_STATIC_PORT to mitigate the issue
>>>> (even though this is the only user today).
>>>>
>>>> A few of alternative solutions:
>>>>      1) Handle preemption in alloc_evtchn_bucket()
>>>>      2) Allocate all the buckets when the domain is created (the max
>>>> numbers event channel is known). We may need to think about preemption
>>>>      3) Tweak is_port_valid() to check if the bucket is valid. This would
>>>> introduce a couple of extra memory access (might be OK as the bucket
>>>> would be accessed afterwards) and we would need to update some users.
>>>>
>>>> At the moment, 3) is appealing me the most. I would be interested to
>>>> have an opionions from the other maintainers.
>>>
>>> Fwiw of the named alternatives I would also prefer 3. Whether things
>>> really need generalizing at this point I'm not sure, though.
>> I am worry that we may end up to forget that we had non-generaic way
>> (e.g. MAX_STATIC_PORT) to prevent trigger the issue. So we could end up
>> to mistakenly introduce a security issue.
>>
>> However, my point was less about generalization but more about
>> introducing CONFIG_MAX_STATIC_PORT.
>>
>> It seems strange to let the admin to decide the maximum number of static
>> port supported.
> 
> Why (assuming s/admin/build admin/)? I view both a build time upper bound
> as well as (alternatively) a command line driven upper bound as reasonable
> (in the latter case there of course still would want/need to be an upper
> bound on what is legitimate to specify from the command line). Using
> static ports after all means there's a static largest port number.
> Determined across all intended uses of a build such an upper bound can be
> a feasible mechanism.

AFAICT, the limit is only here to mitigate the risk with the patch I 
proposed. With a proper fix, the limit would be articial and therefore 
it is not clear why the admin should be able to configure it (even 
temporarily).

Instead, I think we want to have a limit that apply for both statically 
and dynamically allocated even channel. This is what d->max_evtchn_port 
is for.

Cheers,

-- 
Julien Grall

Re: [PATCH 2/8] xen/evtchn: modify evtchn_alloc_unbound to allocate specified port

[Julien Grall]

Hi,

On 13/07/2022 11:03, Bertrand Marquis wrote:
> 
> 
>> On 13 Jul 2022, at 10:53, Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 13.07.2022 11:35, Julien Grall wrote:
>>> Hi,
>>>
>>> On 13/07/2022 07:21, Jan Beulich wrote:
>>>>>> For the FIFO issue, we can introduce the new config option to restrict the maximum number of static
>>>>>> port supported in Xen. We can check the user-defined static port when we parse the device tree and if
>>>>>> a user-defined static port is greater than the maximum allowed static port will return an error to the user.
>>>>>> In this way, we can avoid allocating a lot of memory to fill the hole.
>>>>>>
>>>>>> Let me know your view on this.
>>>>>>
>>>>>> config MAX_STATIC_PORT
>>>>>> int "Maximum number of static ports”
>>>>>> range 1 4095
>>>>>> help
>>>>>> Controls the build-time maximum number of static port supported
>>>>>
>>>>> The problem is not exclusive to the static event channel. So I don't
>>>>> think this is right to introduce MAX_STATIC_PORT to mitigate the issue
>>>>> (even though this is the only user today).
>>>>>
>>>>> A few of alternative solutions:
>>>>> 1) Handle preemption in alloc_evtchn_bucket()
>>>>> 2) Allocate all the buckets when the domain is created (the max
>>>>> numbers event channel is known). We may need to think about preemption
>>>>> 3) Tweak is_port_valid() to check if the bucket is valid. This would
>>>>> introduce a couple of extra memory access (might be OK as the bucket
>>>>> would be accessed afterwards) and we would need to update some users.
>>>>>
>>>>> At the moment, 3) is appealing me the most. I would be interested to
>>>>> have an opionions from the other maintainers.
>>>>
>>>> Fwiw of the named alternatives I would also prefer 3. Whether things
>>>> really need generalizing at this point I'm not sure, though.
>>> I am worry that we may end up to forget that we had non-generaic way
>>> (e.g. MAX_STATIC_PORT) to prevent trigger the issue. So we could end up
>>> to mistakenly introduce a security issue.
>>>
>>> However, my point was less about generalization but more about
>>> introducing CONFIG_MAX_STATIC_PORT.
>>>
>>> It seems strange to let the admin to decide the maximum number of static
>>> port supported.
>>
>> Why (assuming s/admin/build admin/)? I view both a build time upper bound
>> as well as (alternatively) a command line driven upper bound as reasonable
>> (in the latter case there of course still would want/need to be an upper
>> bound on what is legitimate to specify from the command line). Using
>> static ports after all means there's a static largest port number.
>> Determined across all intended uses of a build such an upper bound can be
>> a feasible mechanism.
> 
> I agree with this. Right now all static port must be defined on boot so this is fully
> ok to have a maximum compilation value or something that can be customised
> on command line.
> In this case the admin must be fully aware of what he does from the start.

Let me start that I think it would be a mistake to introduce a command 
line option (or Kconfig) that have high chance to disappear or would 
become moot.

Today we already have a per-domain limit for the number of event 
channels (see d->max_evtchn_port). If we leave aside the issue in the 
event channel code, it is not clear to me why we need a separate limit 
for statically allocated event channel.

Cheers,

-- 
Julien Grall

Re: [PATCH 2/8] xen/evtchn: modify evtchn_alloc_unbound to allocate specified port

[Jan Beulich]

On 13.07.2022 12:18, Julien Grall wrote:
> On 13/07/2022 10:53, Jan Beulich wrote:
>> On 13.07.2022 11:35, Julien Grall wrote:
>>> On 13/07/2022 07:21, Jan Beulich wrote:
>>>>>> For the FIFO issue, we can introduce the new config option to restrict the maximum number of static
>>>>>> port supported in Xen. We can check the user-defined static port when we parse the device tree and if
>>>>>> a user-defined static port is greater than the maximum allowed static port will return an error to the user.
>>>>>> In this way, we can avoid allocating a lot of memory to fill the hole.
>>>>>>
>>>>>> Let me know your view on this.
>>>>>>
>>>>>> config MAX_STATIC_PORT
>>>>>>        int "Maximum number of static ports”
>>>>>>        range 1 4095
>>>>>>        help
>>>>>>           Controls the build-time maximum number of static port supported
>>>>>
>>>>> The problem is not exclusive to the static event channel. So I don't
>>>>> think this is right to introduce MAX_STATIC_PORT to mitigate the issue
>>>>> (even though this is the only user today).
>>>>>
>>>>> A few of alternative solutions:
>>>>>      1) Handle preemption in alloc_evtchn_bucket()
>>>>>      2) Allocate all the buckets when the domain is created (the max
>>>>> numbers event channel is known). We may need to think about preemption
>>>>>      3) Tweak is_port_valid() to check if the bucket is valid. This would
>>>>> introduce a couple of extra memory access (might be OK as the bucket
>>>>> would be accessed afterwards) and we would need to update some users.
>>>>>
>>>>> At the moment, 3) is appealing me the most. I would be interested to
>>>>> have an opionions from the other maintainers.
>>>>
>>>> Fwiw of the named alternatives I would also prefer 3. Whether things
>>>> really need generalizing at this point I'm not sure, though.
>>> I am worry that we may end up to forget that we had non-generaic way
>>> (e.g. MAX_STATIC_PORT) to prevent trigger the issue. So we could end up
>>> to mistakenly introduce a security issue.
>>>
>>> However, my point was less about generalization but more about
>>> introducing CONFIG_MAX_STATIC_PORT.
>>>
>>> It seems strange to let the admin to decide the maximum number of static
>>> port supported.
>>
>> Why (assuming s/admin/build admin/)? I view both a build time upper bound
>> as well as (alternatively) a command line driven upper bound as reasonable
>> (in the latter case there of course still would want/need to be an upper
>> bound on what is legitimate to specify from the command line). Using
>> static ports after all means there's a static largest port number.
>> Determined across all intended uses of a build such an upper bound can be
>> a feasible mechanism.
> 
> AFAICT, the limit is only here to mitigate the risk with the patch I 
> proposed. With a proper fix, the limit would be articial and therefore 
> it is not clear why the admin should be able to configure it (even 
> temporarily).

The limit would be as artificial as other limits we enforce. I can't
see why it would be wrong to have a more tight limit on static ports
than on traditional ("dynamic") ones. Even if only to make sure so
many dynamic ones are left. That said, ...

> Instead, I think we want to have a limit that apply for both statically 
> and dynamically allocated even channel. This is what d->max_evtchn_port 
> is for.

... I also have no issue with following your way of thinking. I view
both perspectives as valid ones to take.

Jan

Re: [PATCH 2/8] xen/evtchn: modify evtchn_alloc_unbound to allocate specified port

[Julien Grall]

Hi Jan,

On 13/07/2022 11:56, Jan Beulich wrote:
> On 13.07.2022 12:18, Julien Grall wrote:
>> On 13/07/2022 10:53, Jan Beulich wrote:
>>> On 13.07.2022 11:35, Julien Grall wrote:
>>>> On 13/07/2022 07:21, Jan Beulich wrote:
>>>>>>> For the FIFO issue, we can introduce the new config option to restrict the maximum number of static
>>>>>>> port supported in Xen. We can check the user-defined static port when we parse the device tree and if
>>>>>>> a user-defined static port is greater than the maximum allowed static port will return an error to the user.
>>>>>>> In this way, we can avoid allocating a lot of memory to fill the hole.
>>>>>>>
>>>>>>> Let me know your view on this.
>>>>>>>
>>>>>>> config MAX_STATIC_PORT
>>>>>>>         int "Maximum number of static ports”
>>>>>>>         range 1 4095
>>>>>>>         help
>>>>>>>            Controls the build-time maximum number of static port supported
>>>>>>
>>>>>> The problem is not exclusive to the static event channel. So I don't
>>>>>> think this is right to introduce MAX_STATIC_PORT to mitigate the issue
>>>>>> (even though this is the only user today).
>>>>>>
>>>>>> A few of alternative solutions:
>>>>>>       1) Handle preemption in alloc_evtchn_bucket()
>>>>>>       2) Allocate all the buckets when the domain is created (the max
>>>>>> numbers event channel is known). We may need to think about preemption
>>>>>>       3) Tweak is_port_valid() to check if the bucket is valid. This would
>>>>>> introduce a couple of extra memory access (might be OK as the bucket
>>>>>> would be accessed afterwards) and we would need to update some users.
>>>>>>
>>>>>> At the moment, 3) is appealing me the most. I would be interested to
>>>>>> have an opionions from the other maintainers.
>>>>>
>>>>> Fwiw of the named alternatives I would also prefer 3. Whether things
>>>>> really need generalizing at this point I'm not sure, though.
>>>> I am worry that we may end up to forget that we had non-generaic way
>>>> (e.g. MAX_STATIC_PORT) to prevent trigger the issue. So we could end up
>>>> to mistakenly introduce a security issue.
>>>>
>>>> However, my point was less about generalization but more about
>>>> introducing CONFIG_MAX_STATIC_PORT.
>>>>
>>>> It seems strange to let the admin to decide the maximum number of static
>>>> port supported.
>>>
>>> Why (assuming s/admin/build admin/)? I view both a build time upper bound
>>> as well as (alternatively) a command line driven upper bound as reasonable
>>> (in the latter case there of course still would want/need to be an upper
>>> bound on what is legitimate to specify from the command line). Using
>>> static ports after all means there's a static largest port number.
>>> Determined across all intended uses of a build such an upper bound can be
>>> a feasible mechanism.
>>
>> AFAICT, the limit is only here to mitigate the risk with the patch I
>> proposed. With a proper fix, the limit would be articial and therefore
>> it is not clear why the admin should be able to configure it (even
>> temporarily).
> 
> The limit would be as artificial as other limits we enforce. 

You are right. But we need to be cautious in adding new one that 
overlaps with existing one.

> I can't
> see why it would be wrong to have a more tight limit on static ports
> than on traditional ("dynamic") ones. Even if only to make sure so
> many dynamic ones are left.

This is similar to Xen forbidding to close a static port: it is not the 
hypervisor business to check that there are enough event channel ports 
freed for dynamic allocation.

Cheers,

-- 
Julien Grall

Re: [PATCH 2/8] xen/evtchn: modify evtchn_alloc_unbound to allocate specified port

[Bertrand Marquis]

Hi,

> On 13 Jul 2022, at 12:31, Julien Grall <julien@xen.org> wrote:
> 
> Hi Jan,
> 
> On 13/07/2022 11:56, Jan Beulich wrote:
>> On 13.07.2022 12:18, Julien Grall wrote:
>>> On 13/07/2022 10:53, Jan Beulich wrote:
>>>> On 13.07.2022 11:35, Julien Grall wrote:
>>>>> On 13/07/2022 07:21, Jan Beulich wrote:
>>>>>>>> For the FIFO issue, we can introduce the new config option to restrict the maximum number of static
>>>>>>>> port supported in Xen. We can check the user-defined static port when we parse the device tree and if
>>>>>>>> a user-defined static port is greater than the maximum allowed static port will return an error to the user.
>>>>>>>> In this way, we can avoid allocating a lot of memory to fill the hole.
>>>>>>>> 
>>>>>>>> Let me know your view on this.
>>>>>>>> 
>>>>>>>> config MAX_STATIC_PORT
>>>>>>>> int "Maximum number of static ports”
>>>>>>>> range 1 4095
>>>>>>>> help
>>>>>>>> Controls the build-time maximum number of static port supported
>>>>>>> 
>>>>>>> The problem is not exclusive to the static event channel. So I don't
>>>>>>> think this is right to introduce MAX_STATIC_PORT to mitigate the issue
>>>>>>> (even though this is the only user today).
>>>>>>> 
>>>>>>> A few of alternative solutions:
>>>>>>> 1) Handle preemption in alloc_evtchn_bucket()
>>>>>>> 2) Allocate all the buckets when the domain is created (the max
>>>>>>> numbers event channel is known). We may need to think about preemption
>>>>>>> 3) Tweak is_port_valid() to check if the bucket is valid. This would
>>>>>>> introduce a couple of extra memory access (might be OK as the bucket
>>>>>>> would be accessed afterwards) and we would need to update some users.
>>>>>>> 
>>>>>>> At the moment, 3) is appealing me the most. I would be interested to
>>>>>>> have an opionions from the other maintainers.
>>>>>> 
>>>>>> Fwiw of the named alternatives I would also prefer 3. Whether things
>>>>>> really need generalizing at this point I'm not sure, though.
>>>>> I am worry that we may end up to forget that we had non-generaic way
>>>>> (e.g. MAX_STATIC_PORT) to prevent trigger the issue. So we could end up
>>>>> to mistakenly introduce a security issue.
>>>>> 
>>>>> However, my point was less about generalization but more about
>>>>> introducing CONFIG_MAX_STATIC_PORT.
>>>>> 
>>>>> It seems strange to let the admin to decide the maximum number of static
>>>>> port supported.
>>>> 
>>>> Why (assuming s/admin/build admin/)? I view both a build time upper bound
>>>> as well as (alternatively) a command line driven upper bound as reasonable
>>>> (in the latter case there of course still would want/need to be an upper
>>>> bound on what is legitimate to specify from the command line). Using
>>>> static ports after all means there's a static largest port number.
>>>> Determined across all intended uses of a build such an upper bound can be
>>>> a feasible mechanism.
>>> 
>>> AFAICT, the limit is only here to mitigate the risk with the patch I
>>> proposed. With a proper fix, the limit would be articial and therefore
>>> it is not clear why the admin should be able to configure it (even
>>> temporarily).
>> The limit would be as artificial as other limits we enforce. 
> 
> You are right. But we need to be cautious in adding new one that overlaps with existing one.
> 
>> I can't
>> see why it would be wrong to have a more tight limit on static ports
>> than on traditional ("dynamic") ones. Even if only to make sure so
>> many dynamic ones are left.
> 
> This is similar to Xen forbidding to close a static port: it is not the hypervisor business to check that there are enough event channel ports freed for dynamic allocation.

On other side we need to be cautious not to add too much complexity in the code by trying to make things always magically work.
If you want Xen to be accessible to non expert by magically working all the time, there would be a lot of work to do.

Cheers
Bertrand

> 
> Cheers,
> 
> -- 
> Julien Grall

Re: [PATCH 2/8] xen/evtchn: modify evtchn_alloc_unbound to allocate specified port

[Julien Grall]

On 13/07/2022 13:12, Bertrand Marquis wrote:
>> On 13 Jul 2022, at 12:31, Julien Grall <julien@xen.org> wrote:
>>> I can't
>>> see why it would be wrong to have a more tight limit on static ports
>>> than on traditional ("dynamic") ones. Even if only to make sure so
>>> many dynamic ones are left.
>>
>> This is similar to Xen forbidding to close a static port: it is not the hypervisor business to check that there are enough event channel ports freed for dynamic allocation.
> 
> On other side we need to be cautious not to add too much complexity in the code by trying to make things always magically work.
> If you want Xen to be accessible to non expert by magically working all the time, there would be a lot of work to do.

It is not clear to me whether you are referring to a developper or admin 
here.

On the admin side, we need to make sure they have an easy way to 
configure event channels. One knob is always going to easier than two knobs.

On the developper side, this could be resolved by better documentation 
in the code/interface.

Cheers,

-- 
Julien Grall

Re: [PATCH 2/8] xen/evtchn: modify evtchn_alloc_unbound to allocate specified port

[Rahul Singh]

Hi ,

> On 13 Jul 2022, at 1:29 pm, Julien Grall <julien@xen.org> wrote:
> 
> 
> 
> On 13/07/2022 13:12, Bertrand Marquis wrote:
>>> On 13 Jul 2022, at 12:31, Julien Grall <julien@xen.org> wrote:
>>>> I can't
>>>> see why it would be wrong to have a more tight limit on static ports
>>>> than on traditional ("dynamic") ones. Even if only to make sure so
>>>> many dynamic ones are left.
>>> 
>>> This is similar to Xen forbidding to close a static port: it is not the hypervisor business to check that there are enough event channel ports freed for dynamic allocation.
>> On other side we need to be cautious not to add too much complexity in the code by trying to make things always magically work.
>> If you want Xen to be accessible to non expert by magically working all the time, there would be a lot of work to do.
> 
> It is not clear to me whether you are referring to a developper or admin here.
> 
> On the admin side, we need to make sure they have an easy way to configure event channels. One knob is always going to easier than two knobs.
> 
> On the developper side, this could be resolved by better documentation in the code/interface.
> 
> Cheers,

To conclude the discussion, If everyone agree I will add the below patch or similar in the next version to restrict the
max number of evtchn supported as suggested.


diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 532e50e321..a8c5825a4f 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -3422,7 +3422,7 @@ 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,
+            .max_evtchn_port = MAX_EVTCHNS_PORT,
             .max_grant_frames = -1,
             .max_maptrack_frames = -1,
             .grant_opts = XEN_DOMCTL_GRANT_version(opt_gnttab_max_version),
@@ -3582,7 +3582,7 @@ void __init create_dom0(void)
     struct domain *dom0;
     struct xen_domctl_createdomain dom0_cfg = {
         .flags = XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap,
-        .max_evtchn_port = -1,
+        .max_evtchn_port = MAX_EVTCHNS_PORT,
         .max_grant_frames = gnttab_dom0_frames(),
         .max_maptrack_frames = -1,
         .grant_opts = XEN_DOMCTL_GRANT_version(opt_gnttab_max_version),
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index f08b07b8de..b1f95fbe1a 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -763,7 +763,7 @@ static struct domain *__init create_dom0(const module_t *image,
 {
     struct xen_domctl_createdomain dom0_cfg = {
         .flags = IS_ENABLED(CONFIG_TBOOT) ? XEN_DOMCTL_CDF_s3_integrity : 0,
-        .max_evtchn_port = -1,
+        .max_evtchn_port = MAX_EVTCHNS_PORT,
         .max_grant_frames = -1,
         .max_maptrack_frames = -1,
         .grant_opts = XEN_DOMCTL_GRANT_version(opt_gnttab_max_version),
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index d6c029020f..783359f733 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -76,6 +76,8 @@ extern domid_t hardware_domid;
 /* Maximum number of event channels for any ABI. */
 #define MAX_NR_EVTCHNS MAX(EVTCHN_2L_NR_CHANNELS, EVTCHN_FIFO_NR_CHANNELS)
 
+#define MAX_EVTCHNS_PORT 4096
+
 #define EVTCHNS_PER_BUCKET (PAGE_SIZE / next_power_of_2(sizeof(struct evtchn)))
 #define EVTCHNS_PER_GROUP  (BUCKETS_PER_GROUP * EVTCHNS_PER_BUCKET)
 #define NR_EVTCHN_GROUPS   DIV_ROUND_UP(MAX_NR_EVTCHNS, EVTCHNS_PER_GROUP

Regards,
Rahul

Re: [PATCH 2/8] xen/evtchn: modify evtchn_alloc_unbound to allocate specified port

[Julien Grall]

Hi Rahul,

On 20/07/2022 10:59, Rahul Singh wrote:
>> On 13 Jul 2022, at 1:29 pm, Julien Grall <julien@xen.org> wrote:
>>
>>
>>
>> On 13/07/2022 13:12, Bertrand Marquis wrote:
>>>> On 13 Jul 2022, at 12:31, Julien Grall <julien@xen.org> wrote:
>>>>> I can't
>>>>> see why it would be wrong to have a more tight limit on static ports
>>>>> than on traditional ("dynamic") ones. Even if only to make sure so
>>>>> many dynamic ones are left.
>>>>
>>>> This is similar to Xen forbidding to close a static port: it is not the hypervisor business to check that there are enough event channel ports freed for dynamic allocation.
>>> On other side we need to be cautious not to add too much complexity in the code by trying to make things always magically work.
>>> If you want Xen to be accessible to non expert by magically working all the time, there would be a lot of work to do.
>>
>> It is not clear to me whether you are referring to a developper or admin here.
>>
>> On the admin side, we need to make sure they have an easy way to configure event channels. One knob is always going to easier than two knobs.
>>
>> On the developper side, this could be resolved by better documentation in the code/interface.
>>
>> Cheers,
> 
> To conclude the discussion, If everyone agree I will add the below patch or similar in the next version to restrict the
> max number of evtchn supported as suggested.

I am fine if the limit for domU is fixed by Xen for now. However, for 
dom0, 4096 is potentially too low if you have many PV drivers (each 
backend will need a few event channels). So I don't think this wants to 
be fixed by Xen.

I am not entirely sure we want to limit the number of event channels for 
dom0. But if you want to, then this will have to be done via a command 
line option (or device-tree property).

Cheers,

-- 
Julien Grall

Re: [PATCH 2/8] xen/evtchn: modify evtchn_alloc_unbound to allocate specified port

[Jan Beulich]

On 20.07.2022 13:16, Julien Grall wrote:
> On 20/07/2022 10:59, Rahul Singh wrote:
>>> On 13 Jul 2022, at 1:29 pm, Julien Grall <julien@xen.org> wrote:
>>> On 13/07/2022 13:12, Bertrand Marquis wrote:
>>>>> On 13 Jul 2022, at 12:31, Julien Grall <julien@xen.org> wrote:
>>>>>> I can't
>>>>>> see why it would be wrong to have a more tight limit on static ports
>>>>>> than on traditional ("dynamic") ones. Even if only to make sure so
>>>>>> many dynamic ones are left.
>>>>>
>>>>> This is similar to Xen forbidding to close a static port: it is not the hypervisor business to check that there are enough event channel ports freed for dynamic allocation.
>>>> On other side we need to be cautious not to add too much complexity in the code by trying to make things always magically work.
>>>> If you want Xen to be accessible to non expert by magically working all the time, there would be a lot of work to do.
>>>
>>> It is not clear to me whether you are referring to a developper or admin here.
>>>
>>> On the admin side, we need to make sure they have an easy way to configure event channels. One knob is always going to easier than two knobs.
>>>
>>> On the developper side, this could be resolved by better documentation in the code/interface.
>>>
>>> Cheers,
>>
>> To conclude the discussion, If everyone agree I will add the below patch or similar in the next version to restrict the
>> max number of evtchn supported as suggested.
> 
> I am fine if the limit for domU is fixed by Xen for now. However, for 
> dom0, 4096 is potentially too low if you have many PV drivers (each 
> backend will need a few event channels). So I don't think this wants to 
> be fixed by Xen.
> 
> I am not entirely sure we want to limit the number of event channels for 
> dom0. But if you want to, then this will have to be done via a command 
> line option (or device-tree property).

Imo there would need to be a very good reason to limit Dom0's port range.

Jan

Re: [PATCH 2/8] xen/evtchn: modify evtchn_alloc_unbound to allocate specified port

[Rahul Singh]

Hi Julien,

> On 20 Jul 2022, at 12:16 pm, Julien Grall <julien@xen.org> wrote:
> 
> Hi Rahul,
> 
> On 20/07/2022 10:59, Rahul Singh wrote:
>>> On 13 Jul 2022, at 1:29 pm, Julien Grall <julien@xen.org> wrote:
>>> 
>>> 
>>> 
>>> On 13/07/2022 13:12, Bertrand Marquis wrote:
>>>>> On 13 Jul 2022, at 12:31, Julien Grall <julien@xen.org> wrote:
>>>>>> I can't
>>>>>> see why it would be wrong to have a more tight limit on static ports
>>>>>> than on traditional ("dynamic") ones. Even if only to make sure so
>>>>>> many dynamic ones are left.
>>>>> 
>>>>> This is similar to Xen forbidding to close a static port: it is not the hypervisor business to check that there are enough event channel ports freed for dynamic allocation.
>>>> On other side we need to be cautious not to add too much complexity in the code by trying to make things always magically work.
>>>> If you want Xen to be accessible to non expert by magically working all the time, there would be a lot of work to do.
>>> 
>>> It is not clear to me whether you are referring to a developper or admin here.
>>> 
>>> On the admin side, we need to make sure they have an easy way to configure event channels. One knob is always going to easier than two knobs.
>>> 
>>> On the developper side, this could be resolved by better documentation in the code/interface.
>>> 
>>> Cheers,
>> To conclude the discussion, If everyone agree I will add the below patch or similar in the next version to restrict the
>> max number of evtchn supported as suggested.
> 
> I am fine if the limit for domU is fixed by Xen for now. However, for dom0, 4096 is potentially too low if you have many PV drivers (each backend will need a few event channels). So I don't think this wants to be fixed by Xen.
Agree.
> 
> I am not entirely sure we want to limit the number of event channels for dom0. But if you want to, then this will have to be done via a command line option (or device-tree property).

We need to support the static event channel for dom0 also, in that case, we need to restrict the max number of evtchn for dom0 to mitigate the security issue.
I am okay with introducing the new command line option "max_event_channels” for dom0 and setting the default value to 4096.

If the admin set the value greater than 4096 (or what we agreed on) and static event channel support is enabled we will print the warning to the user related to fill
the hole issue for FIFO ABI.

Regards,
Rahul

Re: [PATCH 2/8] xen/evtchn: modify evtchn_alloc_unbound to allocate specified port

[Julien Grall]

On 21/07/2022 13:50, Rahul Singh wrote:
> Hi Julien,

Hi Rahul,

>> On 20 Jul 2022, at 12:16 pm, Julien Grall <julien@xen.org> wrote:
>>
>> Hi Rahul,
>>
>> On 20/07/2022 10:59, Rahul Singh wrote:
>>>> On 13 Jul 2022, at 1:29 pm, Julien Grall <julien@xen.org> wrote:
>>>>
>>>>
>>>>
>>>> On 13/07/2022 13:12, Bertrand Marquis wrote:
>>>>>> On 13 Jul 2022, at 12:31, Julien Grall <julien@xen.org> wrote:
>>>>>>> I can't
>>>>>>> see why it would be wrong to have a more tight limit on static ports
>>>>>>> than on traditional ("dynamic") ones. Even if only to make sure so
>>>>>>> many dynamic ones are left.
>>>>>>
>>>>>> This is similar to Xen forbidding to close a static port: it is not the hypervisor business to check that there are enough event channel ports freed for dynamic allocation.
>>>>> On other side we need to be cautious not to add too much complexity in the code by trying to make things always magically work.
>>>>> If you want Xen to be accessible to non expert by magically working all the time, there would be a lot of work to do.
>>>>
>>>> It is not clear to me whether you are referring to a developper or admin here.
>>>>
>>>> On the admin side, we need to make sure they have an easy way to configure event channels. One knob is always going to easier than two knobs.
>>>>
>>>> On the developper side, this could be resolved by better documentation in the code/interface.
>>>>
>>>> Cheers,
>>> To conclude the discussion, If everyone agree I will add the below patch or similar in the next version to restrict the
>>> max number of evtchn supported as suggested.
>>
>> I am fine if the limit for domU is fixed by Xen for now. However, for dom0, 4096 is potentially too low if you have many PV drivers (each backend will need a few event channels). So I don't think this wants to be fixed by Xen.
> Agree.
>>
>> I am not entirely sure we want to limit the number of event channels for dom0. But if you want to, then this will have to be done via a command line option (or device-tree property).
> 
> We need to support the static event channel for dom0 also, in that case, we need to restrict the max number of evtchn for dom0 to mitigate the security issue.

It sounds like there are some misundertanding or I am missing some 
context. The static event channels will be allocated at boot, so the 
worse that can happen is it will be slower to boot.

My point regarding fifo was more in the generic case of allowing the 
caller to select the port. This would be a concern in the context of 
non-cooperative live-migration. An easy way is to restrict the number of 
ports. For you, this is just an increase in boot time.

Furthermore, there is an issue for dom0less domUs because we don't limit 
the number of port by default. This means that a domU can allocate a 
large amount of memory in Xen (we need some per-event channel state). 
Hence why I suggested to update max_evtchn_channel.

> If the admin set the value greater than 4096 (or what we agreed on) and static event channel support is enabled we will print the warning to the user related to fill
> the hole issue for FIFO ABI.

See above. I don't see the need for a warning. The admin will notice 
that it is slower to boot.

See above.

Cheers,


> 
> Regards,
> Rahul
> 

-- 
Julien Grall

Re: [PATCH 2/8] xen/evtchn: modify evtchn_alloc_unbound to allocate specified port

[Rahul Singh]

Hi Julien,

> On 21 Jul 2022, at 2:29 pm, Julien Grall <julien@xen.org> wrote:
> 
> On 21/07/2022 13:50, Rahul Singh wrote:
>> Hi Julien,
> 
> Hi Rahul,
> 
>>> On 20 Jul 2022, at 12:16 pm, Julien Grall <julien@xen.org> wrote:
>>> 
>>> Hi Rahul,
>>> 
>>> On 20/07/2022 10:59, Rahul Singh wrote:
>>>>> On 13 Jul 2022, at 1:29 pm, Julien Grall <julien@xen.org> wrote:
>>>>> 
>>>>> 
>>>>> 
>>>>> On 13/07/2022 13:12, Bertrand Marquis wrote:
>>>>>>> On 13 Jul 2022, at 12:31, Julien Grall <julien@xen.org> wrote:
>>>>>>>> I can't
>>>>>>>> see why it would be wrong to have a more tight limit on static ports
>>>>>>>> than on traditional ("dynamic") ones. Even if only to make sure so
>>>>>>>> many dynamic ones are left.
>>>>>>> 
>>>>>>> This is similar to Xen forbidding to close a static port: it is not the hypervisor business to check that there are enough event channel ports freed for dynamic allocation.
>>>>>> On other side we need to be cautious not to add too much complexity in the code by trying to make things always magically work.
>>>>>> If you want Xen to be accessible to non expert by magically working all the time, there would be a lot of work to do.
>>>>> 
>>>>> It is not clear to me whether you are referring to a developper or admin here.
>>>>> 
>>>>> On the admin side, we need to make sure they have an easy way to configure event channels. One knob is always going to easier than two knobs.
>>>>> 
>>>>> On the developper side, this could be resolved by better documentation in the code/interface.
>>>>> 
>>>>> Cheers,
>>>> To conclude the discussion, If everyone agree I will add the below patch or similar in the next version to restrict the
>>>> max number of evtchn supported as suggested.
>>> 
>>> I am fine if the limit for domU is fixed by Xen for now. However, for dom0, 4096 is potentially too low if you have many PV drivers (each backend will need a few event channels). So I don't think this wants to be fixed by Xen.
>> Agree.
>>> 
>>> I am not entirely sure we want to limit the number of event channels for dom0. But if you want to, then this will have to be done via a command line option (or device-tree property).
>> We need to support the static event channel for dom0 also, in that case, we need to restrict the max number of evtchn for dom0 to mitigate the security issue.
> 
> It sounds like there are some misundertanding or I am missing some context. The static event channels will be allocated at boot, so the worse that can happen is it will be slower to boot.
> 
> My point regarding fifo was more in the generic case of allowing the caller to select the port. This would be a concern in the context of non-cooperative live-migration. An easy way is to restrict the number of ports. For you, this is just an increase in boot time.
> 
> Furthermore, there is an issue for dom0less domUs because we don't limit the number of port by default. This means that a domU can allocate a large amount of memory in Xen (we need some per-event channel state). Hence why I suggested to update max_evtchn_channel.

Thanks for the clarification. 
> 
>> If the admin set the value greater than 4096 (or what we agreed on) and static event channel support is enabled we will print the warning to the user related to fill
>> the hole issue for FIFO ABI.
> 
> See above. I don't see the need for a warning. The admin will notice that it is slower to boot.

Ok. I will not add the warning. Just to confirm again is that okay If I add new command line option "max_event_channels”  in
next version for dom0 to restrict the max number of evtchn.

Regards,
Rahul

Re: [PATCH 2/8] xen/evtchn: modify evtchn_alloc_unbound to allocate specified port

[Julien Grall]

On 21/07/2022 16:37, Rahul Singh wrote:
> Hi Julien,

Hi Rahul,

>> On 21 Jul 2022, at 2:29 pm, Julien Grall <julien@xen.org> wrote:
>>
>> On 21/07/2022 13:50, Rahul Singh wrote:
>>> Hi Julien,
>>
>> Hi Rahul,
>>
>>>> On 20 Jul 2022, at 12:16 pm, Julien Grall <julien@xen.org> wrote:
>>>>
>>>> Hi Rahul,
>>>>
>>>> On 20/07/2022 10:59, Rahul Singh wrote:
>>>>>> On 13 Jul 2022, at 1:29 pm, Julien Grall <julien@xen.org> wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 13/07/2022 13:12, Bertrand Marquis wrote:
>>>>>>>> On 13 Jul 2022, at 12:31, Julien Grall <julien@xen.org> wrote:
>>>>>>>>> I can't
>>>>>>>>> see why it would be wrong to have a more tight limit on static ports
>>>>>>>>> than on traditional ("dynamic") ones. Even if only to make sure so
>>>>>>>>> many dynamic ones are left.
>>>>>>>>
>>>>>>>> This is similar to Xen forbidding to close a static port: it is not the hypervisor business to check that there are enough event channel ports freed for dynamic allocation.
>>>>>>> On other side we need to be cautious not to add too much complexity in the code by trying to make things always magically work.
>>>>>>> If you want Xen to be accessible to non expert by magically working all the time, there would be a lot of work to do.
>>>>>>
>>>>>> It is not clear to me whether you are referring to a developper or admin here.
>>>>>>
>>>>>> On the admin side, we need to make sure they have an easy way to configure event channels. One knob is always going to easier than two knobs.
>>>>>>
>>>>>> On the developper side, this could be resolved by better documentation in the code/interface.
>>>>>>
>>>>>> Cheers,
>>>>> To conclude the discussion, If everyone agree I will add the below patch or similar in the next version to restrict the
>>>>> max number of evtchn supported as suggested.
>>>>
>>>> I am fine if the limit for domU is fixed by Xen for now. However, for dom0, 4096 is potentially too low if you have many PV drivers (each backend will need a few event channels). So I don't think this wants to be fixed by Xen.
>>> Agree.
>>>>
>>>> I am not entirely sure we want to limit the number of event channels for dom0. But if you want to, then this will have to be done via a command line option (or device-tree property).
>>> We need to support the static event channel for dom0 also, in that case, we need to restrict the max number of evtchn for dom0 to mitigate the security issue.
>>
>> It sounds like there are some misundertanding or I am missing some context. The static event channels will be allocated at boot, so the worse that can happen is it will be slower to boot.
>>
>> My point regarding fifo was more in the generic case of allowing the caller to select the port. This would be a concern in the context of non-cooperative live-migration. An easy way is to restrict the number of ports. For you, this is just an increase in boot time.
>>
>> Furthermore, there is an issue for dom0less domUs because we don't limit the number of port by default. This means that a domU can allocate a large amount of memory in Xen (we need some per-event channel state). Hence why I suggested to update max_evtchn_channel.
> 
> Thanks for the clarification.
>>
>>> If the admin set the value greater than 4096 (or what we agreed on) and static event channel support is enabled we will print the warning to the user related to fill
>>> the hole issue for FIFO ABI.
>>
>> See above. I don't see the need for a warning. The admin will notice that it is slower to boot.
> 
> Ok. I will not add the warning. Just to confirm again is that okay If I add new command line option "max_event_channels”  in
> next version for dom0 to restrict the max number of evtchn.

Personally I am fine with a command line option to *globally* restrict 
the number of events channel. But Jan seemed to have some reservation. 
Quoting what he wrote previously:

"Imo there would need to be a very good reason to limit Dom0's port range.
"

Cheers,

-- 
Julien Grall

Re: [PATCH 2/8] xen/evtchn: modify evtchn_alloc_unbound to allocate specified port

[Rahul Singh]

Hi Julien

> On 26 Jul 2022, at 6:37 pm, Julien Grall <julien@xen.org> wrote:
> 
> 
> 
> On 21/07/2022 16:37, Rahul Singh wrote:
>> Hi Julien,
> 
> Hi Rahul,
> 
>>> On 21 Jul 2022, at 2:29 pm, Julien Grall <julien@xen.org> wrote:
>>> 
>>> On 21/07/2022 13:50, Rahul Singh wrote:
>>>> Hi Julien,
>>> 
>>> Hi Rahul,
>>> 
>>>>> On 20 Jul 2022, at 12:16 pm, Julien Grall <julien@xen.org> wrote:
>>>>> 
>>>>> Hi Rahul,
>>>>> 
>>>>> On 20/07/2022 10:59, Rahul Singh wrote:
>>>>>>> On 13 Jul 2022, at 1:29 pm, Julien Grall <julien@xen.org> wrote:
>>>>>>> 
>>>>>>> 
>>>>>>> 
>>>>>>> On 13/07/2022 13:12, Bertrand Marquis wrote:
>>>>>>>>> On 13 Jul 2022, at 12:31, Julien Grall <julien@xen.org> wrote:
>>>>>>>>>> I can't
>>>>>>>>>> see why it would be wrong to have a more tight limit on static ports
>>>>>>>>>> than on traditional ("dynamic") ones. Even if only to make sure so
>>>>>>>>>> many dynamic ones are left.
>>>>>>>>> 
>>>>>>>>> This is similar to Xen forbidding to close a static port: it is not the hypervisor business to check that there are enough event channel ports freed for dynamic allocation.
>>>>>>>> On other side we need to be cautious not to add too much complexity in the code by trying to make things always magically work.
>>>>>>>> If you want Xen to be accessible to non expert by magically working all the time, there would be a lot of work to do.
>>>>>>> 
>>>>>>> It is not clear to me whether you are referring to a developper or admin here.
>>>>>>> 
>>>>>>> On the admin side, we need to make sure they have an easy way to configure event channels. One knob is always going to easier than two knobs.
>>>>>>> 
>>>>>>> On the developper side, this could be resolved by better documentation in the code/interface.
>>>>>>> 
>>>>>>> Cheers,
>>>>>> To conclude the discussion, If everyone agree I will add the below patch or similar in the next version to restrict the
>>>>>> max number of evtchn supported as suggested.
>>>>> 
>>>>> I am fine if the limit for domU is fixed by Xen for now. However, for dom0, 4096 is potentially too low if you have many PV drivers (each backend will need a few event channels). So I don't think this wants to be fixed by Xen.
>>>> Agree.
>>>>> 
>>>>> I am not entirely sure we want to limit the number of event channels for dom0. But if you want to, then this will have to be done via a command line option (or device-tree property).
>>>> We need to support the static event channel for dom0 also, in that case, we need to restrict the max number of evtchn for dom0 to mitigate the security issue.
>>> 
>>> It sounds like there are some misundertanding or I am missing some context. The static event channels will be allocated at boot, so the worse that can happen is it will be slower to boot.
>>> 
>>> My point regarding fifo was more in the generic case of allowing the caller to select the port. This would be a concern in the context of non-cooperative live-migration. An easy way is to restrict the number of ports. For you, this is just an increase in boot time.
>>> 
>>> Furthermore, there is an issue for dom0less domUs because we don't limit the number of port by default. This means that a domU can allocate a large amount of memory in Xen (we need some per-event channel state). Hence why I suggested to update max_evtchn_channel.
>> Thanks for the clarification.
>>> 
>>>> If the admin set the value greater than 4096 (or what we agreed on) and static event channel support is enabled we will print the warning to the user related to fill
>>>> the hole issue for FIFO ABI.
>>> 
>>> See above. I don't see the need for a warning. The admin will notice that it is slower to boot.
>> Ok. I will not add the warning. Just to confirm again is that okay If I add new command line option "max_event_channels” in
>> next version for dom0 to restrict the max number of evtchn.
> 
> Personally I am fine with a command line option to *globally* restrict the number of events channel. But Jan seemed to have some reservation. Quoting what he wrote previously:
> 
> "Imo there would need to be a very good reason to limit Dom0's port range.

As you mentioned, if we don’t restrict the number of events channel for the dom0 system will boot slower.
This is a good reason to restrict the number of event channels for dom0.
 
@Jan: I need your input on this before I send the next version of the patch series.

Regards,
Rahul

Re: [PATCH 2/8] xen/evtchn: modify evtchn_alloc_unbound to allocate specified port

[Jan Beulich]

On 28.07.2022 17:37, Rahul Singh wrote:
>> On 26 Jul 2022, at 6:37 pm, Julien Grall <julien@xen.org> wrote:
>> On 21/07/2022 16:37, Rahul Singh wrote:
>>> Ok. I will not add the warning. Just to confirm again is that okay If I add new command line option "max_event_channels” in
>>> next version for dom0 to restrict the max number of evtchn.
>>
>> Personally I am fine with a command line option to *globally* restrict the number of events channel. But Jan seemed to have some reservation. Quoting what he wrote previously:
>>
>> "Imo there would need to be a very good reason to limit Dom0's port range.
> 
> As you mentioned, if we don’t restrict the number of events channel for the dom0 system will boot slower.
> This is a good reason to restrict the number of event channels for dom0.
>  
> @Jan: I need your input on this before I send the next version of the patch series.

I have to admit that it's not clear to me what you expect - I continue to
think the way expressed before.

Jan

Re: [PATCH 2/8] xen/evtchn: modify evtchn_alloc_unbound to allocate specified port

[Julien Grall]

Hi Rahul,

On 28/07/2022 16:37, Rahul Singh wrote:
> As you mentioned, if we don’t restrict the number of events channel for the dom0 system will boot slower.
> This is a good reason to restrict the number of event channels for dom0.
Let me start that I am still fine if you want to push for a new 
parameter (so long it is not Arm specific). However, I am afraid that I 
will not be able to argue for it because I don't see a strict need for it.

Let me play the devil's advocate for a bit. AFAIU, you would like to 
introduce the new parameter just to tell the admin the boot is going to 
be slower if you use a event channel ID higher than N.

To me this sounds like the same as if an admin decide to use 10GB rather 
than 1GB. There will be slow down.

This slowness is only boot specific and will not vary. So one could 
argue this is easily noticeable and an admin can take remediation.

Given Jan's objection, I would like to propose to document it in the 
bindings instead (a concerned admin will likely read it). Below a rough 
proposal for the documentation:

"It is recommended to use low event channel ID."

Would that be suitable for you?

Cheers,

-- 
Julien Grall

Re: [PATCH 2/8] xen/evtchn: modify evtchn_alloc_unbound to allocate specified port

[Rahul Singh]

Hi Julien

> On 28 Jul 2022, at 9:50 pm, Julien Grall <julien@xen.org> wrote:
> 
> Hi Rahul,
> 
> On 28/07/2022 16:37, Rahul Singh wrote:
>> As you mentioned, if we don’t restrict the number of events channel for the dom0 system will boot slower.
>> This is a good reason to restrict the number of event channels for dom0.
> Let me start that I am still fine if you want to push for a new parameter (so long it is not Arm specific). However, I am afraid that I will not be able to argue for it because I don't see a strict need for it.
> 
> Let me play the devil's advocate for a bit. AFAIU, you would like to introduce the new parameter just to tell the admin the boot is going to be slower if you use a event channel ID higher than N.
> 
> To me this sounds like the same as if an admin decide to use 10GB rather than 1GB. There will be slow down.
> 
> This slowness is only boot specific and will not vary. So one could argue this is easily noticeable and an admin can take remediation.
> 
> Given Jan's objection, I would like to propose to document it in the bindings instead (a concerned admin will likely read it). Below a rough proposal for the documentation:
> 
> "It is recommended to use low event channel ID."
> 
> Would that be suitable for you?

Yes, that will works for me. I will restrict the max event channel for domU only and also add the comment in 
"docs/misc/arm/device-tree/booting.txt” as suggested by you.

Regards,
Rahul

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

[Rahul Singh]

Hi , 

> On 22 Jun 2022, at 3:38 pm, Rahul Singh <Rahul.Singh@arm.com> 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>
> ---
> docs/misc/arm/device-tree/booting.txt |  62 +++++++++++-
> xen/arch/arm/domain_build.c           | 139 ++++++++++++++++++++++++++
> xen/arch/arm/include/asm/domain.h     |   1 +
> xen/arch/arm/include/asm/setup.h      |   1 +
> xen/arch/arm/setup.c                  |   2 +
> 5 files changed, 204 insertions(+), 1 deletion(-)

I am waiting for a review for this patch and the next patch in the series before
I send the next version. Sending this email as a gentle reminder.

Thanks in advance. 

Regards,
Rahul

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

[Julien Grall]

On 05/08/2022 17:10, Rahul Singh wrote:
> Hi ,

Hi Rahul,

>> On 22 Jun 2022, at 3:38 pm, Rahul Singh <Rahul.Singh@arm.com> 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>
>> ---
>> docs/misc/arm/device-tree/booting.txt |  62 +++++++++++-
>> xen/arch/arm/domain_build.c           | 139 ++++++++++++++++++++++++++
>> xen/arch/arm/include/asm/domain.h     |   1 +
>> xen/arch/arm/include/asm/setup.h      |   1 +
>> xen/arch/arm/setup.c                  |   2 +
>> 5 files changed, 204 insertions(+), 1 deletion(-)
> 
> I am waiting for a review for this patch and the next patch in the series before
> I send the next version. Sending this email as a gentle reminder.

I wasn't planning to review this patch and the next one yet because this 
looks mostly parsing. I think this is more important to get patch #1-#6 
correct first.

Cheers,

-- 
Julien Grall

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

[Rahul Singh]

Hi Julien,

> On 5 Aug 2022, at 5:14 pm, Julien Grall <julien@xen.org> wrote:
> 
> 
> 
> On 05/08/2022 17:10, Rahul Singh wrote:
>> Hi ,
> 
> Hi Rahul,
> 
>>> On 22 Jun 2022, at 3:38 pm, Rahul Singh <Rahul.Singh@arm.com> 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>
>>> ---
>>> docs/misc/arm/device-tree/booting.txt |  62 +++++++++++-
>>> xen/arch/arm/domain_build.c           | 139 ++++++++++++++++++++++++++
>>> xen/arch/arm/include/asm/domain.h     |   1 +
>>> xen/arch/arm/include/asm/setup.h      |   1 +
>>> xen/arch/arm/setup.c                  |   2 +
>>> 5 files changed, 204 insertions(+), 1 deletion(-)
>> I am waiting for a review for this patch and the next patch in the series before
>> I send the next version. Sending this email as a gentle reminder.
> 
> I wasn't planning to review this patch and the next one yet because this looks mostly parsing. I think this is more important to get patch #1-#6 correct first.

Make sense, then I will send the next version of patches #1-#6 for review.

Regards,
Rahul 
> 
> Cheers,
> 
> -- 
> Julien Grall