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

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

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

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

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

 docs/misc/arm/device-tree/booting.txt |  63 +++++-
 xen/arch/arm/domain_build.c           | 289 +++++++++++++++++++-------
 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            | 122 +++++++----
 xen/include/xen/event.h               |   8 +-
 xen/include/xen/sched.h               |   3 +
 9 files changed, 373 insertions(+), 119 deletions(-)

-- 
2.25.1

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

[Rahul Singh]

From: Julien Grall <jgrall@amazon.com>

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

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

When using Guest Transparent Migration and LiveUpdate, we will only
preserve ports that are currently in use. As a guest can open/close
event channels, this means the ports may be sparse.

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

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

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

Signed-off-by: Julien Grall <jgrall@amazon.com>
Signed-off-by: Rahul Singh <rahul.singh@arm.com>
---
Changes in v2:
 - new patch in this version to fix the security issue
---
---
 xen/common/event_channel.c | 56 ++++++++++++++++++++++++--------------
 1 file changed, 35 insertions(+), 21 deletions(-)

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

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

[Jan Beulich]

On 19.08.2022 12:02, Rahul Singh wrote:
> From: Julien Grall <jgrall@amazon.com>
> 
> Since commit 01280dc19cf3 "evtchn: simplify port_is_valid()", the event
> channels code assumes that all the buckets below d->valid_evtchns are
> always allocated.
> 
> This assumption hold in most of the situation because a guest is not
> allowed to chose the port. Instead, it will be the first free from port
> 0.
> 
> When using Guest Transparent Migration and LiveUpdate, we will only
> preserve ports that are currently in use. As a guest can open/close
> event channels, this means the ports may be sparse.
> 
> The existing implementation of evtchn_allocate_port() is not able to
> deal with such situation and will end up to override bucket or/and leave
> some bucket unallocated. The latter will result to a droplet crash if
> the event channel belongs to an unallocated bucket.
> 
> This can be solved by making sure that all the buckets below
> d->valid_evtchns are allocated. There should be no impact for most of
> the situation but LM/LU as only one bucket would be allocated. For
> LM/LU, we may end up to allocate multiple buckets if ports in use are
> sparse.
> 
> A potential alternative is to check that the bucket is valid in
> is_port_valid(). This should still possible to do it without taking
> per-domain lock but will result a couple more of memory access.
> 
> Signed-off-by: Julien Grall <jgrall@amazon.com>
> Signed-off-by: Rahul Singh <rahul.singh@arm.com>

While I'm mostly okay with the code, I think the description wants
changing / amending as long as the features talked about above aren't
anywhere near reaching upstream (afaict), to at least _also_ mention
the goal you have with this.

> Changes in v2:
>  - new patch in this version to fix the security issue

I guess you mean "avoid", not "fix".

> @@ -207,30 +216,35 @@ int evtchn_allocate_port(struct domain *d, evtchn_port_t port)
>      }
>      else
>      {
> -        struct evtchn *chn;
> -        struct evtchn **grp;
> -
> -        if ( !group_from_port(d, port) )
> +        do
>          {
> -            grp = xzalloc_array(struct evtchn *, BUCKETS_PER_GROUP);
> -            if ( !grp )
> -                return -ENOMEM;
> -            group_from_port(d, port) = grp;
> -        }
> +            struct evtchn *chn;
> +            struct evtchn **grp;
> +            unsigned int alloc_port = read_atomic(&d->valid_evtchns);
>  
> -        chn = alloc_evtchn_bucket(d, port);
> -        if ( !chn )
> -            return -ENOMEM;
> -        bucket_from_port(d, port) = chn;
> +            if ( !group_from_port(d, alloc_port) )
> +            {
> +                grp = xzalloc_array(struct evtchn *, BUCKETS_PER_GROUP);
> +                if ( !grp )
> +                    return -ENOMEM;
> +                group_from_port(d, alloc_port) = grp;
> +            }
>  
> -        /*
> -         * d->valid_evtchns is used to check whether the bucket can be
> -         * accessed without the per-domain lock. Therefore,
> -         * d->valid_evtchns should be seen *after* the new bucket has
> -         * been setup.
> -         */
> -        smp_wmb();
> -        write_atomic(&d->valid_evtchns, d->valid_evtchns + EVTCHNS_PER_BUCKET);
> +            chn = alloc_evtchn_bucket(d, alloc_port);
> +            if ( !chn )
> +                return -ENOMEM;
> +            bucket_from_port(d, alloc_port) = chn;
> +
> +            /*
> +             * d->valid_evtchns is used to check whether the bucket can be
> +             * accessed without the per-domain lock. Therefore,
> +             * d->valid_evtchns should be seen *after* the new bucket has
> +             * been setup.
> +             */
> +            smp_wmb();
> +            write_atomic(&d->valid_evtchns,
> +                         d->valid_evtchns + EVTCHNS_PER_BUCKET);
> +        } while ( port >= read_atomic(&d->valid_evtchns) );

This updating of d->valid_evtchns looks a little inconsistent to me,
wrt the uses of {read,write}_atomic(). To make obvious that there's
an implicit expectation that no 2nd invocation of this function
could race the updates, I'd recommend reading allocate_port ahead
of the loop and then never again. Here you'd then do

            smp_wmb();
            allocate_port += EVTCHNS_PER_BUCKET;
            write_atomic(&d->valid_evtchns, allocate_port);
        } while ( port >= allocate_port );

at the same time rendering the code (imo) a little more legible.

Jan

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

[Julien Grall]

Hi Jan,

On 22/08/2022 14:08, Jan Beulich wrote:
> On 19.08.2022 12:02, Rahul Singh wrote:
>> From: Julien Grall <jgrall@amazon.com>
>>
>> Since commit 01280dc19cf3 "evtchn: simplify port_is_valid()", the event
>> channels code assumes that all the buckets below d->valid_evtchns are
>> always allocated.
>>
>> This assumption hold in most of the situation because a guest is not
>> allowed to chose the port. Instead, it will be the first free from port
>> 0.
>>
>> When using Guest Transparent Migration and LiveUpdate, we will only
>> preserve ports that are currently in use. As a guest can open/close
>> event channels, this means the ports may be sparse.
>>
>> The existing implementation of evtchn_allocate_port() is not able to
>> deal with such situation and will end up to override bucket or/and leave
>> some bucket unallocated. The latter will result to a droplet crash if
>> the event channel belongs to an unallocated bucket.
>>
>> This can be solved by making sure that all the buckets below
>> d->valid_evtchns are allocated. There should be no impact for most of
>> the situation but LM/LU as only one bucket would be allocated. For
>> LM/LU, we may end up to allocate multiple buckets if ports in use are
>> sparse.
>>
>> A potential alternative is to check that the bucket is valid in
>> is_port_valid(). This should still possible to do it without taking
>> per-domain lock but will result a couple more of memory access.
>>
>> Signed-off-by: Julien Grall <jgrall@amazon.com>
>> Signed-off-by: Rahul Singh <rahul.singh@arm.com>
> 
> While I'm mostly okay with the code, I think the description wants
> changing / amending as long as the features talked about above aren't
> anywhere near reaching upstream (afaict), to at least _also_ mention
> the goal you have with this.

Correct, neither Guest Transparent Migration nor Live-Update is going to 
reach Xen in 4.17 :). Also, if we decide to continue to mention it, then
we would need to s/Guest Transparent Migration/non-cooperative 
migration/ to match the name we decided to use in upstream (see 
docs/designs/non-cooperative-migration.md).

> 
>> Changes in v2:
>>   - new patch in this version to fix the security issue
> 
> I guess you mean "avoid", not "fix".
> 
>> @@ -207,30 +216,35 @@ int evtchn_allocate_port(struct domain *d, evtchn_port_t port)
>>       }
>>       else
>>       {
>> -        struct evtchn *chn;
>> -        struct evtchn **grp;
>> -
>> -        if ( !group_from_port(d, port) )
>> +        do
>>           {
>> -            grp = xzalloc_array(struct evtchn *, BUCKETS_PER_GROUP);
>> -            if ( !grp )
>> -                return -ENOMEM;
>> -            group_from_port(d, port) = grp;
>> -        }
>> +            struct evtchn *chn;
>> +            struct evtchn **grp;
>> +            unsigned int alloc_port = read_atomic(&d->valid_evtchns);
>>   
>> -        chn = alloc_evtchn_bucket(d, port);
>> -        if ( !chn )
>> -            return -ENOMEM;
>> -        bucket_from_port(d, port) = chn;
>> +            if ( !group_from_port(d, alloc_port) )
>> +            {
>> +                grp = xzalloc_array(struct evtchn *, BUCKETS_PER_GROUP);
>> +                if ( !grp )
>> +                    return -ENOMEM;
>> +                group_from_port(d, alloc_port) = grp;
>> +            }
>>   
>> -        /*
>> -         * d->valid_evtchns is used to check whether the bucket can be
>> -         * accessed without the per-domain lock. Therefore,
>> -         * d->valid_evtchns should be seen *after* the new bucket has
>> -         * been setup.
>> -         */
>> -        smp_wmb();
>> -        write_atomic(&d->valid_evtchns, d->valid_evtchns + EVTCHNS_PER_BUCKET);
>> +            chn = alloc_evtchn_bucket(d, alloc_port);
>> +            if ( !chn )
>> +                return -ENOMEM;
>> +            bucket_from_port(d, alloc_port) = chn;
>> +
>> +            /*
>> +             * d->valid_evtchns is used to check whether the bucket can be
>> +             * accessed without the per-domain lock. Therefore,
>> +             * d->valid_evtchns should be seen *after* the new bucket has
>> +             * been setup.
>> +             */
>> +            smp_wmb();
>> +            write_atomic(&d->valid_evtchns,
>> +                         d->valid_evtchns + EVTCHNS_PER_BUCKET);
>> +        } while ( port >= read_atomic(&d->valid_evtchns) );
> 
> This updating of d->valid_evtchns looks a little inconsistent to me,
> wrt the uses of {read,write}_atomic(). To make obvious that there's
> an implicit expectation that no 2nd invocation of this function
> could race the updates, I'd recommend reading allocate_port ahead
> of the loop and then never again. Here you'd then do
> 
>              smp_wmb();
>              allocate_port += EVTCHNS_PER_BUCKET;
>              write_atomic(&d->valid_evtchns, allocate_port);
>          } while ( port >= allocate_port );


I know it is my code. But I agree with this comment :).

> 
> at the same time rendering the code (imo) a little more legible.
> 
> Jan

-- 
Julien Grall

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

[Rahul Singh]

Hi Jan.

> On 22 Aug 2022, at 2:08 pm, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 19.08.2022 12:02, Rahul Singh wrote:
>> From: Julien Grall <jgrall@amazon.com>
>> 
>> Since commit 01280dc19cf3 "evtchn: simplify port_is_valid()", the event
>> channels code assumes that all the buckets below d->valid_evtchns are
>> always allocated.
>> 
>> This assumption hold in most of the situation because a guest is not
>> allowed to chose the port. Instead, it will be the first free from port
>> 0.
>> 
>> When using Guest Transparent Migration and LiveUpdate, we will only
>> preserve ports that are currently in use. As a guest can open/close
>> event channels, this means the ports may be sparse.
>> 
>> The existing implementation of evtchn_allocate_port() is not able to
>> deal with such situation and will end up to override bucket or/and leave
>> some bucket unallocated. The latter will result to a droplet crash if
>> the event channel belongs to an unallocated bucket.
>> 
>> This can be solved by making sure that all the buckets below
>> d->valid_evtchns are allocated. There should be no impact for most of
>> the situation but LM/LU as only one bucket would be allocated. For
>> LM/LU, we may end up to allocate multiple buckets if ports in use are
>> sparse.
>> 
>> A potential alternative is to check that the bucket is valid in
>> is_port_valid(). This should still possible to do it without taking
>> per-domain lock but will result a couple more of memory access.
>> 
>> Signed-off-by: Julien Grall <jgrall@amazon.com>
>> Signed-off-by: Rahul Singh <rahul.singh@arm.com>
> 
> While I'm mostly okay with the code, I think the description wants
> changing / amending as long as the features talked about above aren't
> anywhere near reaching upstream (afaict), to at least _also_ mention
> the goal you have with this.

Ok. I will remove this and add that we need this patch to support static event channel.
Something like:
“ When static event channel support will be added for dom0less domains
  user can request to allocate the evtchn port numbers that are scattered
  in nature."

> 
>> Changes in v2:
>> - new patch in this version to fix the security issue
> 
> I guess you mean "avoid", not "fix".

Ack. 
> 
>> @@ -207,30 +216,35 @@ int evtchn_allocate_port(struct domain *d, evtchn_port_t port)
>>     }
>>     else
>>     {
>> -        struct evtchn *chn;
>> -        struct evtchn **grp;
>> -
>> -        if ( !group_from_port(d, port) )
>> +        do
>>         {
>> -            grp = xzalloc_array(struct evtchn *, BUCKETS_PER_GROUP);
>> -            if ( !grp )
>> -                return -ENOMEM;
>> -            group_from_port(d, port) = grp;
>> -        }
>> +            struct evtchn *chn;
>> +            struct evtchn **grp;
>> +            unsigned int alloc_port = read_atomic(&d->valid_evtchns);
>> 
>> -        chn = alloc_evtchn_bucket(d, port);
>> -        if ( !chn )
>> -            return -ENOMEM;
>> -        bucket_from_port(d, port) = chn;
>> +            if ( !group_from_port(d, alloc_port) )
>> +            {
>> +                grp = xzalloc_array(struct evtchn *, BUCKETS_PER_GROUP);
>> +                if ( !grp )
>> +                    return -ENOMEM;
>> +                group_from_port(d, alloc_port) = grp;
>> +            }
>> 
>> -        /*
>> -         * d->valid_evtchns is used to check whether the bucket can be
>> -         * accessed without the per-domain lock. Therefore,
>> -         * d->valid_evtchns should be seen *after* the new bucket has
>> -         * been setup.
>> -         */
>> -        smp_wmb();
>> -        write_atomic(&d->valid_evtchns, d->valid_evtchns + EVTCHNS_PER_BUCKET);
>> +            chn = alloc_evtchn_bucket(d, alloc_port);
>> +            if ( !chn )
>> +                return -ENOMEM;
>> +            bucket_from_port(d, alloc_port) = chn;
>> +
>> +            /*
>> +             * d->valid_evtchns is used to check whether the bucket can be
>> +             * accessed without the per-domain lock. Therefore,
>> +             * d->valid_evtchns should be seen *after* the new bucket has
>> +             * been setup.
>> +             */
>> +            smp_wmb();
>> +            write_atomic(&d->valid_evtchns,
>> +                         d->valid_evtchns + EVTCHNS_PER_BUCKET);
>> +        } while ( port >= read_atomic(&d->valid_evtchns) );
> 
> This updating of d->valid_evtchns looks a little inconsistent to me,
> wrt the uses of {read,write}_atomic(). To make obvious that there's
> an implicit expectation that no 2nd invocation of this function
> could race the updates, I'd recommend reading allocate_port ahead
> of the loop and then never again. Here you'd then do
> 
>            smp_wmb();
>            allocate_port += EVTCHNS_PER_BUCKET;
>            write_atomic(&d->valid_evtchns, allocate_port);
>        } while ( port >= allocate_port );
> 
> at the same time rendering the code (imo) a little more legible.
> 
> Jan

Ack. I will fix this as suggested by you I next version.

Regards,
Rahul

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

[Rahul Singh]

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

A new wrapper is introduced to either reserved a given port or allocate
an empty one if zero.

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

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

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

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

[Jan Beulich]

On 19.08.2022 12:02, Rahul Singh wrote:
> In a follow-up patch we will be able to either reserve or allocate a
> port for various event channel helpers.

Maybe "... we will want to ..."?

> A new wrapper is introduced to either reserved a given port or allocate
> an empty one if zero.

Maybe a/empty/fresh/ ?

> Take the opportunity to replace the open-coded version in
> evtchn_bind_virq().
> 
> Signed-off-by: Stanislav Kinsburskii <staskins@amazon.com>
> Signed-off-by: Julien Grall <jgrall@amazon.com>
> Signed-off-by: Rahul Singh <rahul.singh@arm.com>

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

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

[Rahul Singh]

Hi Jan,

> On 22 Aug 2022, at 2:45 pm, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 19.08.2022 12:02, Rahul Singh wrote:
>> In a follow-up patch we will be able to either reserve or allocate a
>> port for various event channel helpers.
> 
> Maybe "... we will want to ..."?

