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

[Jan Beulich <jbeulich@suse.com>]

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