From: Luca Miccio <lucmiccio@gmail.com>

The xenstore event channel will be allocated for dom0less domains. It is
necessary to have access to the evtchn_alloc_unbound function to do
that.

Factor out the part that actually allocates the event channel from
evtchn_alloc_unbound and introduce this new function as
_evtchn_alloc_unbound. (xsm_evtchn_unbound wouldn't work for a call
originated from Xen.)

Signed-off-by: Luca Miccio <lucmiccio@gmail.com>
Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
CC: Julien Grall <julien@xen.org>
CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
CC: Bertrand Marquis <bertrand.marquis@arm.com>
CC: Andrew Cooper <andrew.cooper3@citrix.com>
CC: George Dunlap <george.dunlap@citrix.com>
CC: Jan Beulich <jbeulich@suse.com>
CC: Wei Liu <wl@xen.org>
---
 xen/common/event_channel.c | 49 +++++++++++++++++++++++++-------------
 xen/include/xen/event.h    |  3 +++
 2 files changed, 36 insertions(+), 16 deletions(-)

diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
index da88ad141a..8a19bbf7ae 100644
--- a/xen/common/event_channel.c
+++ b/xen/common/event_channel.c
@@ -18,6 +18,7 @@
 
 #include <xen/init.h>
 #include <xen/lib.h>
+#include <xen/err.h>
 #include <xen/errno.h>
 #include <xen/sched.h>
 #include <xen/irq.h>
@@ -284,11 +285,32 @@ void evtchn_free(struct domain *d, struct evtchn *chn)
     xsm_evtchn_close_post(chn);
 }
 
-static int evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc)
+struct evtchn *_evtchn_alloc_unbound(struct domain *d, domid_t remote_dom)
 {
     struct evtchn *chn;
+    int port;
+
+    if ( (port = get_free_port(d)) < 0 )
+        return ERR_PTR(port);
+    chn = evtchn_from_port(d, port);
+
+    evtchn_write_lock(chn);
+
+    chn->state = ECS_UNBOUND;
+    if ( (chn->u.unbound.remote_domid = remote_dom) == DOMID_SELF )
+        chn->u.unbound.remote_domid = current->domain->domain_id;
+    evtchn_port_init(d, chn);
+
+    evtchn_write_unlock(chn);
+
+    return chn;
+}
+
+static int evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc)
+{
+    struct evtchn *chn = NULL;
     struct domain *d;
-    int            port, rc;
+    int            rc;
     domid_t        dom = alloc->dom;
 
     d = rcu_lock_domain_by_any_id(dom);
@@ -297,27 +319,22 @@ static int evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc)
 
     spin_lock(&d->event_lock);
 
-    if ( (port = get_free_port(d)) < 0 )
-        ERROR_EXIT_DOM(port, d);
-    chn = evtchn_from_port(d, port);
+    chn = _evtchn_alloc_unbound(d, alloc->remote_dom);
+    if ( IS_ERR(chn) )
+    {
+        rc = PTR_ERR(chn);
+        ERROR_EXIT_DOM(rc, d);
+    }
 
     rc = xsm_evtchn_unbound(XSM_TARGET, d, chn, alloc->remote_dom);
     if ( rc )
         goto out;
 
-    evtchn_write_lock(chn);
-
-    chn->state = ECS_UNBOUND;
-    if ( (chn->u.unbound.remote_domid = alloc->remote_dom) == DOMID_SELF )
-        chn->u.unbound.remote_domid = current->domain->domain_id;
-    evtchn_port_init(d, chn);
-
-    evtchn_write_unlock(chn);
-
-    alloc->port = port;
+    alloc->port = chn->port;
 
  out:
-    check_free_port(d, port);
+    if ( chn != NULL )
+        check_free_port(d, chn->port);
     spin_unlock(&d->event_lock);
     rcu_unlock_domain(d);
 