Ack.
> 
>> A new wrapper is introduced to either reserved a given port or allocate
>> an empty one if zero.
> 
> Maybe a/empty/fresh/ ?

Ack. I will fix in next version.
 
Regards,
Rahul

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

[Rahul Singh]

Static event channel support will be added for dom0less domains.
Restrict the maximum number of evtchn supported for domUs to avoid
allocating a large amount of memory in Xen.

Signed-off-by: Rahul Singh <rahul.singh@arm.com>
---
Changes in v2:
 - new patch in the version
---
---
 xen/arch/arm/domain_build.c | 2 +-
 xen/include/xen/sched.h     | 3 +++
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 3fd1186b53..6d447367be 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -3277,7 +3277,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),
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index e2b3b6daa3..e5cc4f3e3e 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -76,6 +76,9 @@ 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)
 
+/* Maximum number of event channels supported for domUs. */
+#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)
-- 
2.25.1

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

[Jan Beulich]

On 19.08.2022 12:02, Rahul Singh wrote:
> Static event channel support will be added for dom0less domains.
> Restrict the maximum number of evtchn supported for domUs to avoid
> allocating a large amount of memory in Xen.

Please clarify here how you arrived at 4096 and why you expect no
dom0less DomU would ever want to have more. The limit, after all,
is far below that of FIFO event channels.

> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -3277,7 +3277,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),
> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -76,6 +76,9 @@ 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)
>  
> +/* Maximum number of event channels supported for domUs. */
> +#define MAX_EVTCHNS_PORT 4096

I'm afraid the variable name doesn't express its purpose, and the
comment also claims wider applicability than is actually the case.
It's also not clear whether the constant really needs to live in
the already heavily overloaded xen/sched.h.

Jan

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

[Julien Grall]

Hi Rahul and Jan,

On 22/08/2022 14:49, Jan Beulich wrote:
> On 19.08.2022 12:02, Rahul Singh wrote:
>> Static event channel support will be added for dom0less domains.

I am not sure how this sentence is related to this patch. You...

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

... still need the limit to prevent a domain using more memory because 
at the moment they are unlimited.

> 
> Please clarify here how you arrived at 4096 and why you expect no
> dom0less DomU would ever want to have more. The limit, after all,
> is far below that of FIFO event channels.

I will reply on this because I suggested the limit. A dom0less DomU is 
exactly the same as a DomU created by the toolstack. The default is 1023 
(I originally thought it was 4096).

I would expect that is 1023 is going to be fine by default also for 
dom0less domU as on Arm we don't bind physical interrupts to event 
channels. So the only big use for them is for inter-domain communication.

Therefore, I think it should be ok to default to 1023 if we want 
consistency.

If someone needs more than 1023, we could introduce a per-domain 
device-tree property to override the default maximum.

> 
>> --- a/xen/arch/arm/domain_build.c
>> +++ b/xen/arch/arm/domain_build.c
>> @@ -3277,7 +3277,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),
>> --- a/xen/include/xen/sched.h
>> +++ b/xen/include/xen/sched.h
>> @@ -76,6 +76,9 @@ 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)
>>   
>> +/* Maximum number of event channels supported for domUs. */
>> +#define MAX_EVTCHNS_PORT 4096
> 
> I'm afraid the variable name doesn't express its purpose, and the
> comment also claims wider applicability than is actually the case.
> It's also not clear whether the constant really needs to live in
> the already heavily overloaded xen/sched.h.

IMHO, I think the value would be better hardcoded with an explanation on 
top how we chose the default value.

Cheers,

-- 
Julien Grall

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

[Jan Beulich]

On 23.08.2022 09:56, Julien Grall wrote:
> On 22/08/2022 14:49, Jan Beulich wrote:
>> On 19.08.2022 12:02, Rahul Singh wrote:
>>> --- a/xen/arch/arm/domain_build.c
>>> +++ b/xen/arch/arm/domain_build.c
>>> @@ -3277,7 +3277,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),
>>> --- a/xen/include/xen/sched.h
>>> +++ b/xen/include/xen/sched.h
>>> @@ -76,6 +76,9 @@ 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)
>>>   
>>> +/* Maximum number of event channels supported for domUs. */
>>> +#define MAX_EVTCHNS_PORT 4096
>>
>> I'm afraid the variable name doesn't express its purpose, and the
>> comment also claims wider applicability than is actually the case.
>> It's also not clear whether the constant really needs to live in
>> the already heavily overloaded xen/sched.h.
> 
> IMHO, I think the value would be better hardcoded with an explanation on 
> top how we chose the default value.

Indeed that might be best, at least as long as no 2nd party appears.
What I was actually considering a valid reason for having a constant
in a header was the case of other arches also wanting to support
dom0less, at which point they likely ought to use the same value
without needing to duplicate any commentary or alike.

Jan

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

[Rahul Singh]

Hi Jan,

> On 23 Aug 2022, at 9:29 am, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 23.08.2022 09:56, Julien Grall wrote:
>> On 22/08/2022 14:49, Jan Beulich wrote:
>>> On 19.08.2022 12:02, Rahul Singh wrote:
>>>> --- a/xen/arch/arm/domain_build.c
>>>> +++ b/xen/arch/arm/domain_build.c
>>>> @@ -3277,7 +3277,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),
>>>> --- a/xen/include/xen/sched.h
>>>> +++ b/xen/include/xen/sched.h
>>>> @@ -76,6 +76,9 @@ 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)
>>>> 
>>>> +/* Maximum number of event channels supported for domUs. */
>>>> +#define MAX_EVTCHNS_PORT 4096
>>> 
>>> I'm afraid the variable name doesn't express its purpose, and the
>>> comment also claims wider applicability than is actually the case.
>>> It's also not clear whether the constant really needs to live in
>>> the already heavily overloaded xen/sched.h.
>> 
>> IMHO, I think the value would be better hardcoded with an explanation on 
>> top how we chose the default value.
> 
> Indeed that might be best, at least as long as no 2nd party appears.
> What I was actually considering a valid reason for having a constant
> in a header was the case of other arches also wanting to support
> dom0less, at which point they likely ought to use the same value
> without needing to duplicate any commentary or alike.


If everyone is  okay I will modify the patch as below:

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

Regards,
Rahul

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

[Jan Beulich]

On 23.08.2022 12:39, Rahul Singh wrote:
> Hi Jan,
> 
>> On 23 Aug 2022, at 9:29 am, Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 23.08.2022 09:56, Julien Grall wrote:
>>> On 22/08/2022 14:49, Jan Beulich wrote:
>>>> On 19.08.2022 12:02, Rahul Singh wrote:
>>>>> --- a/xen/arch/arm/domain_build.c
>>>>> +++ b/xen/arch/arm/domain_build.c
>>>>> @@ -3277,7 +3277,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),
>>>>> --- a/xen/include/xen/sched.h
>>>>> +++ b/xen/include/xen/sched.h
>>>>> @@ -76,6 +76,9 @@ 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)
>>>>>
>>>>> +/* Maximum number of event channels supported for domUs. */
>>>>> +#define MAX_EVTCHNS_PORT 4096
>>>>
>>>> I'm afraid the variable name doesn't express its purpose, and the
>>>> comment also claims wider applicability than is actually the case.
>>>> It's also not clear whether the constant really needs to live in
>>>> the already heavily overloaded xen/sched.h.
>>>
>>> IMHO, I think the value would be better hardcoded with an explanation on 
>>> top how we chose the default value.
>>
>> Indeed that might be best, at least as long as no 2nd party appears.
>> What I was actually considering a valid reason for having a constant
>> in a header was the case of other arches also wanting to support
>> dom0less, at which point they likely ought to use the same value
>> without needing to duplicate any commentary or alike.
> 
> 
> If everyone is  okay I will modify the patch as below:

Well, I'm not an Arm maintainer, so my view might not matter, but
if this was a change to code I was a maintainer for, I'd object.
You enforce a limit here which you can't know whether it might
cause issues to anyone.

Jan

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

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

[Bertrand Marquis]

Hi Jan,

> On 23 Aug 2022, at 12:35, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 23.08.2022 12:39, Rahul Singh wrote:
>> Hi Jan,
>> 
>>> On 23 Aug 2022, at 9:29 am, Jan Beulich <jbeulich@suse.com> wrote:
>>> 
>>> On 23.08.2022 09:56, Julien Grall wrote:
>>>> On 22/08/2022 14:49, Jan Beulich wrote:
>>>>> On 19.08.2022 12:02, Rahul Singh wrote:
>>>>>> --- a/xen/arch/arm/domain_build.c
>>>>>> +++ b/xen/arch/arm/domain_build.c
>>>>>> @@ -3277,7 +3277,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),
>>>>>> --- a/xen/include/xen/sched.h
>>>>>> +++ b/xen/include/xen/sched.h
>>>>>> @@ -76,6 +76,9 @@ 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)
>>>>>> 
>>>>>> +/* Maximum number of event channels supported for domUs. */
>>>>>> +#define MAX_EVTCHNS_PORT 4096
>>>>> 
>>>>> I'm afraid the variable name doesn't express its purpose, and the
>>>>> comment also claims wider applicability than is actually the case.
>>>>> It's also not clear whether the constant really needs to live in
>>>>> the already heavily overloaded xen/sched.h.
>>>> 
>>>> IMHO, I think the value would be better hardcoded with an explanation on 
>>>> top how we chose the default value.
>>> 
>>> Indeed that might be best, at least as long as no 2nd party appears.
>>> What I was actually considering a valid reason for having a constant
>>> in a header was the case of other arches also wanting to support
>>> dom0less, at which point they likely ought to use the same value
>>> without needing to duplicate any commentary or alike.
>> 
>> 
>> If everyone is  okay I will modify the patch as below:
> 
> Well, I'm not an Arm maintainer, so my view might not matter, but
> if this was a change to code I was a maintainer for, I'd object.
> You enforce a limit here which you can't know whether it might
> cause issues to anyone.

The limit was agreed and discussed between him and Julien and
I agree with them (if any more views were required).

Not quite sure if your mail was to request an other maintainer to
confirm but done anyway.


Bertrand


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

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

[Julien Grall]

Hi Jan,

On 23/08/2022 12:35, Jan Beulich wrote:
> On 23.08.2022 12:39, Rahul Singh wrote:
>> Hi Jan,
>>
>>> On 23 Aug 2022, at 9:29 am, Jan Beulich <jbeulich@suse.com> wrote:
>>>
>>> On 23.08.2022 09:56, Julien Grall wrote:
>>>> On 22/08/2022 14:49, Jan Beulich wrote:
>>>>> On 19.08.2022 12:02, Rahul Singh wrote:
>>>>>> --- a/xen/arch/arm/domain_build.c
>>>>>> +++ b/xen/arch/arm/domain_build.c
>>>>>> @@ -3277,7 +3277,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),
>>>>>> --- a/xen/include/xen/sched.h
>>>>>> +++ b/xen/include/xen/sched.h
>>>>>> @@ -76,6 +76,9 @@ 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)
>>>>>>
>>>>>> +/* Maximum number of event channels supported for domUs. */
>>>>>> +#define MAX_EVTCHNS_PORT 4096
>>>>>
>>>>> I'm afraid the variable name doesn't express its purpose, and the
>>>>> comment also claims wider applicability than is actually the case.
>>>>> It's also not clear whether the constant really needs to live in
>>>>> the already heavily overloaded xen/sched.h.
>>>>
>>>> IMHO, I think the value would be better hardcoded with an explanation on
>>>> top how we chose the default value.
>>>
>>> Indeed that might be best, at least as long as no 2nd party appears.
>>> What I was actually considering a valid reason for having a constant
>>> in a header was the case of other arches also wanting to support
>>> dom0less, at which point they likely ought to use the same value
>>> without needing to duplicate any commentary or alike.
>>
>>
>> If everyone is  okay I will modify the patch as below:
> 
> Well, I'm not an Arm maintainer, so my view might not matter, but
> if this was a change to code I was a maintainer for, I'd object.
> You enforce a limit here which you can't know whether it might
> cause issues to anyone.

I understand the theory and in general I am not in favor of restricting 
a limit without any data. However, here, I think we have all the data 
necessary that would justify the limit.

In order to use event channels, a guest needs to know which PPI is used 
to notify the guest.

Until recently, we didn't expose the node to dom0less domUs (this was 
introduced when adding support for PV devices). So a guest couldn't 
discover that event channels are used. That said, if the guest figured 
out the PPI (the value can be guessed) then it could potentially use the 
event channels.

However, for Xen on Arm, we are not supporting any guest that don't use 
the firmware tables (e.g. device tree/ACPI). So for such use case, I 
don't care if it breaks because they are relying on unstable information.

What I care about is any user that follow the rules. We only started to 
advertise Xen via Device-Tree to dom0less domUs after 4.16. So I think 
this is fine to restrict the limit now because we haven't released 4.17 yet.

Regarding the default limit, I think it is better to stay consistent 
with libxl and also use 1023. If an admin wants more event channels, 
then we could introduce per-domain property to overwrite the default.

It should not be too difficult to add, but I don't think this is a must.
So I will let Rahul to decide whether he has time to add it.

Cheers,

-- 
Julien Grall

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

[Rahul Singh]

Hi Julien,

> On 23 Aug 2022, at 1:48 pm, Julien Grall <julien@xen.org> wrote:
> 
> Hi Jan,
> 
> On 23/08/2022 12:35, Jan Beulich wrote:
>> On 23.08.2022 12:39, Rahul Singh wrote:
>>> Hi Jan,
>>> 
>>>> On 23 Aug 2022, at 9:29 am, Jan Beulich <jbeulich@suse.com> wrote:
>>>> 
>>>> On 23.08.2022 09:56, Julien Grall wrote:
>>>>> On 22/08/2022 14:49, Jan Beulich wrote:
>>>>>> On 19.08.2022 12:02, Rahul Singh wrote:
>>>>>>> --- a/xen/arch/arm/domain_build.c
>>>>>>> +++ b/xen/arch/arm/domain_build.c
>>>>>>> @@ -3277,7 +3277,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),
>>>>>>> --- a/xen/include/xen/sched.h
>>>>>>> +++ b/xen/include/xen/sched.h
>>>>>>> @@ -76,6 +76,9 @@ 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)
>>>>>>> 
>>>>>>> +/* Maximum number of event channels supported for domUs. */
>>>>>>> +#define MAX_EVTCHNS_PORT 4096
>>>>>> 
>>>>>> I'm afraid the variable name doesn't express its purpose, and the
>>>>>> comment also claims wider applicability than is actually the case.
>>>>>> It's also not clear whether the constant really needs to live in
>>>>>> the already heavily overloaded xen/sched.h.
>>>>> 
>>>>> IMHO, I think the value would be better hardcoded with an explanation on
>>>>> top how we chose the default value.
>>>> 
>>>> Indeed that might be best, at least as long as no 2nd party appears.
>>>> What I was actually considering a valid reason for having a constant
>>>> in a header was the case of other arches also wanting to support
>>>> dom0less, at which point they likely ought to use the same value
>>>> without needing to duplicate any commentary or alike.
>>> 
>>> 
>>> If everyone is  okay I will modify the patch as below:
>> Well, I'm not an Arm maintainer, so my view might not matter, but
>> if this was a change to code I was a maintainer for, I'd object.
>> You enforce a limit here which you can't know whether it might
>> cause issues to anyone.
> 
> I understand the theory and in general I am not in favor of restricting a limit without any data. However, here, I think we have all the data necessary that would justify the limit.
> 
> In order to use event channels, a guest needs to know which PPI is used to notify the guest.
> 
> Until recently, we didn't expose the node to dom0less domUs (this was introduced when adding support for PV devices). So a guest couldn't discover that event channels are used. That said, if the guest figured out the PPI (the value can be guessed) then it could potentially use the event channels.
> 
> However, for Xen on Arm, we are not supporting any guest that don't use the firmware tables (e.g. device tree/ACPI). So for such use case, I don't care if it breaks because they are relying on unstable information.
> 
> What I care about is any user that follow the rules. We only started to advertise Xen via Device-Tree to dom0less domUs after 4.16. So I think this is fine to restrict the limit now because we haven't released 4.17 yet.
> 
> Regarding the default limit, I think it is better to stay consistent with libxl and also use 1023. If an admin wants more event channels, then we could introduce per-domain property to overwrite the default.
> 
> It should not be too difficult to add, but I don't think this is a must.
> So I will let Rahul to decide whether he has time to add it.

