Re: [XEN PATCH 2/7] xen: introduce _evtchn_alloc_unbound

[Stefano Stabellini <sstabellini@kernel.org>]

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