diff --git a/xen/include/xen/event.h b/xen/include/xen/event.h
index 21c95e14fd..6aedbccbf1 100644
--- a/xen/include/xen/event.h
+++ b/xen/include/xen/event.h
@@ -68,6 +68,9 @@ int evtchn_close(struct domain *d1, int port1, bool guest);
 /* Free an event channel. */
 void evtchn_free(struct domain *d, struct evtchn *chn);
 
+/* Create a new event channel port */
+struct evtchn *_evtchn_alloc_unbound(struct domain *d, domid_t remote_dom);
+
 /* Allocate a specific event channel port. */
 int evtchn_allocate_port(struct domain *d, unsigned int port);
 
-- 
2.25.1

On 08.01.2022 01:49, Stefano Stabellini wrote:
> @@ -284,11 +285,32 @@ void evtchn_free(struct domain *d, struct evtchn *chn)
>      xsm_evtchn_close_post(chn);
>  }
>  
> -static int evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc)
> +struct evtchn *_evtchn_alloc_unbound(struct domain *d, domid_t remote_dom)

Function names want to be the other way around, to be in line with
naming rules of the C spec: The static function may be underscore-
prefixed, while the non-static one may not.

>  {
>      struct evtchn *chn;
> +    int port;
> +
> +    if ( (port = get_free_port(d)) < 0 )
> +        return ERR_PTR(port);
> +    chn = evtchn_from_port(d, port);
> +
> +    evtchn_write_lock(chn);
> +
> +    chn->state = ECS_UNBOUND;
> +    if ( (chn->u.unbound.remote_domid = remote_dom) == DOMID_SELF )
> +        chn->u.unbound.remote_domid = current->domain->domain_id;

I think the resolving of DOMID_SELF should remain in the caller, as I'm
pretty sure your planned new user(s) can't sensibly pass that value.

> +    evtchn_port_init(d, chn);
> +
> +    evtchn_write_unlock(chn);
> +
> +    return chn;
> +}
> +
> +static int evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc)
> +{
> +    struct evtchn *chn = NULL;

I don't think the initializer is needed.

> @@ -297,27 +319,22 @@ static int evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc)
>  
>      spin_lock(&d->event_lock);
>  
> -    if ( (port = get_free_port(d)) < 0 )
> -        ERROR_EXIT_DOM(port, d);
> -    chn = evtchn_from_port(d, port);
> +    chn = _evtchn_alloc_unbound(d, alloc->remote_dom);
> +    if ( IS_ERR(chn) )
> +    {
> +        rc = PTR_ERR(chn);
> +        ERROR_EXIT_DOM(rc, d);
> +    }
>  
>      rc = xsm_evtchn_unbound(XSM_TARGET, d, chn, alloc->remote_dom);
>      if ( rc )
>          goto out;
>  
> -    evtchn_write_lock(chn);
> -
> -    chn->state = ECS_UNBOUND;

This cannot be pulled ahead of the XSM check (or in general anything
potentially resulting in an error), as check_free_port() relies on
->state remaining ECS_FREE until it is known that the calling function
can't fail anymore.

> -    if ( (chn->u.unbound.remote_domid = alloc->remote_dom) == DOMID_SELF )
> -        chn->u.unbound.remote_domid = current->domain->domain_id;
> -    evtchn_port_init(d, chn);
> -
> -    evtchn_write_unlock(chn);
> -
> -    alloc->port = port;
> +    alloc->port = chn->port;
>  
>   out:
> -    check_free_port(d, port);
> +    if ( chn != NULL )
> +        check_free_port(d, chn->port);

Without the initializer above it'll then be more obvious that the
condition here needs to be !IS_ERR(chn).

Also (nit) please prefer the shorter "if ( chn )".

Overall I wonder in how far it would be possible to instead re-use PV
shim's "backdoor" into port allocation: evtchn_allocate_port() was
specifically made available for it, iirc.

Jan