I prefer that we will first finish merging the ongoing event channel series.
I have created the task in our backlog, Arm will handle this task in the near future.

Regards,
Rahul

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

[Rahul Singh]

Hi Julien,

> On 23 Aug 2022, at 8:56 am, Julien Grall <julien@xen.org> wrote:
> 
> Hi Rahul and Jan,
> 
> On 22/08/2022 14:49, Jan Beulich wrote:
>> On 19.08.2022 12:02, Rahul Singh wrote:
>>> Static event channel support will be added for dom0less domains.
> 
> I am not sure how this sentence is related to this patch. You...
> 
>>> Restrict the maximum number of evtchn supported for domUs to avoid
>>> allocating a large amount of memory in Xen.
> 
> ... still need the limit to prevent a domain using more memory because at the moment they are unlimited.

Ok. I will remove the sentence.
> 
>> Please clarify here how you arrived at 4096 and why you expect no
>> dom0less DomU would ever want to have more. The limit, after all,
>> is far below that of FIFO event channels.
> 
> I will reply on this because I suggested the limit. A dom0less DomU is exactly the same as a DomU created by the toolstack. The default is 1023 (I originally thought it was 4096).
> 
> I would expect that is 1023 is going to be fine by default also for dom0less domU as on Arm we don't bind physical interrupts to event channels. So the only big use for them is for inter-domain communication.
> 

I will add this information in commit msg.

> Therefore, I think it should be ok to default to 1023 if we want consistency.
> 
> If someone needs more than 1023, we could introduce a per-domain device-tree property to override the default maximum.
> 
>>> --- a/xen/arch/arm/domain_build.c
>>> +++ b/xen/arch/arm/domain_build.c
>>> @@ -3277,7 +3277,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),
>>> --- a/xen/include/xen/sched.h
>>> +++ b/xen/include/xen/sched.h
>>> @@ -76,6 +76,9 @@ 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)
>>>  +/* Maximum number of event channels supported for domUs. */
>>> +#define MAX_EVTCHNS_PORT 4096
>> I'm afraid the variable name doesn't express its purpose, and the
>> comment also claims wider applicability than is actually the case.
>> It's also not clear whether the constant really needs to live in
>> the already heavily overloaded xen/sched.h.
> 
> IMHO, I think the value would be better hardcoded with an explanation on top how we chose the default value.

Ack. 
 
Regards,
Rahul

[PATCH v2 4/7] xen/evtchn: modify evtchn_bind_interdomain to support static evtchn

[Rahul Singh]

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

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

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.

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

Signed-off-by: Rahul Singh <rahul.singh@arm.com>
---
Changes in v2:
 - Merged patches related to evtchn_bind_interdomain in one patch
---
---
 xen/common/event_channel.c | 20 ++++++++++++++------
 xen/include/xen/event.h    |  5 +++++
 2 files changed, 19 insertions(+), 6 deletions(-)

diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
index 194f5346fb..eed0e94d07 100644
--- a/xen/common/event_channel.c
+++ b/xen/common/event_channel.c
@@ -373,11 +373,16 @@ static void double_evtchn_unlock(struct evtchn *lchn, struct evtchn *rchn)
     evtchn_write_unlock(rchn);
 }
 
-static int evtchn_bind_interdomain(evtchn_bind_interdomain_t *bind)
+/*
+ * If lport is zero get the next free port and allocate. If port is non-zero
+ * allocate the specified lport.
+ */
+int evtchn_bind_interdomain(evtchn_bind_interdomain_t *bind, struct domain *ld,
+                            evtchn_port_t lport)
 {
     struct evtchn *lchn, *rchn;
-    struct domain *ld = current->domain, *rd;
-    int            lport, rc;
+    struct domain *rd;
+    int            rc;
     evtchn_port_t  rport = bind->remote_port;
     domid_t        rdom = bind->remote_dom;
 
@@ -397,8 +402,11 @@ static int evtchn_bind_interdomain(evtchn_bind_interdomain_t *bind)
         write_lock(&ld->event_lock);
     }
 
-    if ( (lport = get_free_port(ld)) < 0 )
-        ERROR_EXIT(lport);
+    lport = rc = evtchn_get_port(ld, lport);
+    if ( rc < 0 )
+        ERROR_EXIT(rc);
+    rc = 0;
+
     lchn = evtchn_from_port(ld, lport);
 
     rchn = _evtchn_from_port(rd, rport);
@@ -1231,7 +1239,7 @@ long do_event_channel_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
         struct evtchn_bind_interdomain bind_interdomain;
         if ( copy_from_guest(&bind_interdomain, arg, 1) != 0 )
             return -EFAULT;
-        rc = evtchn_bind_interdomain(&bind_interdomain);
+        rc = evtchn_bind_interdomain(&bind_interdomain, current->domain, 0);
         if ( !rc && __copy_to_guest(arg, &bind_interdomain, 1) )
             rc = -EFAULT; /* Cleaning up here would be a mess! */
         break;
diff --git a/xen/include/xen/event.h b/xen/include/xen/event.h
index f3021fe304..f29b1123d9 100644
--- a/xen/include/xen/event.h
+++ b/xen/include/xen/event.h
@@ -74,6 +74,11 @@ 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,
+                                         struct domain *ld,
+                                         evtchn_port_t port);
+
 /* Unmask a local event-channel port. */
 int evtchn_unmask(unsigned int port);
 
-- 
2.25.1

Re: [PATCH v2 4/7] xen/evtchn: modify evtchn_bind_interdomain to support static evtchn

[Jan Beulich]

On 19.08.2022 12:02, Rahul Singh wrote:
> Static event channel support will be added for dom0less domains. Modify
> evtchn_bind_interdomain to support static evtchn.
> 
> It is necessary to have access to the evtchn_bind_interdomain function
> to do that, so make evtchn_bind_interdomain global and also make it
> __must_check.
> 
> 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.
> 
> evtchn_bind_interdomain() finds the local domain from "current->domain"
> pointer. evtchn_bind_interdomain() will be called from the XEN to create
> static event channel during domain creation. "current" pointer is not
> valid at that time, therefore modify the evtchn_bind_interdomain() to
> pass domain as an argument.
> 
> Signed-off-by: Rahul Singh <rahul.singh@arm.com>

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

Re: [PATCH v2 4/7] xen/evtchn: modify evtchn_bind_interdomain to support static evtchn

[Julien Grall]

Hi Rahul,

On 19/08/2022 11:02, Rahul Singh wrote:
> Static event channel support will be added for dom0less domains. Modify
> evtchn_bind_interdomain to support static evtchn.
> 
> It is necessary to have access to the evtchn_bind_interdomain function
> to do that, so make evtchn_bind_interdomain global and also make it
> __must_check.
> 
> evtchn_bind_interdomain() always allocates the next available local
> port. Static event channel support for dom0less domains requires
> allocating a specified port.

NIT: I first read this as you are trying to describe what the patch 
does. I would add "currently", "at the moment" or similar to make clear 
this is the current behavior.


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

Ditto.

> 
> Signed-off-by: Rahul Singh <rahul.singh@arm.com>

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

Cheers,

-- 
Julien Grall

Re: [PATCH v2 4/7] xen/evtchn: modify evtchn_bind_interdomain to support static evtchn

[Rahul Singh]

Hi Julien,

> On 23 Aug 2022, at 9:23 am, Julien Grall <julien@xen.org> wrote:
> 
> Hi Rahul,
> 
> On 19/08/2022 11:02, Rahul Singh wrote:
>> Static event channel support will be added for dom0less domains. Modify
>> evtchn_bind_interdomain to support static evtchn.
>> It is necessary to have access to the evtchn_bind_interdomain function
>> to do that, so make evtchn_bind_interdomain global and also make it
>> __must_check.
>> evtchn_bind_interdomain() always allocates the next available local
>> port. Static event channel support for dom0less domains requires
>> allocating a specified port.
> 
> NIT: I first read this as you are trying to describe what the patch does. I would add "currently", "at the moment" or similar to make clear this is the current behavior.

Ack.  I will add “currently” in next version.
> 
> 
>> 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.
>> evtchn_bind_interdomain() finds the local domain from "current->domain"
>> pointer. evtchn_bind_interdomain() will be called from the XEN to create
>> static event channel during domain creation. "current" pointer is not
>> valid at that time, therefore modify the evtchn_bind_interdomain() to
>> pass domain as an argument.
> 
> Ditto.

Ack. 
 
Regards,
Rahul

[PATCH v2 5/7] 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>
---
Changes in v2:
 - fix minor comments
---
---
 xen/arch/arm/domain_build.c |  2 +-
 xen/common/event_channel.c  | 17 ++++++++++++-----
 xen/include/xen/event.h     |  3 ++-
 3 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 6d447367be..11a8c6b8b5 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 eed0e94d07..f2d8c2d61a 100644
--- a/xen/common/event_channel.c
+++ b/xen/common/event_channel.c
@@ -316,11 +316,15 @@ static int evtchn_get_port(struct domain *d, evtchn_port_t port)
     return rc ?: port;
 }
 
-int evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc)
+/*
+ * If port is zero get the next free port and allocate. If port is non-zero
+ * allocate the specified port.
+ */
+int evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc, evtchn_port_t port)
 {
     struct evtchn *chn;
     struct domain *d;
-    int            port, rc;
+    int            rc;
     domid_t        dom = alloc->dom;
 
     d = rcu_lock_domain_by_any_id(dom);
@@ -329,8 +333,11 @@ int evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc)
 
     write_lock(&d->event_lock);
 
-    if ( (port = get_free_port(d)) < 0 )
-        ERROR_EXIT_DOM(port, d);
+    port = rc = evtchn_get_port(d, port);
+    if ( rc < 0 )
+        ERROR_EXIT(rc);
+    rc = 0;
+
     chn = evtchn_from_port(d, port);
 
     rc = xsm_evtchn_unbound(XSM_TARGET, d, chn, alloc->remote_dom);
@@ -1229,7 +1236,7 @@ long do_event_channel_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
         struct evtchn_alloc_unbound alloc_unbound;
         if ( copy_from_guest(&alloc_unbound, arg, 1) != 0 )
             return -EFAULT;
-        rc = evtchn_alloc_unbound(&alloc_unbound);
+        rc = evtchn_alloc_unbound(&alloc_unbound, 0);
         if ( !rc && __copy_to_guest(arg, &alloc_unbound, 1) )
             rc = -EFAULT; /* Cleaning up here would be a mess! */
         break;
diff --git a/xen/include/xen/event.h b/xen/include/xen/event.h
index f29b1123d9..8eae9984a9 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

Re: [PATCH v2 5/7] xen/evtchn: modify evtchn_alloc_unbound to allocate specified port

[Jan Beulich]

On 19.08.2022 12:02, 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.
> 
> Signed-off-by: Rahul Singh <rahul.singh@arm.com>

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

As a minor remark: Personally I'd find it more logical if the alloc-unbound
adjustments came ahead of the bind-interdomain ones.

Jan

Re: [PATCH v2 5/7] xen/evtchn: modify evtchn_alloc_unbound to allocate specified port

[Rahul Singh]

Hi Jan,

> On 22 Aug 2022, at 2:57 pm, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 19.08.2022 12:02, 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.
>> 
>> Signed-off-by: Rahul Singh <rahul.singh@arm.com>
> 
> Acked-by: Jan Beulich <jbeulich@suse.com>
> 
> As a minor remark: Personally I'd find it more logical if the alloc-unbound
> adjustments came ahead of the bind-interdomain ones.
>  

I will move this patch before evtchn_bind_interdomain() patch.

Regard,
Rahul

Re: [PATCH v2 5/7] xen/evtchn: modify evtchn_alloc_unbound to allocate specified port

[Julien Grall]

Hi Rahul,

On 19/08/2022 11:02, Rahul Singh wrote:
> evtchn_alloc_unbound() always allocates the next available port. Static
> event channel support for dom0less domains requires allocating a
> specified port.

NIT: Same as patch #4, it is not clear you are talking about the current 
behavior.

> 
> Modify the evtchn_alloc_unbound() to accept the port number as an
> argument and allocate the specified port if available. If the port
> number argument is zero, the next available port will be allocated.
> 
> Signed-off-by: Rahul Singh <rahul.singh@arm.com>

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

Cheers,

-- 
Julien Grall

Re: [PATCH v2 5/7] xen/evtchn: modify evtchn_alloc_unbound to allocate specified port

[Rahul Singh]

Hi Julien,

> On 23 Aug 2022, at 9:31 am, Julien Grall <julien@xen.org> wrote:
> 
> Hi Rahul,
> 
> On 19/08/2022 11:02, Rahul Singh wrote:
>> evtchn_alloc_unbound() always allocates the next available port. Static
>> event channel support for dom0less domains requires allocating a
>> specified port.
> 
> NIT: Same as patch #4, it is not clear you are talking about the current behavior.
> 

Ack. I will add “currently” in next version.
 
Regards,
Rahul

[PATCH v2 6/7] 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>
---
Changes in v2:
 - no change
---
---
 docs/misc/arm/device-tree/booting.txt |  63 +++++++++++-
 xen/arch/arm/domain_build.c           | 136 ++++++++++++++++++++++++++
 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, 202 insertions(+), 1 deletion(-)

diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt
index 98253414b8..ec7dbcaf8f 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,43 @@ The ramdisk sub-node has the following properties:
     property because it will be created by the UEFI stub on boot.
     This option is needed only when UEFI boot is used.
 
+The static event channel sub-node has the following properties:
+
+- compatible
+
+    "xen,evtchn"
+
+- xen,evtchn
+
+    The property is tuples of two numbers
+    (local-evtchn link-to-foreign-evtchn) where:
+
+    local-evtchn is an integer value that will be used to allocate local port
+    for a domain to send and receive event notifications to/from the remote
+    domain. Maximum supported value is 2^17 for FIFO ABI and 4096 for 2L ABI.
+    It is recommended to use low event channel ID.
+
+    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 +309,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 +345,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 11a8c6b8b5..5101bca979 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -3052,6 +3052,141 @@ void __init evtchn_allocate(struct domain *d)
     d->arch.hvm.params[HVM_PARAM_CALLBACK_IRQ] = val;
 }
 
+static const void *__init get_evtchn_dt_property(
+        const struct dt_device_node *np)
+{
+    const void *prop = NULL;
+    uint32_t len;
+
+    prop = dt_get_property(np, "xen,evtchn", &len);
+    if ( !prop )
+        return NULL;
+
+    if ( !len )
+    {
+        printk(XENLOG_ERR "xen,evtchn property cannot be empty.\n");
+        return ERR_PTR(-EINVAL);
+    }
+
+    return prop;
+}
+
+static int __init allocate_domain_evtchn(const struct dt_device_node *node)
+{
+    const void *prop = NULL;
+    const __be32 *cell;
+    uint32_t 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 = get_evtchn_dt_property(evtchn_node);
+        /* If the property is not found, return without errors */
+        if ( !prop || IS_ERR(prop) )
+            return IS_ERR(prop) ? PTR_ERR(prop) : 0;
+
+        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 = get_evtchn_dt_property(remote_node);
+        /* If the property is not found, return without errors */
+        if ( !prop || IS_ERR(prop) )
+            return IS_ERR(prop) ? PTR_ERR(prop) : 0;
+
+        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 && dt_device_is_compatible(node, "multiboot,kernel") )
+            d1 = hardware_domain;
+
+        if ( !d2 && dt_device_is_compatible(dt_get_parent(remote_node),
+                                            "multiboot,kernel") )
+            d2 = hardware_domain;
+
+        if ( !d1 || !d2 )
+        {
+            printk(XENLOG_ERR "evtchn: could not find domains\n" );
+            return -EINVAL;
+        }
+
+        alloc_unbound.dom = d1->domain_id;
+        alloc_unbound.remote_dom = d2->domain_id;
+
+        rc = evtchn_alloc_unbound(&alloc_unbound, domU1_port);
+        if ( rc < 0 && 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);
+        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 +3493,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 cd9ce19b4b..51192b28ee 100644
--- a/xen/arch/arm/include/asm/domain.h
+++ b/xen/arch/arm/include/asm/domain.h
@@ -105,6 +105,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 500307edc0..8eead619ae 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -1063,6 +1063,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

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

[Julien Grall]

Hi Rahul,

