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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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