On Mon, 10 Jan 2022, Jan Beulich wrote:
> On 08.01.2022 01:49, Stefano Stabellini wrote:
> > @@ -284,11 +285,32 @@ void evtchn_free(struct domain *d, struct evtchn *chn)
> >      xsm_evtchn_close_post(chn);
> >  }
> >  
> > -static int evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc)
> > +struct evtchn *_evtchn_alloc_unbound(struct domain *d, domid_t remote_dom)
> 
> Function names want to be the other way around, to be in line with
> naming rules of the C spec: The static function may be underscore-
> prefixed, while the non-static one may not.

OK


> >  {
> >      struct evtchn *chn;
> > +    int port;
> > +
> > +    if ( (port = get_free_port(d)) < 0 )
> > +        return ERR_PTR(port);
> > +    chn = evtchn_from_port(d, port);
> > +
> > +    evtchn_write_lock(chn);
> > +
> > +    chn->state = ECS_UNBOUND;
> > +    if ( (chn->u.unbound.remote_domid = remote_dom) == DOMID_SELF )
> > +        chn->u.unbound.remote_domid = current->domain->domain_id;
> 
> I think the resolving of DOMID_SELF should remain in the caller, as I'm
> pretty sure your planned new user(s) can't sensibly pass that value.

Yep, no problem


> > +    evtchn_port_init(d, chn);
> > +
> > +    evtchn_write_unlock(chn);
> > +
> > +    return chn;
> > +}
> > +
> > +static int evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc)
> > +{
> > +    struct evtchn *chn = NULL;
> 
> I don't think the initializer is needed.

OK


> > @@ -297,27 +319,22 @@ static int evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc)
> >  
> >      spin_lock(&d->event_lock);
> >  
> > -    if ( (port = get_free_port(d)) < 0 )
> > -        ERROR_EXIT_DOM(port, d);
> > -    chn = evtchn_from_port(d, port);
> > +    chn = _evtchn_alloc_unbound(d, alloc->remote_dom);
> > +    if ( IS_ERR(chn) )
> > +    {
> > +        rc = PTR_ERR(chn);
> > +        ERROR_EXIT_DOM(rc, d);
> > +    }
> >  
> >      rc = xsm_evtchn_unbound(XSM_TARGET, d, chn, alloc->remote_dom);
> >      if ( rc )
> >          goto out;
> >  
> > -    evtchn_write_lock(chn);
> > -
> > -    chn->state = ECS_UNBOUND;
> 
> This cannot be pulled ahead of the XSM check (or in general anything
> potentially resulting in an error), as check_free_port() relies on
> ->state remaining ECS_FREE until it is known that the calling function
> can't fail anymore.

OK, I didn't realize. Unfortunately it means we have to move setting
chn->state = ECS_UNBOUND to the caller.


> > -    if ( (chn->u.unbound.remote_domid = alloc->remote_dom) == DOMID_SELF )
> > -        chn->u.unbound.remote_domid = current->domain->domain_id;
> > -    evtchn_port_init(d, chn);
> > -
> > -    evtchn_write_unlock(chn);
> > -
> > -    alloc->port = port;
> > +    alloc->port = chn->port;
> >  
> >   out:
> > -    check_free_port(d, port);
> > +    if ( chn != NULL )
> > +        check_free_port(d, chn->port);
> 
> Without the initializer above it'll then be more obvious that the
> condition here needs to be !IS_ERR(chn).
> 
> Also (nit) please prefer the shorter "if ( chn )".
> 
> Overall I wonder in how far it would be possible to instead re-use PV
> shim's "backdoor" into port allocation: evtchn_allocate_port() was
> specifically made available for it, iirc.

I don't see an obvious way to do it. These are the 4 things we need to
do:

1) call get_free_port/evtchn_allocate_port
2) set state = ECS_UNBOUND
3) set remote_domid
4) call evtchn_port_init

It doesn't look like we could enhance evtchn_allocate_port to do 2) and
3). And probably even 4) couldn't be added to evtchn_allocate_port.