On 19/08/2022 11:02, Rahul Singh wrote:
> Introduce a new sub-node under /chosen node to establish static event
> channel communication between domains on dom0less systems.
> 
> An event channel will be created beforehand to allow the domains to
> send notifications to each other.
> 
> Signed-off-by: Rahul Singh <rahul.singh@arm.com>
> ---
> Changes in v2:
>   - no change
> ---
> ---
>   docs/misc/arm/device-tree/booting.txt |  63 +++++++++++-
>   xen/arch/arm/domain_build.c           | 136 ++++++++++++++++++++++++++
>   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, 202 insertions(+), 1 deletion(-)
> 
> diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt
> index 98253414b8..ec7dbcaf8f 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,43 @@ The ramdisk sub-node has the following properties:
>       property because it will be created by the UEFI stub on boot.
>       This option is needed only when UEFI boot is used.
>   
> +The static event channel sub-node has the following properties:
> +
> +- compatible
> +
> +    "xen,evtchn"
> +
> +- xen,evtchn
> +
> +    The property is tuples of two numbers
> +    (local-evtchn link-to-foreign-evtchn) where:
> +
> +    local-evtchn is an integer value that will be used to allocate local port
> +    for a domain to send and receive event notifications to/from the remote
> +    domain. Maximum supported value is 2^17 for FIFO ABI and 4096 for 2L ABI.
> +    It is recommended to use low event channel ID.

I think you are either missing a 'a' or 'ID' should be 'IDs'

> +
> +    link-to-foreign-evtchn is a single phandle to a remote evtchn to which
> +    local-evtchn will be connected.
>   
>   Example
>   =======
>   
>   chosen {
> +
> +    module@0 {
> +        compatible = "multiboot,kernel", "multiboot,module";
> +        xen,uefi-binary = "...";
> +        bootargs = "...";
> +
> +        /* one sub-node per local event channel */
> +        ec1: evtchn@1 {
> +            compatible = "xen,evtchn-v1";
> +            /* local-evtchn link-to-foreign-evtchn */
> +            xen,evtchn = <0xa &ec2>;
> +        };

AFAIU, this is meant to describe the static event channels for dom0. I 
can't find the documentation for it. Do they always need to be a subnode 
the node "multiboot,kernel"?

The reason I am asking is it feels strange to define them below that 
subnode when for domUs, both nodes have the same parent. So I think it 
would make more sense to define them in chosen.

> +    };
> +
>       domU1 {
>           compatible = "xen,domain";
>           #address-cells = <0x2>;
> @@ -277,6 +309,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 +345,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 11a8c6b8b5..5101bca979 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -3052,6 +3052,141 @@ void __init evtchn_allocate(struct domain *d)
>       d->arch.hvm.params[HVM_PARAM_CALLBACK_IRQ] = val;
>   }
>   
> +static const void *__init get_evtchn_dt_property(
> +        const struct dt_device_node *np)
> +{
> +    const void *prop = NULL;
> +    uint32_t len;
> +
> +    prop = dt_get_property(np, "xen,evtchn", &len);
> +    if ( !prop )
> +        return NULL;
> +
> +    if ( !len )
> +    {
> +        printk(XENLOG_ERR "xen,evtchn property cannot be empty.\n")

Looking at the callers, they all assume that there is enough cells in 
the property. So I think you should check the size as well.

> +        return ERR_PTR(-EINVAL);
> +    }
> +
> +    return prop;
> +}
> +
> +static int __init allocate_domain_evtchn(const struct dt_device_node *node)
> +{
> +    const void *prop = NULL;
> +    const __be32 *cell;
> +    uint32_t 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 = get_evtchn_dt_property(evtchn_node);
> +        /* If the property is not found, return without errors */

 From the binding description, the property is not optional. So do we 
want to ignore the error? If you treat it as an error, then ...

> +        if ( !prop || IS_ERR(prop) )
> +            return IS_ERR(prop) ? PTR_ERR(prop) : 0;

... you could return ERR_PTR(-ENOMEM) instead of NULL and then simplify 
this code with:

> +
> +        cell = (const __be32 *)prop;

prop is a void pointer. So the cast is unnecessary.

> +        domU1_port = dt_next_cell(1, &cell);
> +        remote_phandle = dt_next_cell(1, &cell);
The code is also duplicated below for the remote port. I think it would 
be better if this is part of your helper get_evtchn_dt_property().

> +
> +        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 = get_evtchn_dt_property(remote_node);
> +        /* If the property is not found, return without errors */
> +        if ( !prop || IS_ERR(prop) )
> +            return IS_ERR(prop) ? PTR_ERR(prop) : 0;
> +
> +        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;
> +        }

The loop could be avoided if you stash the domid in the field 'used_by' 
of the device-tree node when the domain is created.

> +
> +        if ( !d1 && dt_device_is_compatible(node, "multiboot,kernel") )
> +            d1 = hardware_domain;
> +
> +        if ( !d2 && dt_device_is_compatible(dt_get_parent(remote_node),
> +                                            "multiboot,kernel") )
> +            d2 = hardware_domain;

Any particular reason to handle the hardware domain differently?

> +
> +        if ( !d1 || !d2 )
> +        {
> +            printk(XENLOG_ERR "evtchn: could not find domains\n" );
> +            return -EINVAL;
> +        }
> +
> +        alloc_unbound.dom = d1->domain_id;
> +        alloc_unbound.remote_dom = d2->domain_id;
> +
> +        rc = evtchn_alloc_unbound(&alloc_unbound, domU1_port);
> +        if ( rc < 0 && rc != -EBUSY )

Please explain in a comment why you want to handle -EBUSY differently.

> +        {
> +            printk(XENLOG_ERR
> +                   "evtchn_alloc_unbound() failure (Error %d) \n", rc);
> +            return rc;
> +        }
> +
> +        bind_interdomain.remote_dom  = d1->domain_id;
> +        bind_interdomain.remote_port = domU1_port;
> +
> +        rc = evtchn_bind_interdomain(&bind_interdomain, d2, domU2_port);
> +        if ( rc < 0 && rc != -EBUSY )

AFAIU, EBUSY only tells you the port is been used. It doesn't tell you 
the link is the same. So I think you want to also confirm that to avoid 
to continuing with the wrong setup.

> +        {
> +            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;

AFAICT, all the users below can deal with constisfied node. So I think 
you want to add 'const' here.

> +    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 +3493,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 you follow my suggestion above, this should not be necessary. 
However, if this is still needed for some reason, then I think we should 
also set d->arch.node for the Hardware Domain and ...

>   
>           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 cd9ce19b4b..51192b28ee 100644
> --- a/xen/arch/arm/include/asm/domain.h
> +++ b/xen/arch/arm/include/asm/domain.h
> @@ -105,6 +105,7 @@ struct arch_domain
>   #endif
>   
>       bool directmap;
> +    struct dt_device_node *node;

... this should be const as the node shouldn't be modifiable.

>   }  __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 500307edc0..8eead619ae 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -1063,6 +1063,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).

Cheers,

-- 
Julien Grall

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

[Rahul Singh]

Hi Julien,

> On 23 Aug 2022, at 10:32 am, Julien Grall <julien@xen.org> wrote:
> 
> Hi Rahul,
> 
> On 19/08/2022 11:02, Rahul Singh wrote:
>> Introduce a new sub-node under /chosen node to establish static event
>> channel communication between domains on dom0less systems.
>> An event channel will be created beforehand to allow the domains to
>> send notifications to each other.
>> Signed-off-by: Rahul Singh <rahul.singh@arm.com>
>> ---
>> Changes in v2:
>>  - no change
>> ---
>> ---
>>  docs/misc/arm/device-tree/booting.txt |  63 +++++++++++-
>>  xen/arch/arm/domain_build.c           | 136 ++++++++++++++++++++++++++
>>  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, 202 insertions(+), 1 deletion(-)
>> diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt
>> index 98253414b8..ec7dbcaf8f 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,43 @@ The ramdisk sub-node has the following properties:
>>      property because it will be created by the UEFI stub on boot.
>>      This option is needed only when UEFI boot is used.
>>  +The static event channel sub-node has the following properties:
>> +
>> +- compatible
>> +
>> +    "xen,evtchn"
>> +
>> +- xen,evtchn
>> +
>> +    The property is tuples of two numbers
>> +    (local-evtchn link-to-foreign-evtchn) where:
>> +
>> +    local-evtchn is an integer value that will be used to allocate local port
>> +    for a domain to send and receive event notifications to/from the remote
>> +    domain. Maximum supported value is 2^17 for FIFO ABI and 4096 for 2L ABI.
>> +    It is recommended to use low event channel ID.
> 
> I think you are either missing a 'a' or 'ID' should be 'IDs'

Ack. 
> 
>> +
>> +    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>;
>> +        };
> 
> AFAIU, this is meant to describe the static event channels for dom0. I can't find the documentation for it. Do they always need to be a subnode the node "multiboot,kernel"?
> 
> The reason I am asking is it feels strange to define them below that subnode when for domUs, both nodes have the same parent. So I think it would make more sense to define them in chosen.

Ok. I will move the dom0 evtchn node under chosen done.
> 
>> +    };
>> +
>>      domU1 {
>>          compatible = "xen,domain";
>>          #address-cells = <0x2>;
>> @@ -277,6 +309,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 +345,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 11a8c6b8b5..5101bca979 100644
>> --- a/xen/arch/arm/domain_build.c
>> +++ b/xen/arch/arm/domain_build.c
>> @@ -3052,6 +3052,141 @@ void __init evtchn_allocate(struct domain *d)
>>      d->arch.hvm.params[HVM_PARAM_CALLBACK_IRQ] = val;
>>  }
>>  +static const void *__init get_evtchn_dt_property(
>> +        const struct dt_device_node *np)
>> +{
>> +    const void *prop = NULL;
>> +    uint32_t len;
>> +
>> +    prop = dt_get_property(np, "xen,evtchn", &len);
>> +    if ( !prop )
>> +        return NULL;
>> +
>> +    if ( !len )
>> +    {
>> +        printk(XENLOG_ERR "xen,evtchn property cannot be empty.\n")
> 
> Looking at the callers, they all assume that there is enough cells in the property. So I think you should check the size as well.

Ack, 
> 
>> +        return ERR_PTR(-EINVAL);
>> +    }
>> +
>> +    return prop;
>> +}
>> +
>> +static int __init allocate_domain_evtchn(const struct dt_device_node *node)
>> +{
>> +    const void *prop = NULL;
>> +    const __be32 *cell;
>> +    uint32_t 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 = get_evtchn_dt_property(evtchn_node);
>> +        /* If the property is not found, return without errors */
> 
> From the binding description, the property is not optional. So do we want to ignore the error? If you treat it as an error, then ...

Ok. I will treat it as en error.
> 
>> +        if ( !prop || IS_ERR(prop) )
>> +            return IS_ERR(prop) ? PTR_ERR(prop) : 0;
> 
> ... you could return ERR_PTR(-ENOMEM) instead of NULL and then simplify this code with:

Ack. 
> 
>> +
>> +        cell = (const __be32 *)prop;
> 
> prop is a void pointer. So the cast is unnecessary.

Ack. 
> 
>> +        domU1_port = dt_next_cell(1, &cell);
>> +        remote_phandle = dt_next_cell(1, &cell);
> The code is also duplicated below for the remote port. I think it would be better if this is part of your helper get_evtchn_dt_property().

Ack .
> 
>> +
>> +        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 = get_evtchn_dt_property(remote_node);
>> +        /* If the property is not found, return without errors */
>> +        if ( !prop || IS_ERR(prop) )
>> +            return IS_ERR(prop) ? PTR_ERR(prop) : 0;
>> +
>> +        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;
>> +        }
> 
> The loop could be avoided if you stash the domid in the field 'used_by' of the device-tree node when the domain is created.

Ok. I will used the “used_by” to store the domid.
> 
>> +
>> +        if ( !d1 && dt_device_is_compatible(node, "multiboot,kernel") )
>> +            d1 = hardware_domain;
>> +
>> +        if ( !d2 && dt_device_is_compatible(dt_get_parent(remote_node),
>> +                                            "multiboot,kernel") )
>> +            d2 = hardware_domain;
> 
> Any particular reason to handle the hardware domain differently?

As I have not set the "d->arch.node” for hwdom that why there is differently handling for hwdom.
I will try to use the “used_by” and will avoid this handling.

> 
>> +
>> +        if ( !d1 || !d2 )
>> +        {
>> +            printk(XENLOG_ERR "evtchn: could not find domains\n" );
>> +            return -EINVAL;
>> +        }
>> +
>> +        alloc_unbound.dom = d1->domain_id;
>> +        alloc_unbound.remote_dom = d2->domain_id;
>> +
>> +        rc = evtchn_alloc_unbound(&alloc_unbound, domU1_port);
>> +        if ( rc < 0 && rc != -EBUSY )
> 
> Please explain in a comment why you want to handle -EBUSY differently.

-EBUSY is used to check if evtchn is not already created while we scanning
the evtchn dt nodes.

> 
>> +        {
>> +            printk(XENLOG_ERR
>> +                   "evtchn_alloc_unbound() failure (Error %d) \n", rc);
>> +            return rc;
>> +        }
>> +
>> +        bind_interdomain.remote_dom  = d1->domain_id;
>> +        bind_interdomain.remote_port = domU1_port;
>> +
>> +        rc = evtchn_bind_interdomain(&bind_interdomain, d2, domU2_port);
>> +        if ( rc < 0 && rc != -EBUSY )
> 
> AFAIU, EBUSY only tells you the port is been used. It doesn't tell you the link is the same. So I think you want to also confirm that to avoid to continuing with the wrong setup.

Ack. 
> 
>> +        {
>> +            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;
> 
> AFAICT, all the users below can deal with constisfied node. So I think you want to add 'const' here.

Ack.
> 
>> +    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 +3493,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 you follow my suggestion above, this should not be necessary. However, if this is still needed for some reason, then I think we should also set d->arch.node for the Hardware Domain and ...

Ack. 
> 
>>            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 cd9ce19b4b..51192b28ee 100644
>> --- a/xen/arch/arm/include/asm/domain.h
>> +++ b/xen/arch/arm/include/asm/domain.h
>> @@ -105,6 +105,7 @@ struct arch_domain
>>  #endif
>>        bool directmap;
>> +    struct dt_device_node *node;
> 
> ... this should be const as the node shouldn't be modifiable.

Ack. 

Regards,
Rahul

[PATCH v2 7/7] 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>
---
Changes in v2:
 - no change
---
---
 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 5101bca979..bd8b8475b7 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 )
@@ -3343,11 +3350,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;
@@ -3526,6 +3533,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 v2 7/7] xen/arm: introduce new xen,enhanced property value

[Julien Grall]

Hi,

On 19/08/2022 11:02, Rahul Singh wrote:
> Introduce a new "xen,enhanced" dom0less property value "evtchn" to
> enable/disable event-channel interfaces for dom0less guests.

The documentation in docs/misc/arm/device-tree/booting.txt is missing. 
Also, you probably wants to update docs/feature/dom0less.pandoc because 
the section "PV drivers" suggests that if the property "xen,enhanced" is 
specified, then we would end up to allocate information for PV drivers.

AFAIU, this is not the case when "evtchn" is specified.

> 
> 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>
> ---
> Changes in v2:
>   - no change
> ---
> ---
>   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 5101bca979..bd8b8475b7 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;
>   


The diff below is quite difficult to read. I have applied to have a 
look. You seem to have simply indented the code and now some of the
lines are over the 80 characters mark.

Ideally, I would like to avoid large 'if'. So I would suggest to either
re-ordering the code or split in multiple functions.

However, reading the binding of "xen,xen", the property "reg" and 
"interrupts" are not optional.

I also don't think can make them optional because some OSes may not boot 
if it can't find one of the property.

In any case, at minimum you should explain why this is fine to make them 
optional.

[...]