So basically it is like calling get_free_port() and do 2,3,4 ourselves
from the caller in xen/arch/arm/domain_build.c. But that might be a good
idea actually. Maybe we should leave evtchn_alloc_unbound unmodified and
instead open-code what we need to do in xen/arch/arm/domain_build.c.
This is how it would look like as a new function in
xen/arch/arm/domain_build.c:

static int alloc_xenstore_evtchn(struct domain *d)
{
    struct evtchn *chn;
    int port;

    if ( (port = get_free_port(d)) < 0 )
        return ERR_PTR(port);
    chn = evtchn_from_port(d, port);

    chn->state = ECS_UNBOUND;
    chn->u.unbound.remote_domid = hardware_domain->domain_id;
    evtchn_port_init(d, chn);

    return chn->port;
}

What do you think? It might not be worth introducing
evtchn_alloc_unbound / _evtchn_alloc_unbound for this?

I am happy to follow what you think is best.

Cheers,

Stefano

On 11.01.2022 23:49, Stefano Stabellini wrote:
> On Mon, 10 Jan 2022, Jan Beulich wrote:
>> On 08.01.2022 01:49, Stefano Stabellini wrote:
>>> @@ -284,11 +285,32 @@ void evtchn_free(struct domain *d, struct evtchn *chn)
>>>      xsm_evtchn_close_post(chn);
>>>  }
>>>  
>>> -static int evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc)
>>> +struct evtchn *_evtchn_alloc_unbound(struct domain *d, domid_t remote_dom)
>>
>> Function names want to be the other way around, to be in line with
>> naming rules of the C spec: The static function may be underscore-
>> prefixed, while the non-static one may not.
> 
> OK
> 
> 
>>>  {
>>>      struct evtchn *chn;
>>> +    int port;
>>> +
>>> +    if ( (port = get_free_port(d)) < 0 )
>>> +        return ERR_PTR(port);
>>> +    chn = evtchn_from_port(d, port);
>>> +
>>> +    evtchn_write_lock(chn);
>>> +
>>> +    chn->state = ECS_UNBOUND;
>>> +    if ( (chn->u.unbound.remote_domid = remote_dom) == DOMID_SELF )
>>> +        chn->u.unbound.remote_domid = current->domain->domain_id;
>>
>> I think the resolving of DOMID_SELF should remain in the caller, as I'm
>> pretty sure your planned new user(s) can't sensibly pass that value.
> 
> Yep, no problem
> 
> 
>>> +    evtchn_port_init(d, chn);
>>> +
>>> +    evtchn_write_unlock(chn);
>>> +
>>> +    return chn;
>>> +}
>>> +
>>> +static int evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc)
>>> +{
>>> +    struct evtchn *chn = NULL;
>>
>> I don't think the initializer is needed.
> 
> OK
> 
> 
>>> @@ -297,27 +319,22 @@ static int evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc)
>>>  
>>>      spin_lock(&d->event_lock);
>>>  
>>> -    if ( (port = get_free_port(d)) < 0 )
>>> -        ERROR_EXIT_DOM(port, d);
>>> -    chn = evtchn_from_port(d, port);
>>> +    chn = _evtchn_alloc_unbound(d, alloc->remote_dom);
>>> +    if ( IS_ERR(chn) )
>>> +    {
>>> +        rc = PTR_ERR(chn);
>>> +        ERROR_EXIT_DOM(rc, d);
>>> +    }
>>>  
>>>      rc = xsm_evtchn_unbound(XSM_TARGET, d, chn, alloc->remote_dom);
>>>      if ( rc )
>>>          goto out;
>>>  
>>> -    evtchn_write_lock(chn);
>>> -
>>> -    chn->state = ECS_UNBOUND;
>>
>> This cannot be pulled ahead of the XSM check (or in general anything
>> potentially resulting in an error), as check_free_port() relies on
>> ->state remaining ECS_FREE until it is known that the calling function
>> can't fail anymore.
> 
> OK, I didn't realize. Unfortunately it means we have to move setting
> chn->state = ECS_UNBOUND to the caller.
> 
> 
>>> -    if ( (chn->u.unbound.remote_domid = alloc->remote_dom) == DOMID_SELF )
>>> -        chn->u.unbound.remote_domid = current->domain->domain_id;
>>> -    evtchn_port_init(d, chn);
>>> -
>>> -    evtchn_write_unlock(chn);
>>> -
>>> -    alloc->port = port;
>>> +    alloc->port = chn->port;
>>>  
>>>   out:
>>> -    check_free_port(d, port);
>>> +    if ( chn != NULL )
>>> +        check_free_port(d, chn->port);
>>
>> Without the initializer above it'll then be more obvious that the
>> condition here needs to be !IS_ERR(chn).
>>
>> Also (nit) please prefer the shorter "if ( chn )".
>>
>> Overall I wonder in how far it would be possible to instead re-use PV
>> shim's "backdoor" into port allocation: evtchn_allocate_port() was
>> specifically made available for it, iirc.
> 
> I don't see an obvious way to do it. These are the 4 things we need to
> do:
> 
> 1) call get_free_port/evtchn_allocate_port
> 2) set state = ECS_UNBOUND
> 3) set remote_domid
> 4) call evtchn_port_init
> 
> It doesn't look like we could enhance evtchn_allocate_port to do 2) and
> 3). And probably even 4) couldn't be added to evtchn_allocate_port.
> 
> So basically it is like calling get_free_port() and do 2,3,4 ourselves
> from the caller in xen/arch/arm/domain_build.c. But that might be a good
> idea actually. Maybe we should leave evtchn_alloc_unbound unmodified and
> instead open-code what we need to do in xen/arch/arm/domain_build.c.