> -    /*
> -     * 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 )

So I understand why you want to make the first part optional. But this 
is not clear why this one become conditional to "dom0less_evtchn". Do 
you have any plan to only present the node "xen,xen" where neither event 
channels nor PV interfaces would be used?

> +    {
> +        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 )

I think the first part of the if can be removed because you can't do 
without event channel.

>       {
>           ret = make_hypervisor_node(d, kinfo, addrcells, sizecells);
>           if ( ret )
> @@ -3343,11 +3350,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;
> @@ -3526,6 +3533,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;

So technically, the event channel interface is still exposed even if 
this is false. This is because we are still allocate the PPI and set the 
number of events to a non-zero value.

IMHO, if dom0less_evtchn is false, then we should properly disable the 
event channels interface not just hide it.

Cheers,


-- 
Julien Grall

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

[Rahul Singh]

Hi Julien,

> On 23 Aug 2022, at 11:05 am, Julien Grall <julien@xen.org> wrote:
> 
> Hi,
> 
> On 19/08/2022 11:02, Rahul Singh wrote:
>> Introduce a new "xen,enhanced" dom0less property value "evtchn" to
>> enable/disable event-channel interfaces for dom0less guests.
> 
> The documentation in docs/misc/arm/device-tree/booting.txt is missing. Also, you probably wants to update docs/feature/dom0less.pandoc because the section "PV drivers" suggests that if the property "xen,enhanced" is specified, then we would end up to allocate information for PV drivers.
> 
> AFAIU, this is not the case when "evtchn" is specified.
> 
>> 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>
>> ---
>> Changes in v2:
>>  - no change
>> ---
>> ---
>>  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 5101bca979..bd8b8475b7 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;
>>  
> 
> 
> The diff below is quite difficult to read. I have applied to have a look. You seem to have simply indented the code and now some of the
> lines are over the 80 characters mark.
> 
> Ideally, I would like to avoid large 'if'. So I would suggest to either
> re-ordering the code or split in multiple functions.
> 
> However, reading the binding of "xen,xen", the property "reg" and "interrupts" are not optional.
> 
> I also don't think can make them optional because some OSes may not boot if it can't find one of the property.
> 
> In any case, at minimum you should explain why this is fine to make them optional.
> 
> [...]

If we want to expose the "reg” and “interrupts” property always to guests and these properties are not 
optional then we can discard this patch and add support for "xen,enhanced” property for domUs for
static evtchn to work for domUs

Please let me know your view on this.

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index bfe7bc6b36..a1e23eee59 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -3562,12 +3562,7 @@ static int __init construct_domU(struct domain *d,
   if ( rc == -EILSEQ ||
     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”);
-  }
   if ( vcpu_create(d, 0) == NULL )
     return -ENOMEM;
 
 
Regards,
Rahul

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

[Julien Grall]

On 24/08/2022 13:15, Rahul Singh wrote:
> Hi Julien,

Hi Rahul,

> Please let me know your view on this.
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index bfe7bc6b36..a1e23eee59 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -3562,12 +3562,7 @@ static int __init construct_domU(struct domain *d,
>     if ( rc == -EILSEQ ||
>       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”);
> -  }

You can't use "xen,enhanced" without dom0. In fact, you will end up to 
dereference NULL in alloc_xenstore_evtchn(). That's because 
"xen,enhanced" means the domain will be able to use Xenstored.

Now if you want to support your feature without a dom0. Then I think we 
want to introduce an option which would be the same as "xen,enhanced" 
but doesn't expose Xenstored.

Cheers,

-- 
Julien Grall

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

[Rahul Singh]

Hi Julien,

> On 24 Aug 2022, at 1:59 pm, Julien Grall <julien@xen.org> wrote:
> 
> 
> 
> On 24/08/2022 13:15, Rahul Singh wrote:
>> Hi Julien,
> 
> Hi Rahul,
> 
>> Please let me know your view on this.
>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>> index bfe7bc6b36..a1e23eee59 100644
>> --- a/xen/arch/arm/domain_build.c
>> +++ b/xen/arch/arm/domain_build.c
>> @@ -3562,12 +3562,7 @@ static int __init construct_domU(struct domain *d,
>>    if ( rc == -EILSEQ ||
>>      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”);
>> -  }
> 
> You can't use "xen,enhanced" without dom0. In fact, you will end up to dereference NULL in alloc_xenstore_evtchn(). That's because "xen,enhanced" means the domain will be able to use Xenstored.
> 
> Now if you want to support your feature without a dom0. Then I think we want to introduce an option which would be the same as "xen,enhanced" but doesn't expose Xenstored.

If we modify the patch as below we can use the "xen,enhanced" for domUs without dom0.
I tested the patch and its works fine. Do you see any issue with this approach?


diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index cffd508af2..870846b742 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -3568,12 +3568,7 @@ static int __init construct_domU(struct domain *d,
     if ( rc == -EILSEQ ||
          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");
-    }
 
     if ( vcpu_create(d, 0) == NULL )
         return -ENOMEM;
@@ -3613,9 +3608,8 @@ static int __init construct_domU(struct domain *d,
     if ( rc < 0 )
         return rc;
 
-    if ( kinfo.dom0less_enhanced )
+    if ( kinfo.dom0less_enhanced && hardware_domain )
     {
-        ASSERT(hardware_domain);
         rc = alloc_xenstore_evtchn(d);
         if ( rc < 0 )
             return rc;
 

Regards,
Rahul

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

[Julien Grall]

On 24/08/2022 15:42, Rahul Singh wrote:
>> On 24 Aug 2022, at 1:59 pm, Julien Grall <julien@xen.org> wrote:
>>
>>
>>
>> On 24/08/2022 13:15, Rahul Singh wrote:
>>> Hi Julien,
>>
>> Hi Rahul,
>>
>>> Please let me know your view on this.
>>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>>> index bfe7bc6b36..a1e23eee59 100644
>>> --- a/xen/arch/arm/domain_build.c
>>> +++ b/xen/arch/arm/domain_build.c
>>> @@ -3562,12 +3562,7 @@ static int __init construct_domU(struct domain *d,
>>>     if ( rc == -EILSEQ ||
>>>       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”);
>>> -  }
>>
>> You can't use "xen,enhanced" without dom0. In fact, you will end up to dereference NULL in alloc_xenstore_evtchn(). That's because "xen,enhanced" means the domain will be able to use Xenstored.
>>
>> Now if you want to support your feature without a dom0. Then I think we want to introduce an option which would be the same as "xen,enhanced" but doesn't expose Xenstored.
> 
> If we modify the patch as below we can use the "xen,enhanced" for domUs without dom0.
> I tested the patch and its works fine. Do you see any issue with this approach?

Yes. For two reasons:
  1) It is muddying the meaning of "xen,enhanced". In particular a user 
may not realize that Xenstore is not available if dom0 is not present.
  2) It would be more complicated to handle the case where Xenstored 
lives in a non-dom0 domain. I am not aware of anyone wanting this on Arm 
yet, but I don't want to close the door.

So if you want to support create "xen,xen" without all the rest. Then I 
think we need a different property value. I don't have a good suggestion 
for the name.

Cheers,

-- 
Julien Grall

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

[Rahul Singh]

Hi Julien

> On 24 Aug 2022, at 4:36 pm, Julien Grall <julien@xen.org> wrote:
> 
> On 24/08/2022 15:42, Rahul Singh wrote:
>>> On 24 Aug 2022, at 1:59 pm, Julien Grall <julien@xen.org> wrote:
>>> 
>>> 
>>> 
>>> On 24/08/2022 13:15, Rahul Singh wrote:
>>>> Hi Julien,
>>> 
>>> Hi Rahul,
>>> 
>>>> Please let me know your view on this.
>>>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>>>> index bfe7bc6b36..a1e23eee59 100644
>>>> --- a/xen/arch/arm/domain_build.c
>>>> +++ b/xen/arch/arm/domain_build.c
>>>> @@ -3562,12 +3562,7 @@ static int __init construct_domU(struct domain *d,
>>>>    if ( rc == -EILSEQ ||
>>>>      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”);
>>>> -  }
>>> 
>>> You can't use "xen,enhanced" without dom0. In fact, you will end up to dereference NULL in alloc_xenstore_evtchn(). That's because "xen,enhanced" means the domain will be able to use Xenstored.
>>> 
>>> Now if you want to support your feature without a dom0. Then I think we want to introduce an option which would be the same as "xen,enhanced" but doesn't expose Xenstored.
>> If we modify the patch as below we can use the "xen,enhanced" for domUs without dom0.
>> I tested the patch and its works fine. Do you see any issue with this approach?
> 
> Yes. For two reasons:
> 1) It is muddying the meaning of "xen,enhanced". In particular a user may not realize that Xenstore is not available if dom0 is not present.
> 2) It would be more complicated to handle the case where Xenstored lives in a non-dom0 domain. I am not aware of anyone wanting this on Arm yet, but I don't want to close the door.
> 
> So if you want to support create "xen,xen" without all the rest. Then I think we need a different property value. I don't have a good suggestion for the name.

Is that okay if we use the earlier approach, when user set  "xen,enhanced = evtchn” we will not call alloc_xenstore_evtchn()  
but we create hypervisor node with all fields.
 

Regards,
Rahul

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

[Stefano Stabellini]

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

On Wed, 24 Aug 2022, Rahul Singh wrote:
> > On 24 Aug 2022, at 4:36 pm, Julien Grall <julien@xen.org> wrote:
> > On 24/08/2022 15:42, Rahul Singh wrote:
> >>> On 24 Aug 2022, at 1:59 pm, Julien Grall <julien@xen.org> wrote:
> >>> 
> >>> 
> >>> 
> >>> On 24/08/2022 13:15, Rahul Singh wrote:
> >>>> Hi Julien,
> >>> 
> >>> Hi Rahul,
> >>> 
> >>>> Please let me know your view on this.
> >>>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> >>>> index bfe7bc6b36..a1e23eee59 100644
> >>>> --- a/xen/arch/arm/domain_build.c
> >>>> +++ b/xen/arch/arm/domain_build.c
> >>>> @@ -3562,12 +3562,7 @@ static int __init construct_domU(struct domain *d,
> >>>>    if ( rc == -EILSEQ ||
> >>>>      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”);
> >>>> -  }
> >>> 
> >>> You can't use "xen,enhanced" without dom0. In fact, you will end up to dereference NULL in alloc_xenstore_evtchn(). That's because "xen,enhanced" means the domain will be able to use Xenstored.
> >>> 
> >>> Now if you want to support your feature without a dom0. Then I think we want to introduce an option which would be the same as "xen,enhanced" but doesn't expose Xenstored.
> >> If we modify the patch as below we can use the "xen,enhanced" for domUs without dom0.
> >> I tested the patch and its works fine. Do you see any issue with this approach?
> > 
> > Yes. For two reasons:
> > 1) It is muddying the meaning of "xen,enhanced". In particular a user may not realize that Xenstore is not available if dom0 is not present.
> > 2) It would be more complicated to handle the case where Xenstored lives in a non-dom0 domain. I am not aware of anyone wanting this on Arm yet, but I don't want to close the door.
> > 
> > So if you want to support create "xen,xen" without all the rest. Then I think we need a different property value. I don't have a good suggestion for the name.
> 
> Is that okay if we use the earlier approach, when user set  "xen,enhanced = evtchn” we will not call alloc_xenstore_evtchn()  
> but we create hypervisor node with all fields.

Thinking more about this, today xen,enhanced has the implication that:

- the guest will get a regular and complete "xen,xen" node in device tree
- xenstore and PV drivers will be available (full Xen interfaces support)

We don't necessarely imply that dom0 is required (from a domU point of
view) but we do imply that xenstore+evtchn+gnttab will be available to
the domU.

Now, static event channels are different. They don't require xenstore
and they don't require gnttab.

It is as if the current xen,enhanced node actually meant:

  xen,enhanced = "xenstore,gnttab,evtchn";

and now we are only enabling a subset:

  xen,enhanced = "evtchn";

Is that a correct understanding?


If so, we can clarify that:

  xen,enhanced;

it is a convenient shortend for:

  xen,enhanced = "xenstore,gnttab,evtchn";

and that other combinations are also acceptable, e.g.:

  xen,enhanced = "gnttab";
  xen,enhanced = "evtchn";
  xen,enhanced = "evtchn,gnttab";

It is OK to panic if the user specifies an option that is currently
unsupported (e.g. "gnttab").

In practice xenstore requires both gnttab and evtchn, I don't know if we
want to write that down in the device tree bindings. We could panic if
the user specifies: xen,enhanced = "xenstore,evtchn";

What do you guys think?

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

[Julien Grall]

Hi Stefano,

On 24/08/2022 22:59, Stefano Stabellini wrote:
> On Wed, 24 Aug 2022, Rahul Singh wrote:
>>> On 24 Aug 2022, at 4:36 pm, Julien Grall <julien@xen.org> wrote:
>>> On 24/08/2022 15:42, Rahul Singh wrote:
>>>>> On 24 Aug 2022, at 1:59 pm, Julien Grall <julien@xen.org> wrote:
>>>>>
>>>>>
>>>>>
>>>>> On 24/08/2022 13:15, Rahul Singh wrote:
>>>>>> Hi Julien,
>>>>>
>>>>> Hi Rahul,
>>>>>
>>>>>> Please let me know your view on this.
>>>>>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>>>>>> index bfe7bc6b36..a1e23eee59 100644
>>>>>> --- a/xen/arch/arm/domain_build.c
>>>>>> +++ b/xen/arch/arm/domain_build.c
>>>>>> @@ -3562,12 +3562,7 @@ static int __init construct_domU(struct domain *d,
>>>>>>     if ( rc == -EILSEQ ||
>>>>>>       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”);
>>>>>> -  }
>>>>>
>>>>> You can't use "xen,enhanced" without dom0. In fact, you will end up to dereference NULL in alloc_xenstore_evtchn(). That's because "xen,enhanced" means the domain will be able to use Xenstored.
>>>>>
>>>>> Now if you want to support your feature without a dom0. Then I think we want to introduce an option which would be the same as "xen,enhanced" but doesn't expose Xenstored.
>>>> If we modify the patch as below we can use the "xen,enhanced" for domUs without dom0.
>>>> I tested the patch and its works fine. Do you see any issue with this approach?
>>>
>>> Yes. For two reasons:
>>> 1) It is muddying the meaning of "xen,enhanced". In particular a user may not realize that Xenstore is not available if dom0 is not present.
>>> 2) It would be more complicated to handle the case where Xenstored lives in a non-dom0 domain. I am not aware of anyone wanting this on Arm yet, but I don't want to close the door.
>>>
>>> So if you want to support create "xen,xen" without all the rest. Then I think we need a different property value. I don't have a good suggestion for the name.
>>
>> Is that okay if we use the earlier approach, when user set  "xen,enhanced = evtchn” we will not call alloc_xenstore_evtchn()
>> but we create hypervisor node with all fields.
> 
> Thinking more about this, today xen,enhanced has the implication that:
> 
> - the guest will get a regular and complete "xen,xen" node in device tree
> - xenstore and PV drivers will be available (full Xen interfaces support)
> 
> We don't necessarely imply that dom0 is required (from a domU point of
> view) but we do imply that xenstore+evtchn+gnttab will be available to
> the domU.
> 
> Now, static event channels are different. They don't require xenstore
> and they don't require gnttab.
> 
> It is as if the current xen,enhanced node actually meant:
> 
>    xen,enhanced = "xenstore,gnttab,evtchn";

Correct.

> 
> and now we are only enabling a subset:
> 
>    xen,enhanced = "evtchn";
> 
> Is that a correct understanding?

Yes with some cavears (see below).

> 
> 
> If so, we can clarify that:
> 
>    xen,enhanced;
> 
> it is a convenient shortend for:
> 
>    xen,enhanced = "xenstore,gnttab,evtchn";
> 
> and that other combinations are also acceptable, e.g.:
> 
>    xen,enhanced = "gnttab";
>    xen,enhanced = "evtchn";
>    xen,enhanced = "evtchn,gnttab";
> 
> It is OK to panic if the user specifies an option that is currently
> unsupported (e.g. "gnttab").

So today, if you create the node "xen,xen", the guest will expect to be 
able to use both grant-table and event channel.

Therefore, in the list above, the only configuration we can sensibly 
support without any major rework is "evtchn,gnttab".

If we want to support "evtchn" or "gnttab" only. Then we likely need to 
define a new binding (or new version) because neither "regs" nor 
"interrupts" are optional (although a guest OS is free to ignore them).

> 
> In practice xenstore requires both gnttab and evtchn, I don't know if we
> want to write that down in the device tree bindings. We could panic if
> the user specifies: xen,enhanced = "xenstore,evtchn";

I think the interface for dom0less domUs is quite messy at the moment. 
Even if we don't advertise the support for event channel and 
grant-table, hypercalls. They are still accessible if the guest wish to 
do so.

If we decide to introduce "evtchn", "gnttab" & co then we should also 
make sure that if "evtchn" is not specified then we are not allowing the 
guest to allocate any event channel (or map the grant-table).

Otherwise, this is pointless if we try to tell the user "evtchn", 
"gnttab"...

And just to be clear, I would be perfectly happy to break anyone trying
to use event channel without "xen,enanced" because we didn't advertise 
the feature. So they should not use it.

Cheers,

-- 
Julien Grall

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

[Stefano Stabellini]

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

On Wed, 24 Aug 2022, Julien Grall wrote:
> On 24/08/2022 22:59, Stefano Stabellini wrote:
> > On Wed, 24 Aug 2022, Rahul Singh wrote:
> > > > On 24 Aug 2022, at 4:36 pm, Julien Grall <julien@xen.org> wrote:
> > > > On 24/08/2022 15:42, Rahul Singh wrote:
> > > > > > On 24 Aug 2022, at 1:59 pm, Julien Grall <julien@xen.org> wrote:
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > On 24/08/2022 13:15, Rahul Singh wrote:
> > > > > > > Hi Julien,
> > > > > > 
> > > > > > Hi Rahul,
> > > > > > 
> > > > > > > Please let me know your view on this.
> > > > > > > diff --git a/xen/arch/arm/domain_build.c
> > > > > > > b/xen/arch/arm/domain_build.c
> > > > > > > index bfe7bc6b36..a1e23eee59 100644
> > > > > > > --- a/xen/arch/arm/domain_build.c
> > > > > > > +++ b/xen/arch/arm/domain_build.c
> > > > > > > @@ -3562,12 +3562,7 @@ static int __init construct_domU(struct
> > > > > > > domain *d,
> > > > > > >     if ( rc == -EILSEQ ||
> > > > > > >       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”);
> > > > > > > -  }
> > > > > > 
> > > > > > You can't use "xen,enhanced" without dom0. In fact, you will end up
> > > > > > to dereference NULL in alloc_xenstore_evtchn(). That's because
> > > > > > "xen,enhanced" means the domain will be able to use Xenstored.
> > > > > > 
> > > > > > Now if you want to support your feature without a dom0. Then I think
> > > > > > we want to introduce an option which would be the same as
> > > > > > "xen,enhanced" but doesn't expose Xenstored.
> > > > > If we modify the patch as below we can use the "xen,enhanced" for
> > > > > domUs without dom0.
> > > > > I tested the patch and its works fine. Do you see any issue with this
> > > > > approach?
> > > > 
> > > > Yes. For two reasons:
> > > > 1) It is muddying the meaning of "xen,enhanced". In particular a user
> > > > may not realize that Xenstore is not available if dom0 is not present.
> > > > 2) It would be more complicated to handle the case where Xenstored lives
> > > > in a non-dom0 domain. I am not aware of anyone wanting this on Arm yet,
> > > > but I don't want to close the door.
> > > > 
> > > > So if you want to support create "xen,xen" without all the rest. Then I
> > > > think we need a different property value. I don't have a good suggestion
> > > > for the name.
> > > 
> > > Is that okay if we use the earlier approach, when user set  "xen,enhanced
> > > = evtchn” we will not call alloc_xenstore_evtchn()
> > > but we create hypervisor node with all fields.
> > 
> > Thinking more about this, today xen,enhanced has the implication that:
> > 
> > - the guest will get a regular and complete "xen,xen" node in device tree
> > - xenstore and PV drivers will be available (full Xen interfaces support)
> > 
> > We don't necessarely imply that dom0 is required (from a domU point of
> > view) but we do imply that xenstore+evtchn+gnttab will be available to
> > the domU.
> > 
> > Now, static event channels are different. They don't require xenstore
> > and they don't require gnttab.
> > 
> > It is as if the current xen,enhanced node actually meant:
> > 
> >    xen,enhanced = "xenstore,gnttab,evtchn";
> 
> Correct.
> 
> > 
> > and now we are only enabling a subset:
> > 
> >    xen,enhanced = "evtchn";
> > 
> > Is that a correct understanding?
> 
> Yes with some cavears (see below).
> 
> > 
> > 
> > If so, we can clarify that:
> > 
> >    xen,enhanced;
> > 
> > it is a convenient shortend for:
> > 
> >    xen,enhanced = "xenstore,gnttab,evtchn";
> > 
> > and that other combinations are also acceptable, e.g.:
> > 
> >    xen,enhanced = "gnttab";
> >    xen,enhanced = "evtchn";
> >    xen,enhanced = "evtchn,gnttab";
> > 
> > It is OK to panic if the user specifies an option that is currently
> > unsupported (e.g. "gnttab").
> 
> So today, if you create the node "xen,xen", the guest will expect to be able
> to use both grant-table and event channel.
> 
> Therefore, in the list above, the only configuration we can sensibly support
> without any major rework is "evtchn,gnttab".
> 
> If we want to support "evtchn" or "gnttab" only. Then we likely need to define
> a new binding (or new version) because neither "regs" nor "interrupts" are
> optional (although a guest OS is free to ignore them).

Yes I think you are right. I also broadly agree with the rest of your
reply.

Thinking about it and given the above, we only need 2 "levels" of
enhancement:

1) everything: xenstore, gnttab, evtchn
2) gnttab, evtchn, but not xenstore

Nothing else is really possible because, as Julien pointed out,
"xen,enhanced" implies the xen,xen node in the domU device tree and in
turn that node implies both evtchn and gnttab.

xenstore is separate and is detected using HVM_PARAM_STORE_EVTCHN and
HVM_PARAM_STORE_PFN anyway.

So I think we just need to add a way to express 2). We could do
something like:

  xen,enhanced = "evtchn,gnttab";

Or we could use a new separate option like Julien initially suggested,
e.g.:

  xen,enhanced-no-xenstore;

"xen,enhanced-no-xenstore" is a terrible name actually, but just to
explain what I am thinking :-)

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

[Bertrand Marquis]

Hi,

> On 25 Aug 2022, at 02:10, Stefano Stabellini <sstabellini@kernel.org> wrote:
> 
> On Wed, 24 Aug 2022, Julien Grall wrote:
>> On 24/08/2022 22:59, Stefano Stabellini wrote:
>>> On Wed, 24 Aug 2022, Rahul Singh wrote:
>>>>> On 24 Aug 2022, at 4:36 pm, Julien Grall <julien@xen.org> wrote:
>>>>> On 24/08/2022 15:42, Rahul Singh wrote:
>>>>>>> On 24 Aug 2022, at 1:59 pm, Julien Grall <julien@xen.org> wrote:
>>>>>>> 
>>>>>>> 
>>>>>>> 
>>>>>>> On 24/08/2022 13:15, Rahul Singh wrote:
>>>>>>>> Hi Julien,
>>>>>>> 
>>>>>>> Hi Rahul,
>>>>>>> 
>>>>>>>> Please let me know your view on this.
>>>>>>>> diff --git a/xen/arch/arm/domain_build.c
>>>>>>>> b/xen/arch/arm/domain_build.c
>>>>>>>> index bfe7bc6b36..a1e23eee59 100644
>>>>>>>> --- a/xen/arch/arm/domain_build.c
>>>>>>>> +++ b/xen/arch/arm/domain_build.c
>>>>>>>> @@ -3562,12 +3562,7 @@ static int __init construct_domU(struct
>>>>>>>> domain *d,
>>>>>>>>    if ( rc == -EILSEQ ||
>>>>>>>>      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”);
>>>>>>>> -  }
>>>>>>> 
>>>>>>> You can't use "xen,enhanced" without dom0. In fact, you will end up
>>>>>>> to dereference NULL in alloc_xenstore_evtchn(). That's because
>>>>>>> "xen,enhanced" means the domain will be able to use Xenstored.
>>>>>>> 
>>>>>>> Now if you want to support your feature without a dom0. Then I think
>>>>>>> we want to introduce an option which would be the same as
>>>>>>> "xen,enhanced" but doesn't expose Xenstored.
>>>>>> If we modify the patch as below we can use the "xen,enhanced" for
>>>>>> domUs without dom0.
>>>>>> I tested the patch and its works fine. Do you see any issue with this
>>>>>> approach?
>>>>> 
>>>>> Yes. For two reasons:
>>>>> 1) It is muddying the meaning of "xen,enhanced". In particular a user
>>>>> may not realize that Xenstore is not available if dom0 is not present.
>>>>> 2) It would be more complicated to handle the case where Xenstored lives
>>>>> in a non-dom0 domain. I am not aware of anyone wanting this on Arm yet,
>>>>> but I don't want to close the door.
>>>>> 
>>>>> So if you want to support create "xen,xen" without all the rest. Then I
>>>>> think we need a different property value. I don't have a good suggestion
>>>>> for the name.
>>>> 
>>>> Is that okay if we use the earlier approach, when user set  "xen,enhanced
>>>> = evtchn” we will not call alloc_xenstore_evtchn()
>>>> but we create hypervisor node with all fields.
>>> 
>>> Thinking more about this, today xen,enhanced has the implication that:
>>> 
>>> - the guest will get a regular and complete "xen,xen" node in device tree
>>> - xenstore and PV drivers will be available (full Xen interfaces support)
>>> 
>>> We don't necessarely imply that dom0 is required (from a domU point of
>>> view) but we do imply that xenstore+evtchn+gnttab will be available to
>>> the domU.
>>> 
>>> Now, static event channels are different. They don't require xenstore
>>> and they don't require gnttab.
>>> 
>>> It is as if the current xen,enhanced node actually meant:
>>> 
>>>   xen,enhanced = "xenstore,gnttab,evtchn";
>> 
>> Correct.
>> 
>>> 
>>> and now we are only enabling a subset:
>>> 
>>>   xen,enhanced = "evtchn";
>>> 
>>> Is that a correct understanding?
>> 
>> Yes with some cavears (see below).
>> 
>>> 
>>> 
>>> If so, we can clarify that:
>>> 
>>>   xen,enhanced;
>>> 
>>> it is a convenient shortend for:
>>> 
>>>   xen,enhanced = "xenstore,gnttab,evtchn";
>>> 
>>> and that other combinations are also acceptable, e.g.:
>>> 
>>>   xen,enhanced = "gnttab";
>>>   xen,enhanced = "evtchn";
>>>   xen,enhanced = "evtchn,gnttab";
>>> 
>>> It is OK to panic if the user specifies an option that is currently
>>> unsupported (e.g. "gnttab").
>> 
>> So today, if you create the node "xen,xen", the guest will expect to be able
>> to use both grant-table and event channel.
>> 
>> Therefore, in the list above, the only configuration we can sensibly support
>> without any major rework is "evtchn,gnttab".
>> 
>> If we want to support "evtchn" or "gnttab" only. Then we likely need to define
>> a new binding (or new version) because neither "regs" nor "interrupts" are
>> optional (although a guest OS is free to ignore them).
> 
> Yes I think you are right. I also broadly agree with the rest of your
> reply.
> 
> Thinking about it and given the above, we only need 2 "levels" of
> enhancement:
> 
> 1) everything: xenstore, gnttab, evtchn
> 2) gnttab, evtchn, but not xenstore
> 
> Nothing else is really possible because, as Julien pointed out,
> "xen,enhanced" implies the xen,xen node in the domU device tree and in
> turn that node implies both evtchn and gnttab.

So we could say that xen,enhanced always includes gnttab and Xenstore is optional.

> 
> xenstore is separate and is detected using HVM_PARAM_STORE_EVTCHN and
> HVM_PARAM_STORE_PFN anyway.

Ack, not having Xenstore should be handled properly using the params.

> 
> So I think we just need to add a way to express 2). We could do
> something like:
> 
>  xen,enhanced = "evtchn,gnttab";

I am a bit puzzled here as gnttab is always there.

> 
> Or we could use a new separate option like Julien initially suggested,
> e.g.:
> 
>  xen,enhanced-no-xenstore;
> 
> "xen,enhanced-no-xenstore" is a terrible name actually, but just to
> explain what I am thinking :-)

I think most common use case will be to have all, so make sense to allow to disable Xenstore.

How about:
xen,enhanced = “no-xenstore” ?

An other solution is to keep xen,enhanced as it is and introduce a new option:
Xen,no-xenstore

At the end Xenstore cannot be used if there is no Dom0 and that we can detect easily.
Also there is no solution at this stage to have an other domain then Dom0 providing
Xenstore (maybe in the long term someone will want to introduce that and we will need
a way to specify which domain is handling it).

So I still think that we could just say that Xenstore can only be active if there is a Dom0
and just disable Xenstore automatically if it is not the case.
If there is a dom0 and someone wants a guest without Xenstore, then we would need to
have the no-xenstore support.
But is it a use case ?

All in all, enhance dom0less was not supported before 4.17 so we will not create any
backward compatibility issue.

What do you guys think ?

Cheers
Bertrand

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

[Julien Grall]

On 25/08/2022 08:39, Bertrand Marquis wrote:
> Hi,
> 
>> On 25 Aug 2022, at 02:10, Stefano Stabellini <sstabellini@kernel.org> wrote:
>>
>> On Wed, 24 Aug 2022, Julien Grall wrote:
>>> On 24/08/2022 22:59, Stefano Stabellini wrote:
>>>> On Wed, 24 Aug 2022, Rahul Singh wrote:
>>>>>> On 24 Aug 2022, at 4:36 pm, Julien Grall <julien@xen.org> wrote:
>>>>>> On 24/08/2022 15:42, Rahul Singh wrote:
>>>>>>>> On 24 Aug 2022, at 1:59 pm, Julien Grall <julien@xen.org> wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> On 24/08/2022 13:15, Rahul Singh wrote:
>>>>>>>>> Hi Julien,
>>>>>>>>
>>>>>>>> Hi Rahul,
>>>>>>>>
>>>>>>>>> Please let me know your view on this.
>>>>>>>>> diff --git a/xen/arch/arm/domain_build.c
>>>>>>>>> b/xen/arch/arm/domain_build.c
>>>>>>>>> index bfe7bc6b36..a1e23eee59 100644
>>>>>>>>> --- a/xen/arch/arm/domain_build.c
>>>>>>>>> +++ b/xen/arch/arm/domain_build.c
>>>>>>>>> @@ -3562,12 +3562,7 @@ static int __init construct_domU(struct
>>>>>>>>> domain *d,
>>>>>>>>>     if ( rc == -EILSEQ ||
>>>>>>>>>       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”);
>>>>>>>>> -  }
>>>>>>>>
>>>>>>>> You can't use "xen,enhanced" without dom0. In fact, you will end up
>>>>>>>> to dereference NULL in alloc_xenstore_evtchn(). That's because
>>>>>>>> "xen,enhanced" means the domain will be able to use Xenstored.
>>>>>>>>
>>>>>>>> Now if you want to support your feature without a dom0. Then I think
>>>>>>>> we want to introduce an option which would be the same as
>>>>>>>> "xen,enhanced" but doesn't expose Xenstored.
>>>>>>> If we modify the patch as below we can use the "xen,enhanced" for
>>>>>>> domUs without dom0.
>>>>>>> I tested the patch and its works fine. Do you see any issue with this
>>>>>>> approach?
>>>>>>
>>>>>> Yes. For two reasons:
>>>>>> 1) It is muddying the meaning of "xen,enhanced". In particular a user
>>>>>> may not realize that Xenstore is not available if dom0 is not present.
>>>>>> 2) It would be more complicated to handle the case where Xenstored lives
>>>>>> in a non-dom0 domain. I am not aware of anyone wanting this on Arm yet,
>>>>>> but I don't want to close the door.
>>>>>>
>>>>>> So if you want to support create "xen,xen" without all the rest. Then I
>>>>>> think we need a different property value. I don't have a good suggestion
>>>>>> for the name.
>>>>>
>>>>> Is that okay if we use the earlier approach, when user set  "xen,enhanced
>>>>> = evtchn” we will not call alloc_xenstore_evtchn()
>>>>> but we create hypervisor node with all fields.
>>>>
>>>> Thinking more about this, today xen,enhanced has the implication that:
>>>>
>>>> - the guest will get a regular and complete "xen,xen" node in device tree
>>>> - xenstore and PV drivers will be available (full Xen interfaces support)
>>>>
>>>> We don't necessarely imply that dom0 is required (from a domU point of
>>>> view) but we do imply that xenstore+evtchn+gnttab will be available to
>>>> the domU.
>>>>
>>>> Now, static event channels are different. They don't require xenstore
>>>> and they don't require gnttab.
>>>>
>>>> It is as if the current xen,enhanced node actually meant:
>>>>
>>>>    xen,enhanced = "xenstore,gnttab,evtchn";
>>>
>>> Correct.
>>>
>>>>
>>>> and now we are only enabling a subset:
>>>>
>>>>    xen,enhanced = "evtchn";
>>>>
>>>> Is that a correct understanding?
>>>
>>> Yes with some cavears (see below).
>>>
>>>>
>>>>
>>>> If so, we can clarify that:
>>>>
>>>>    xen,enhanced;
>>>>
>>>> it is a convenient shortend for:
>>>>
>>>>    xen,enhanced = "xenstore,gnttab,evtchn";
>>>>
>>>> and that other combinations are also acceptable, e.g.:
>>>>
>>>>    xen,enhanced = "gnttab";
>>>>    xen,enhanced = "evtchn";
>>>>    xen,enhanced = "evtchn,gnttab";
>>>>
>>>> It is OK to panic if the user specifies an option that is currently
>>>> unsupported (e.g. "gnttab").
>>>
>>> So today, if you create the node "xen,xen", the guest will expect to be able
>>> to use both grant-table and event channel.
>>>
>>> Therefore, in the list above, the only configuration we can sensibly support
>>> without any major rework is "evtchn,gnttab".
>>>
>>> If we want to support "evtchn" or "gnttab" only. Then we likely need to define
>>> a new binding (or new version) because neither "regs" nor "interrupts" are
>>> optional (although a guest OS is free to ignore them).
>>
>> Yes I think you are right. I also broadly agree with the rest of your
>> reply.
>>
>> Thinking about it and given the above, we only need 2 "levels" of
>> enhancement:
>>
>> 1) everything: xenstore, gnttab, evtchn
>> 2) gnttab, evtchn, but not xenstore
>>
>> Nothing else is really possible because, as Julien pointed out,
>> "xen,enhanced" implies the xen,xen node in the domU device tree and in
>> turn that node implies both evtchn and gnttab.
> 
> So we could say that xen,enhanced always includes gnttab and Xenstore is optional.

Not really, Xenstore has always been part of the story in Xen. So I 
think making it optional for "xen,enhanced" is going to make more 
difficult for user to understand what the meaning of the option (in 
particular that in the future we may want to support Xenstored in a 
separate domain).

>> So I think we just need to add a way to express 2). We could do
>> something like:
>>
>>   xen,enhanced = "evtchn,gnttab";
> 
> I am a bit puzzled here as gnttab is always there.

What do you mean?

> 
>>
>> Or we could use a new separate option like Julien initially suggested,
>> e.g.:
>>
>>   xen,enhanced-no-xenstore;
>>
>> "xen,enhanced-no-xenstore" is a terrible name actually, but just to
>> explain what I am thinking :-)
> 
> I think most common use case will be to have all, so make sense to allow to disable Xenstore.
> 
> How about:
> xen,enhanced = “no-xenstore” ?

I would be fine with it.

> 
> An other solution is to keep xen,enhanced as it is and introduce a new option:
> Xen,no-xenstore

I don't like the idea of introducing yet another option.

> 
> At the end Xenstore cannot be used if there is no Dom0 and that we can detect easily.
> Also there is no solution at this stage to have an other domain then Dom0 providing
> Xenstore (maybe in the long term someone will want to introduce that and we will need
> a way to specify which domain is handling it).
> 
> So I still think that we could just say that Xenstore can only be active if there is a Dom0
> and just disable Xenstore automatically if it is not the case.

See above about disabling Xenstore automatically.

> If there is a dom0 and someone wants a guest without Xenstore, then we would need to
> have the no-xenstore support.
> But is it a use case ?

Do you mean when "xen,enhanced" is specified? If yes, this could be 
useful if one want to limit the interface exposed to the guest.

> 
> All in all, enhance dom0less was not supported before 4.17 so we will not create any
> backward compatibility issue.

I agree we still have the flexibility to change.

Cheers,

-- 
Julien Grall

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

[Bertrand Marquis]

Hi Julien,

> On 25 Aug 2022, at 10:37, Julien Grall <julien@xen.org> wrote:
> 
> 
> 
> On 25/08/2022 08:39, Bertrand Marquis wrote:
>> Hi,
>>> On 25 Aug 2022, at 02:10, Stefano Stabellini <sstabellini@kernel.org> wrote:
>>> 
>>> On Wed, 24 Aug 2022, Julien Grall wrote:
>>>> On 24/08/2022 22:59, Stefano Stabellini wrote:
>>>>> On Wed, 24 Aug 2022, Rahul Singh wrote:
>>>>>>> On 24 Aug 2022, at 4:36 pm, Julien Grall <julien@xen.org> wrote:
>>>>>>> On 24/08/2022 15:42, Rahul Singh wrote:
>>>>>>>>> On 24 Aug 2022, at 1:59 pm, Julien Grall <julien@xen.org> wrote:
>>>>>>>>> 
>>>>>>>>> 
>>>>>>>>> 
>>>>>>>>> On 24/08/2022 13:15, Rahul Singh wrote:
>>>>>>>>>> Hi Julien,
>>>>>>>>> 
>>>>>>>>> Hi Rahul,
>>>>>>>>> 
>>>>>>>>>> Please let me know your view on this.
>>>>>>>>>> diff --git a/xen/arch/arm/domain_build.c
>>>>>>>>>> b/xen/arch/arm/domain_build.c
>>>>>>>>>> index bfe7bc6b36..a1e23eee59 100644
>>>>>>>>>> --- a/xen/arch/arm/domain_build.c
>>>>>>>>>> +++ b/xen/arch/arm/domain_build.c
>>>>>>>>>> @@ -3562,12 +3562,7 @@ static int __init construct_domU(struct
>>>>>>>>>> domain *d,
>>>>>>>>>>    if ( rc == -EILSEQ ||
>>>>>>>>>>      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”);
>>>>>>>>>> -  }
>>>>>>>>> 
>>>>>>>>> You can't use "xen,enhanced" without dom0. In fact, you will end up
>>>>>>>>> to dereference NULL in alloc_xenstore_evtchn(). That's because
>>>>>>>>> "xen,enhanced" means the domain will be able to use Xenstored.
>>>>>>>>> 
>>>>>>>>> Now if you want to support your feature without a dom0. Then I think
>>>>>>>>> we want to introduce an option which would be the same as
>>>>>>>>> "xen,enhanced" but doesn't expose Xenstored.
>>>>>>>> If we modify the patch as below we can use the "xen,enhanced" for
>>>>>>>> domUs without dom0.
>>>>>>>> I tested the patch and its works fine. Do you see any issue with this
>>>>>>>> approach?
>>>>>>> 
>>>>>>> Yes. For two reasons:
>>>>>>> 1) It is muddying the meaning of "xen,enhanced". In particular a user
>>>>>>> may not realize that Xenstore is not available if dom0 is not present.
>>>>>>> 2) It would be more complicated to handle the case where Xenstored lives
>>>>>>> in a non-dom0 domain. I am not aware of anyone wanting this on Arm yet,
>>>>>>> but I don't want to close the door.
>>>>>>> 
>>>>>>> So if you want to support create "xen,xen" without all the rest. Then I
>>>>>>> think we need a different property value. I don't have a good suggestion
>>>>>>> for the name.
>>>>>> 
>>>>>> Is that okay if we use the earlier approach, when user set  "xen,enhanced
>>>>>> = evtchn” we will not call alloc_xenstore_evtchn()
>>>>>> but we create hypervisor node with all fields.
>>>>> 
>>>>> Thinking more about this, today xen,enhanced has the implication that:
>>>>> 
>>>>> - the guest will get a regular and complete "xen,xen" node in device tree
>>>>> - xenstore and PV drivers will be available (full Xen interfaces support)
>>>>> 
>>>>> We don't necessarely imply that dom0 is required (from a domU point of
>>>>> view) but we do imply that xenstore+evtchn+gnttab will be available to
>>>>> the domU.
>>>>> 
>>>>> Now, static event channels are different. They don't require xenstore
>>>>> and they don't require gnttab.
>>>>> 
>>>>> It is as if the current xen,enhanced node actually meant:
>>>>> 
>>>>>   xen,enhanced = "xenstore,gnttab,evtchn";
>>>> 
>>>> Correct.
>>>> 
>>>>> 
>>>>> and now we are only enabling a subset:
>>>>> 
>>>>>   xen,enhanced = "evtchn";
>>>>> 
>>>>> Is that a correct understanding?
>>>> 
>>>> Yes with some cavears (see below).
>>>> 
>>>>> 
>>>>> 
>>>>> If so, we can clarify that:
>>>>> 
>>>>>   xen,enhanced;
>>>>> 
>>>>> it is a convenient shortend for:
>>>>> 
>>>>>   xen,enhanced = "xenstore,gnttab,evtchn";
>>>>> 
>>>>> and that other combinations are also acceptable, e.g.:
>>>>> 
>>>>>   xen,enhanced = "gnttab";
>>>>>   xen,enhanced = "evtchn";
>>>>>   xen,enhanced = "evtchn,gnttab";
>>>>> 
>>>>> It is OK to panic if the user specifies an option that is currently
>>>>> unsupported (e.g. "gnttab").
>>>> 
>>>> So today, if you create the node "xen,xen", the guest will expect to be able
>>>> to use both grant-table and event channel.
>>>> 
>>>> Therefore, in the list above, the only configuration we can sensibly support
>>>> without any major rework is "evtchn,gnttab".
>>>> 
>>>> If we want to support "evtchn" or "gnttab" only. Then we likely need to define
>>>> a new binding (or new version) because neither "regs" nor "interrupts" are
>>>> optional (although a guest OS is free to ignore them).
>>> 
>>> Yes I think you are right. I also broadly agree with the rest of your
>>> reply.
>>> 
>>> Thinking about it and given the above, we only need 2 "levels" of
>>> enhancement:
>>> 
>>> 1) everything: xenstore, gnttab, evtchn
>>> 2) gnttab, evtchn, but not xenstore
>>> 
>>> Nothing else is really possible because, as Julien pointed out,
>>> "xen,enhanced" implies the xen,xen node in the domU device tree and in
>>> turn that node implies both evtchn and gnttab.
>> So we could say that xen,enhanced always includes gnttab and Xenstore is optional.
> 
> Not really, Xenstore has always been part of the story in Xen. So I think making it optional for "xen,enhanced" is going to make more difficult for user to understand what the meaning of the option (in particular that in the future we may want to support Xenstored in a separate domain).

Sorry wrong formulation, here I was meaning that we just need a solution to disable Xenstore (should still be here by default when supported).

> 
>>> So I think we just need to add a way to express 2). We could do
>>> something like:
>>> 
>>>  xen,enhanced = "evtchn,gnttab";
>> I am a bit puzzled here as gnttab is always there.
> 
> What do you mean?

Asking the user to specify gnttab in the list even though it is not supported to not have it in the list.

> 
>>> 
>>> Or we could use a new separate option like Julien initially suggested,
>>> e.g.:
>>> 
>>>  xen,enhanced-no-xenstore;
>>> 
>>> "xen,enhanced-no-xenstore" is a terrible name actually, but just to
>>> explain what I am thinking :-)
>> I think most common use case will be to have all, so make sense to allow to disable Xenstore.
>> How about:
>> xen,enhanced = “no-xenstore” ?
> 
> I would be fine with it.
> 
>> An other solution is to keep xen,enhanced as it is and introduce a new option:
>> Xen,no-xenstore
> 
> I don't like the idea of introducing yet another option.
> 
>> At the end Xenstore cannot be used if there is no Dom0 and that we can detect easily.
>> Also there is no solution at this stage to have an other domain then Dom0 providing
>> Xenstore (maybe in the long term someone will want to introduce that and we will need
>> a way to specify which domain is handling it).
>> So I still think that we could just say that Xenstore can only be active if there is a Dom0
>> and just disable Xenstore automatically if it is not the case.
> 
> See above about disabling Xenstore automatically.

Right now Xenstore can only work with a dom0 and if someone wants to have an other domain to provide it we would need a way to specify which one in the configuration.
So in a configuration without dom0, I still think that not enabling Xenstore automatically is ok.

> 
>> If there is a dom0 and someone wants a guest without Xenstore, then we would need to
>> have the no-xenstore support.
>> But is it a use case ?
> 
> Do you mean when "xen,enhanced" is specified? If yes, this could be useful if one want to limit the interface exposed to the guest.

How about the following:
Xen,enhanced: gnttab, events and Xenstore if there is a dom0
Xen,enhanced = “[no-]xenstore,[no-]evtchn,[no-]gnttab” for when the user wants to explicitly specify what he wants (and Xen stopping on unsupported configuration).
   In this I would allow to provide any combinations of the 3

Bertrand

> 
>> All in all, enhance dom0less was not supported before 4.17 so we will not create any
>> backward compatibility issue.
> 
> I agree we still have the flexibility to change.
> 
> Cheers,
> 
> -- 
> Julien Grall

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

[Stefano Stabellini]

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

On Thu, 25 Aug 2022, Bertrand Marquis wrote:
> > On 25 Aug 2022, at 10:37, Julien Grall <julien@xen.org> wrote:
> > On 25/08/2022 08:39, Bertrand Marquis wrote:
> >> Hi,
> >>> On 25 Aug 2022, at 02:10, Stefano Stabellini <sstabellini@kernel.org> wrote:
> >>> 
> >>> On Wed, 24 Aug 2022, Julien Grall wrote:
> >>>> On 24/08/2022 22:59, Stefano Stabellini wrote:
> >>>>> On Wed, 24 Aug 2022, Rahul Singh wrote:
> >>>>>>> On 24 Aug 2022, at 4:36 pm, Julien Grall <julien@xen.org> wrote:
> >>>>>>> On 24/08/2022 15:42, Rahul Singh wrote:
> >>>>>>>>> On 24 Aug 2022, at 1:59 pm, Julien Grall <julien@xen.org> wrote:
> >>>>>>>>> 
> >>>>>>>>> 
> >>>>>>>>> 
> >>>>>>>>> On 24/08/2022 13:15, Rahul Singh wrote:
> >>>>>>>>>> Hi Julien,
> >>>>>>>>> 
> >>>>>>>>> Hi Rahul,
> >>>>>>>>> 
> >>>>>>>>>> Please let me know your view on this.
> >>>>>>>>>> diff --git a/xen/arch/arm/domain_build.c
> >>>>>>>>>> b/xen/arch/arm/domain_build.c
> >>>>>>>>>> index bfe7bc6b36..a1e23eee59 100644
> >>>>>>>>>> --- a/xen/arch/arm/domain_build.c
> >>>>>>>>>> +++ b/xen/arch/arm/domain_build.c
> >>>>>>>>>> @@ -3562,12 +3562,7 @@ static int __init construct_domU(struct
> >>>>>>>>>> domain *d,
> >>>>>>>>>>    if ( rc == -EILSEQ ||
> >>>>>>>>>>      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”);
> >>>>>>>>>> -  }
> >>>>>>>>> 
> >>>>>>>>> You can't use "xen,enhanced" without dom0. In fact, you will end up
> >>>>>>>>> to dereference NULL in alloc_xenstore_evtchn(). That's because
> >>>>>>>>> "xen,enhanced" means the domain will be able to use Xenstored.
> >>>>>>>>> 
> >>>>>>>>> Now if you want to support your feature without a dom0. Then I think
> >>>>>>>>> we want to introduce an option which would be the same as
> >>>>>>>>> "xen,enhanced" but doesn't expose Xenstored.
> >>>>>>>> If we modify the patch as below we can use the "xen,enhanced" for
> >>>>>>>> domUs without dom0.
> >>>>>>>> I tested the patch and its works fine. Do you see any issue with this
> >>>>>>>> approach?
> >>>>>>> 
> >>>>>>> Yes. For two reasons:
> >>>>>>> 1) It is muddying the meaning of "xen,enhanced". In particular a user
> >>>>>>> may not realize that Xenstore is not available if dom0 is not present.
> >>>>>>> 2) It would be more complicated to handle the case where Xenstored lives
> >>>>>>> in a non-dom0 domain. I am not aware of anyone wanting this on Arm yet,
> >>>>>>> but I don't want to close the door.
> >>>>>>> 
> >>>>>>> So if you want to support create "xen,xen" without all the rest. Then I
> >>>>>>> think we need a different property value. I don't have a good suggestion
> >>>>>>> for the name.
> >>>>>> 
> >>>>>> Is that okay if we use the earlier approach, when user set  "xen,enhanced
> >>>>>> = evtchn” we will not call alloc_xenstore_evtchn()
> >>>>>> but we create hypervisor node with all fields.
> >>>>> 
> >>>>> Thinking more about this, today xen,enhanced has the implication that:
> >>>>> 
> >>>>> - the guest will get a regular and complete "xen,xen" node in device tree
> >>>>> - xenstore and PV drivers will be available (full Xen interfaces support)
> >>>>> 
> >>>>> We don't necessarely imply that dom0 is required (from a domU point of
> >>>>> view) but we do imply that xenstore+evtchn+gnttab will be available to
> >>>>> the domU.
> >>>>> 
> >>>>> Now, static event channels are different. They don't require xenstore
> >>>>> and they don't require gnttab.
> >>>>> 
> >>>>> It is as if the current xen,enhanced node actually meant:
> >>>>> 
> >>>>>   xen,enhanced = "xenstore,gnttab,evtchn";
> >>>> 
> >>>> Correct.
> >>>> 
> >>>>> 
> >>>>> and now we are only enabling a subset:
> >>>>> 
> >>>>>   xen,enhanced = "evtchn";
> >>>>> 
> >>>>> Is that a correct understanding?
> >>>> 
> >>>> Yes with some cavears (see below).
> >>>> 
> >>>>> 
> >>>>> 
> >>>>> If so, we can clarify that:
> >>>>> 
> >>>>>   xen,enhanced;
> >>>>> 
> >>>>> it is a convenient shortend for:
> >>>>> 
> >>>>>   xen,enhanced = "xenstore,gnttab,evtchn";
> >>>>> 
> >>>>> and that other combinations are also acceptable, e.g.:
> >>>>> 
> >>>>>   xen,enhanced = "gnttab";
> >>>>>   xen,enhanced = "evtchn";
> >>>>>   xen,enhanced = "evtchn,gnttab";
> >>>>> 
> >>>>> It is OK to panic if the user specifies an option that is currently
> >>>>> unsupported (e.g. "gnttab").
> >>>> 
> >>>> So today, if you create the node "xen,xen", the guest will expect to be able
> >>>> to use both grant-table and event channel.
> >>>> 
> >>>> Therefore, in the list above, the only configuration we can sensibly support
> >>>> without any major rework is "evtchn,gnttab".
> >>>> 
> >>>> If we want to support "evtchn" or "gnttab" only. Then we likely need to define
> >>>> a new binding (or new version) because neither "regs" nor "interrupts" are
> >>>> optional (although a guest OS is free to ignore them).
> >>> 
> >>> Yes I think you are right. I also broadly agree with the rest of your
> >>> reply.
> >>> 
> >>> Thinking about it and given the above, we only need 2 "levels" of
> >>> enhancement:
> >>> 
> >>> 1) everything: xenstore, gnttab, evtchn
> >>> 2) gnttab, evtchn, but not xenstore
> >>> 
> >>> Nothing else is really possible because, as Julien pointed out,
> >>> "xen,enhanced" implies the xen,xen node in the domU device tree and in
> >>> turn that node implies both evtchn and gnttab.
> >> So we could say that xen,enhanced always includes gnttab and Xenstore is optional.
> > 
> > Not really, Xenstore has always been part of the story in Xen. So I think making it optional for "xen,enhanced" is going to make more difficult for user to understand what the meaning of the option (in particular that in the future we may want to support Xenstored in a separate domain).
> 
> Sorry wrong formulation, here I was meaning that we just need a solution to disable Xenstore (should still be here by default when supported).
> 
> > 
> >>> So I think we just need to add a way to express 2). We could do
> >>> something like:
> >>> 
> >>>  xen,enhanced = "evtchn,gnttab";
> >> I am a bit puzzled here as gnttab is always there.
> > 
> > What do you mean?
> 
> Asking the user to specify gnttab in the list even though it is not supported to not have it in the list.
> 
> > 
> >>> 
> >>> Or we could use a new separate option like Julien initially suggested,
> >>> e.g.:
> >>> 
> >>>  xen,enhanced-no-xenstore;
> >>> 
> >>> "xen,enhanced-no-xenstore" is a terrible name actually, but just to
> >>> explain what I am thinking :-)
> >> I think most common use case will be to have all, so make sense to allow to disable Xenstore.
> >> How about:
> >> xen,enhanced = “no-xenstore” ?
> > 
> > I would be fine with it.

We have agreement on this, so I would say let's keep it simple and go
with this option.


> >> An other solution is to keep xen,enhanced as it is and introduce a new option:
> >> Xen,no-xenstore
> > 
> > I don't like the idea of introducing yet another option.
> > 
> >> At the end Xenstore cannot be used if there is no Dom0 and that we can detect easily.
> >> Also there is no solution at this stage to have an other domain then Dom0 providing
> >> Xenstore (maybe in the long term someone will want to introduce that and we will need
> >> a way to specify which domain is handling it).
> >> So I still think that we could just say that Xenstore can only be active if there is a Dom0
> >> and just disable Xenstore automatically if it is not the case.
> > 
> > See above about disabling Xenstore automatically.
> 
> Right now Xenstore can only work with a dom0 and if someone wants to have an other domain to provide it we would need a way to specify which one in the configuration.
> So in a configuration without dom0, I still think that not enabling Xenstore automatically is ok.
> 
> > 
> >> If there is a dom0 and someone wants a guest without Xenstore, then we would need to
> >> have the no-xenstore support.
> >> But is it a use case ?
> > 
> > Do you mean when "xen,enhanced" is specified? If yes, this could be useful if one want to limit the interface exposed to the guest.
> 
> How about the following:
> Xen,enhanced: gnttab, events and Xenstore if there is a dom0
> Xen,enhanced = “[no-]xenstore,[no-]evtchn,[no-]gnttab” for when the user wants to explicitly specify what he wants (and Xen stopping on unsupported configuration).
>    In this I would allow to provide any combinations of the 3

I am OK with what you wrote as well, but considering the additional
complexity that no-gnttab and no-evtchn entail given that they cannot be
actually disabled today, I suggest to keep it simple and go with:

xen,enhanced = "no-xenstore"

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

[Bertrand Marquis]

Hi Stefano,

> On 25 Aug 2022, at 18:44, Stefano Stabellini <sstabellini@kernel.org> wrote:
> 
> On Thu, 25 Aug 2022, Bertrand Marquis wrote:
>>> On 25 Aug 2022, at 10:37, Julien Grall <julien@xen.org> wrote:
>>> On 25/08/2022 08:39, Bertrand Marquis wrote:
>>>> Hi,
>>>>> On 25 Aug 2022, at 02:10, Stefano Stabellini <sstabellini@kernel.org> wrote:
>>>>> 
>>>>> On Wed, 24 Aug 2022, Julien Grall wrote:
>>>>>> On 24/08/2022 22:59, Stefano Stabellini wrote:
>>>>>>> On Wed, 24 Aug 2022, Rahul Singh wrote:
>>>>>>>>> On 24 Aug 2022, at 4:36 pm, Julien Grall <julien@xen.org> wrote:
>>>>>>>>> On 24/08/2022 15:42, Rahul Singh wrote:
>>>>>>>>>>> On 24 Aug 2022, at 1:59 pm, Julien Grall <julien@xen.org> wrote:
>>>>>>>>>>> 
>>>>>>>>>>> 
>>>>>>>>>>> 
>>>>>>>>>>> On 24/08/2022 13:15, Rahul Singh wrote:
>>>>>>>>>>>> Hi Julien,
>>>>>>>>>>> 
>>>>>>>>>>> Hi Rahul,
>>>>>>>>>>> 
>>>>>>>>>>>> Please let me know your view on this.
>>>>>>>>>>>> diff --git a/xen/arch/arm/domain_build.c
>>>>>>>>>>>> b/xen/arch/arm/domain_build.c
>>>>>>>>>>>> index bfe7bc6b36..a1e23eee59 100644
>>>>>>>>>>>> --- a/xen/arch/arm/domain_build.c
>>>>>>>>>>>> +++ b/xen/arch/arm/domain_build.c
>>>>>>>>>>>> @@ -3562,12 +3562,7 @@ static int __init construct_domU(struct
>>>>>>>>>>>> domain *d,
>>>>>>>>>>>>   if ( rc == -EILSEQ ||
>>>>>>>>>>>>     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”);
>>>>>>>>>>>> -  }
>>>>>>>>>>> 
>>>>>>>>>>> You can't use "xen,enhanced" without dom0. In fact, you will end up
>>>>>>>>>>> to dereference NULL in alloc_xenstore_evtchn(). That's because
>>>>>>>>>>> "xen,enhanced" means the domain will be able to use Xenstored.
>>>>>>>>>>> 
>>>>>>>>>>> Now if you want to support your feature without a dom0. Then I think
>>>>>>>>>>> we want to introduce an option which would be the same as
>>>>>>>>>>> "xen,enhanced" but doesn't expose Xenstored.
>>>>>>>>>> If we modify the patch as below we can use the "xen,enhanced" for
>>>>>>>>>> domUs without dom0.
>>>>>>>>>> I tested the patch and its works fine. Do you see any issue with this
>>>>>>>>>> approach?
>>>>>>>>> 
>>>>>>>>> Yes. For two reasons:
>>>>>>>>> 1) It is muddying the meaning of "xen,enhanced". In particular a user
>>>>>>>>> may not realize that Xenstore is not available if dom0 is not present.
>>>>>>>>> 2) It would be more complicated to handle the case where Xenstored lives
>>>>>>>>> in a non-dom0 domain. I am not aware of anyone wanting this on Arm yet,
>>>>>>>>> but I don't want to close the door.
>>>>>>>>> 
>>>>>>>>> So if you want to support create "xen,xen" without all the rest. Then I
>>>>>>>>> think we need a different property value. I don't have a good suggestion
>>>>>>>>> for the name.
>>>>>>>> 
>>>>>>>> Is that okay if we use the earlier approach, when user set  "xen,enhanced
>>>>>>>> = evtchn” we will not call alloc_xenstore_evtchn()
>>>>>>>> but we create hypervisor node with all fields.
>>>>>>> 
>>>>>>> Thinking more about this, today xen,enhanced has the implication that:
>>>>>>> 
>>>>>>> - the guest will get a regular and complete "xen,xen" node in device tree
>>>>>>> - xenstore and PV drivers will be available (full Xen interfaces support)
>>>>>>> 
>>>>>>> We don't necessarely imply that dom0 is required (from a domU point of
>>>>>>> view) but we do imply that xenstore+evtchn+gnttab will be available to
>>>>>>> the domU.
>>>>>>> 
>>>>>>> Now, static event channels are different. They don't require xenstore
>>>>>>> and they don't require gnttab.
>>>>>>> 
>>>>>>> It is as if the current xen,enhanced node actually meant:
>>>>>>> 
>>>>>>>  xen,enhanced = "xenstore,gnttab,evtchn";
>>>>>> 
>>>>>> Correct.
>>>>>> 
>>>>>>> 
>>>>>>> and now we are only enabling a subset:
>>>>>>> 
>>>>>>>  xen,enhanced = "evtchn";
>>>>>>> 
>>>>>>> Is that a correct understanding?
>>>>>> 
>>>>>> Yes with some cavears (see below).
>>>>>> 
>>>>>>> 
>>>>>>> 
>>>>>>> If so, we can clarify that:
>>>>>>> 
>>>>>>>  xen,enhanced;
>>>>>>> 
>>>>>>> it is a convenient shortend for:
>>>>>>> 
>>>>>>>  xen,enhanced = "xenstore,gnttab,evtchn";
>>>>>>> 
>>>>>>> and that other combinations are also acceptable, e.g.:
>>>>>>> 
>>>>>>>  xen,enhanced = "gnttab";
>>>>>>>  xen,enhanced = "evtchn";
>>>>>>>  xen,enhanced = "evtchn,gnttab";
>>>>>>> 
>>>>>>> It is OK to panic if the user specifies an option that is currently
>>>>>>> unsupported (e.g. "gnttab").
>>>>>> 
>>>>>> So today, if you create the node "xen,xen", the guest will expect to be able
>>>>>> to use both grant-table and event channel.
>>>>>> 
>>>>>> Therefore, in the list above, the only configuration we can sensibly support
>>>>>> without any major rework is "evtchn,gnttab".
>>>>>> 
>>>>>> If we want to support "evtchn" or "gnttab" only. Then we likely need to define
>>>>>> a new binding (or new version) because neither "regs" nor "interrupts" are
>>>>>> optional (although a guest OS is free to ignore them).
>>>>> 
>>>>> Yes I think you are right. I also broadly agree with the rest of your
>>>>> reply.
>>>>> 
>>>>> Thinking about it and given the above, we only need 2 "levels" of
>>>>> enhancement:
>>>>> 
>>>>> 1) everything: xenstore, gnttab, evtchn
>>>>> 2) gnttab, evtchn, but not xenstore
>>>>> 
>>>>> Nothing else is really possible because, as Julien pointed out,
>>>>> "xen,enhanced" implies the xen,xen node in the domU device tree and in
>>>>> turn that node implies both evtchn and gnttab.
>>>> So we could say that xen,enhanced always includes gnttab and Xenstore is optional.
>>> 
>>> Not really, Xenstore has always been part of the story in Xen. So I think making it optional for "xen,enhanced" is going to make more difficult for user to understand what the meaning of the option (in particular that in the future we may want to support Xenstored in a separate domain).
>> 
>> Sorry wrong formulation, here I was meaning that we just need a solution to disable Xenstore (should still be here by default when supported).
>> 
>>> 
>>>>> So I think we just need to add a way to express 2). We could do
>>>>> something like:
>>>>> 
>>>>> xen,enhanced = "evtchn,gnttab";
>>>> I am a bit puzzled here as gnttab is always there.
>>> 
>>> What do you mean?
>> 
>> Asking the user to specify gnttab in the list even though it is not supported to not have it in the list.
>> 
>>> 
>>>>> 
>>>>> Or we could use a new separate option like Julien initially suggested,
>>>>> e.g.:
>>>>> 
>>>>> xen,enhanced-no-xenstore;
>>>>> 
>>>>> "xen,enhanced-no-xenstore" is a terrible name actually, but just to
>>>>> explain what I am thinking :-)
>>>> I think most common use case will be to have all, so make sense to allow to disable Xenstore.
>>>> How about:
>>>> xen,enhanced = “no-xenstore” ?
>>> 
>>> I would be fine with it.
> 
> We have agreement on this, so I would say let's keep it simple and go
> with this option.
> 
> 
>>>> An other solution is to keep xen,enhanced as it is and introduce a new option:
>>>> Xen,no-xenstore
>>> 
>>> I don't like the idea of introducing yet another option.
>>> 
>>>> At the end Xenstore cannot be used if there is no Dom0 and that we can detect easily.
>>>> Also there is no solution at this stage to have an other domain then Dom0 providing
>>>> Xenstore (maybe in the long term someone will want to introduce that and we will need
>>>> a way to specify which domain is handling it).
>>>> So I still think that we could just say that Xenstore can only be active if there is a Dom0
>>>> and just disable Xenstore automatically if it is not the case.
>>> 
>>> See above about disabling Xenstore automatically.
>> 
>> Right now Xenstore can only work with a dom0 and if someone wants to have an other domain to provide it we would need a way to specify which one in the configuration.
>> So in a configuration without dom0, I still think that not enabling Xenstore automatically is ok.
>> 
>>> 
>>>> If there is a dom0 and someone wants a guest without Xenstore, then we would need to
>>>> have the no-xenstore support.
>>>> But is it a use case ?
>>> 
>>> Do you mean when "xen,enhanced" is specified? If yes, this could be useful if one want to limit the interface exposed to the guest.
>> 
>> How about the following:
>> Xen,enhanced: gnttab, events and Xenstore if there is a dom0
>> Xen,enhanced = “[no-]xenstore,[no-]evtchn,[no-]gnttab” for when the user wants to explicitly specify what he wants (and Xen stopping on unsupported configuration).
>>   In this I would allow to provide any combinations of the 3
> 
> I am OK with what you wrote as well, but considering the additional
> complexity that no-gnttab and no-evtchn entail given that they cannot be
> actually disabled today, I suggest to keep it simple and go with:
> 
> xen,enhanced = "no-xenstore"

I agree with that.

Cheers
Bertrand