Right, that's what I was trying to hint at as an alternative.

Jan

> This is how it would look like as a new function in
> xen/arch/arm/domain_build.c:
> 
> static int alloc_xenstore_evtchn(struct domain *d)
> {
>     struct evtchn *chn;
>     int port;
> 
>     if ( (port = get_free_port(d)) < 0 )
>         return ERR_PTR(port);
>     chn = evtchn_from_port(d, port);
> 
>     chn->state = ECS_UNBOUND;
>     chn->u.unbound.remote_domid = hardware_domain->domain_id;
>     evtchn_port_init(d, chn);
> 
>     return chn->port;
> }
> 
> What do you think? It might not be worth introducing
> evtchn_alloc_unbound / _evtchn_alloc_unbound for this?
> 
> I am happy to follow what you think is best.
> 
> Cheers,
> 
> Stefano
> 

On Wed, 12 Jan 2022, Jan Beulich wrote:
> On 11.01.2022 23:49, Stefano Stabellini wrote:
> > On Mon, 10 Jan 2022, Jan Beulich wrote:
> >> On 08.01.2022 01:49, Stefano Stabellini wrote:
> >>> @@ -284,11 +285,32 @@ void evtchn_free(struct domain *d, struct evtchn *chn)
> >>>      xsm_evtchn_close_post(chn);
> >>>  }
> >>>  
> >>> -static int evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc)
> >>> +struct evtchn *_evtchn_alloc_unbound(struct domain *d, domid_t remote_dom)
> >>
> >> Function names want to be the other way around, to be in line with
> >> naming rules of the C spec: The static function may be underscore-
> >> prefixed, while the non-static one may not.
> > 
> > OK
> > 
> > 
> >>>  {
> >>>      struct evtchn *chn;
> >>> +    int port;
> >>> +
> >>> +    if ( (port = get_free_port(d)) < 0 )
> >>> +        return ERR_PTR(port);
> >>> +    chn = evtchn_from_port(d, port);
> >>> +
> >>> +    evtchn_write_lock(chn);
> >>> +
> >>> +    chn->state = ECS_UNBOUND;
> >>> +    if ( (chn->u.unbound.remote_domid = remote_dom) == DOMID_SELF )
> >>> +        chn->u.unbound.remote_domid = current->domain->domain_id;
> >>
> >> I think the resolving of DOMID_SELF should remain in the caller, as I'm
> >> pretty sure your planned new user(s) can't sensibly pass that value.
> > 
> > Yep, no problem
> > 
> > 
> >>> +    evtchn_port_init(d, chn);
> >>> +
> >>> +    evtchn_write_unlock(chn);
> >>> +
> >>> +    return chn;
> >>> +}
> >>> +
> >>> +static int evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc)
> >>> +{
> >>> +    struct evtchn *chn = NULL;
> >>
> >> I don't think the initializer is needed.
> > 
> > OK
> > 
> > 
> >>> @@ -297,27 +319,22 @@ static int evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc)
> >>>  
> >>>      spin_lock(&d->event_lock);
> >>>  
> >>> -    if ( (port = get_free_port(d)) < 0 )
> >>> -        ERROR_EXIT_DOM(port, d);
> >>> -    chn = evtchn_from_port(d, port);
> >>> +    chn = _evtchn_alloc_unbound(d, alloc->remote_dom);
> >>> +    if ( IS_ERR(chn) )
> >>> +    {
> >>> +        rc = PTR_ERR(chn);
> >>> +        ERROR_EXIT_DOM(rc, d);
> >>> +    }
> >>>  
> >>>      rc = xsm_evtchn_unbound(XSM_TARGET, d, chn, alloc->remote_dom);
> >>>      if ( rc )
> >>>          goto out;
> >>>  
> >>> -    evtchn_write_lock(chn);
> >>> -
> >>> -    chn->state = ECS_UNBOUND;
> >>
> >> This cannot be pulled ahead of the XSM check (or in general anything
> >> potentially resulting in an error), as check_free_port() relies on
> >> ->state remaining ECS_FREE until it is known that the calling function
> >> can't fail anymore.
> > 
> > OK, I didn't realize. Unfortunately it means we have to move setting
> > chn->state = ECS_UNBOUND to the caller.
> > 
> > 
> >>> -    if ( (chn->u.unbound.remote_domid = alloc->remote_dom) == DOMID_SELF )
> >>> -        chn->u.unbound.remote_domid = current->domain->domain_id;
> >>> -    evtchn_port_init(d, chn);
> >>> -
> >>> -    evtchn_write_unlock(chn);
> >>> -
> >>> -    alloc->port = port;
> >>> +    alloc->port = chn->port;
> >>>  
> >>>   out:
> >>> -    check_free_port(d, port);
> >>> +    if ( chn != NULL )
> >>> +        check_free_port(d, chn->port);
> >>
> >> Without the initializer above it'll then be more obvious that the
> >> condition here needs to be !IS_ERR(chn).
> >>
> >> Also (nit) please prefer the shorter "if ( chn )".
> >>
> >> Overall I wonder in how far it would be possible to instead re-use PV
> >> shim's "backdoor" into port allocation: evtchn_allocate_port() was
> >> specifically made available for it, iirc.
> > 
> > I don't see an obvious way to do it. These are the 4 things we need to
> > do:
> > 
> > 1) call get_free_port/evtchn_allocate_port
> > 2) set state = ECS_UNBOUND
> > 3) set remote_domid
> > 4) call evtchn_port_init
> > 
> > It doesn't look like we could enhance evtchn_allocate_port to do 2) and
> > 3). And probably even 4) couldn't be added to evtchn_allocate_port.
> > 
> > So basically it is like calling get_free_port() and do 2,3,4 ourselves
> > from the caller in xen/arch/arm/domain_build.c. But that might be a good
> > idea actually. Maybe we should leave evtchn_alloc_unbound unmodified and
> > instead open-code what we need to do in xen/arch/arm/domain_build.c.
> 
> Right, that's what I was trying to hint at as an alternative.

OK, I'll do that then